obmarg / cynic Goto Github PK

Thinking I should write something with mdBook for this - maybe an inside->out guide, starting with how to use the selection set functions directly, then through to query_dsl, then through to the various derives?

Might be nice if we could just accept an ident here or something.

There are some places where I've been lazy and stuck unwraps. Should remove them

Split out of #57

• Copy buttons on the results tab (could leave this one till later)
• Package up in a CLI or similar to work around CORS (could also split this into it's own issue)
• Somewhere to add query options

To reproduce use cynic-querygen to generate for a query that has an enum hardcoded into it (not as a variable).

It will complain that "could not find enum: WhateverTheVariantIs"

Right now all IDs need to be cynic::Id. Might be nice to allow people to swap in their own scalars or something. Not sure - think about it.

Noticed that some of the built in derives added these annotations to their types:

#[automatically_derived]
#[allow(unused_qualifications)]

Might be something to look into using myself.

The ID scalar is built into GQL, so we should probably have a newtype for it.

Currently, cynic does not contain HTTP client support. It's not especially hard to write this support yourself (or copy from the various examples) but it's also not as simple as it could be. It might be nice to have HTTP client support added, behind a feature flag. Would just need to take the code from the examples (or documentation) and wrap it up in an easily callbable function.

We wouldn't want to bundle this with cynic by default, so would have to put this behind a feature flag or possibly in another crate (or possibly both).

Initially would like to support:

• Surf (#123)
• Reqwest async (#124)
• Reqwest blocking (#124)

Need far more thorough testing of things like cynic-querygen. Probably thinking a folder of snapshots (possibly hand-written results, possibly using insta), and a macro that generates tests based on the contents of the folder.

Defining the following type against SWAPI:

#[derive(cynic::QueryFragment)]
#[cynic(
    schema_path = "examples/examples/starwars.schema.graphql",
    query_module = "query_dsl",
    graphql_type = "Film"
)]
struct Film {
    title: String,
    director: String,
}

Leads to the following error:

error[E0308]: mismatched types
 --> examples/examples/starwars.rs:9:10
  |
9 | #[derive(cynic::QueryFragment)]
  |          ^^^^^^^^^^^^^^^^^^^^ expected struct `std::string::String`, found enum `std::option::Option`
  |
  = note: expected type `cynic::selection_set::SelectionSet<'_, std::string::String, _>`
             found type `cynic::selection_set::SelectionSet<'static, std::option::Option<std::string::String>, _>`

Which is not great. It should be possible to detect some mismatches like this and error with an appropriate span & message.

Should return something useful.

I could have sworn I had added support for InputObjects, and certainly some of the code appears to be there.

However:

1. They're completely undocumented
2. I looks a lot like you'd have to write the impl of InputObject yourself
3. Querygen doesn't appear to support them.

It would be good to fix all of the above.

Should also try to get IntoArgument support for input objects added while doing this.

At some point I moved from serde::Serialize to a custom SerializableArgument trait.

This implicitly requires encoding to a serde_json::Value and then serializing that into the resulting output. Think I could just use Serialize directly and get more efficient serialization and support for more than just JSON.

When you provide arguments to a querygen type, it generates an argument struct and provides cynic_argument attributes where appropriate. However, it's not adding the argument_struct attribute to the appropriate places in the generated code.

In the quickstart, we recommend using insta to snapshot test the GraphQL output by cynic.

It might be nice to have an easier way to do this - perhaps cynic could expose a function or a macro that would build these when given a query. I'm not exactly sure what this would look like however, investigating and defining that would need to be part of the work on this issue.

I intended cynic-querygen to be a lightweight code generator, so it could be compiled to relatively small WASM and loaded into the browser. As such I avoided pulling in cynic-codegen, to avoid the (presumably large) dependency on quote, syn, proc_macro2 etc. However, this has led to me implementing a selection of things twice: once in cynic-codegen & once in cynic-querygen.

Would be nice to look into resolving this. I see two options:

1. Give up and just use cynic-codegen in cynic-querygen. The querygen bundles are already huge, maybe it's not a real problem. Particularly since I'm thinking about giving up on the fully web approach.
2. Possible that some of the query parsing & introspection tools could be split out of cynic-codegen and shared between the two.

Since we ditched the serde::Serialize requirements, we no longer properly support input objects. Need to find a way to support these (a custom derive?) and then update cynic-querygen to support generating input structs.

A WIP can be found on the input-objects branch

If using a query_module, errors frequently get marked as coming from the query_module annotation rather than where they're actually happening.

Seeing this happen with darling parse errors at least, possibly others.

For some use cases, it might make sense to query for a schema at build time from a running server. Should think about supporting those use cases.

I've taken a lot of shortcuts to get stuff done - clones all over the place, taking references without thinking about lifetimes etc.

Once I have something that works would be good to revisit.

Right now it's assumed that we'll own Arguments: the Argument struct itself stores things in a Box<impl SerializableArgument + 'static> and most of the query_dsl stuff takes things by value.

Might be worth thinking about whether to open these up to using references. Easy enough to stick lifetimes on the Argument struct but need to think about what that would mean for the query_dsl - don't want to force people to use references, as that would limit the ability to generate arguments as part of a QueryFragment heirarchy (for example when dynamically doing an all_of)...

Cynic querygen is a useful (potentially essential) tool for working with cynic, but right now it's not deployed anywhere. Should get on that ASAP

I'm not entirely happy with the current core of the library. It works, but i'm not convinced it's the best syntax possible.

In particular:

• The mapN functions in Rust don't seem particularly ergonomic. They're not very self explanitory, and generally functions with lots of arguments aren't great.
• #40 & #39 suggest that the argument struct situation is also not great.

Some possible APIs

// With builder-style structs for args & a select function.
let query = query_dsl::Query::test_struct()
    .an_arg(1)
    .select(selection_set::map2(
        TestStruct::new,
        query_dsl::TestStruct::field_one(),
        query_dsl::TestStruct::nested().select(selection_set::map(
            Nested::new,
            query_dsl::Nested::a_string(),
        )),
    ));

// Builder style & field selection macro.
let query = query_dsl::Query::test_struct()
    .an_arg(1)
    .select(selection_set::fields!(
        query_dsl::TestStruct::field_one(),
        query_dsl::TestStruct::nested().select(
            selection_set::fields!(query_dsl::Nested::a_string()).construct(Nested::new)
        )
    ))
    .construct(TestStruct::new);

// Builder style w/ field selection macro but prefix constructors.
let query = query_dsl::Query::test_struct().an_arg(1).build(
    TestStruct::new,
    selection_set::fields!(
        query_dsl::TestStruct::field_one(x, y),
        query_dsl::TestStruct::nested().build(
            Nested::new,
            selection_set::fields!(query_dsl::Nested::a_string())
        )
    ),
);

#15 added the query_module attribute, but our documentation still needs updated to recommend it.

GraphQL allows the lookup of any field to fail, regardless of whether that field is nullable. If the field is nullable then it will be set to null. If not, then the null will bubble up to the first nullable parent. Any associated errors will be stored in the root list of errors, ideally with a path that specifies where the error occurred.

Since the optional invariants in the schema hold true, then cynic should be able to parse the data. However, it might be nice to support Result field types that can somehow extract the error from the root errors and move it onto the actual field that errors.

It's functional right now, but would be good if we could write our queries in it, then click a button to write a query.

Reverse engineering functionality would also be great - like you give it rust structs and it spits out a query. Probably quite advanced though...

Selection functions for fields with enum arguments have a generic parameter for the enum argument type. However, when you're not providing the enum argument, there's no way for Rust to infer the type of that generic parameter and you get errors. Since this is happening inside a query fragment derive there's not even a way to provide it.

I did experiment with adding an UnknownEnumType type and putting a default generic parameter on the argument struct, but it wasn't enough.

Think we might need to switch to a builder pattern for arguments (either on the argument structs themselves, or potentially for the fields themselves.

Used querygen on a schema there, and noticed some inconsistencies with casing in the generated structs - they were still using the GQL casings of the type names, which I'm not sure would even work with the query_dsl.

When generating code that uses argument structs, we use a #[derive(Default)] on the optional argumen structs to generate an empty argument struct. However, due to rust-lang/rust#26925 #[derive(Default)] needs all the inner types on Options to also provide default. So we need to stop doing this. If we switch to the builder pattern to fix #36 then that would also fix this.

#28 added support for arguments to querygen.

However the cynic_argument generation doesn't do the right thing in a few cases:

• Where the argument is optional, it doesn't wrap it in a Some(). This could possibly be fixed by fixing #10
• We don't generate input structs that are required.
• We don't support object literals, and don't really error - just print out TODO.

See if I can use this trick to make cynic_arguments with Options simpler: https://deterministic.space/elegant-apis-in-rust.html#intooption_

There's a lot of places where we return Results in cynic and then convert to TokenStream a level up. This is fine, but may limit our ability to error to a single error at a time. Might be more appropriate to convert to TokenStreams at every different point that can error so we can have > 1 error emitted from each macro invocation.

Querygen doesn't currently generate argument structs if you pass it a query with arguments.

Would be good to support this - presumably this will also require us to be able to generate structs for input objects.

field_arguments or graphql_arguments or similar might be clearer - not sure people need to care that it's cynic at that level.

The issue above was that I had used an i64 field where cynic wanted an i32. Not great positioning - wonder whether it could be moved to the field responsible.

To quote the spec:

The Int scalar type represents a signed 32‐bit numeric non‐fractional value. Response formats that support a 32‐bit integer or a number type should use that type to represent this scalar.

Should update our code to do this rather than using i64s.

When querygen encounters a query that needs no additional types, it still outputs an empty types module. See the snapshot added in #70 for an example. Not too important, but seems sub-optimal.

When writing structs for QueryFragments & InputTypes and enums for Enums it's very easy to get the name of a field or variant wrong. Cynic will error you when you do this, but it will just say "field not found".

It would be nice if it could make suggestions if you just made a spelling mistake or typo. Not sure what algorithm is best to do this with, but it looks like there's a few different similarity algorithms built into this library which could be used.

I use the terms serialize, encode (and their deser counterparts) interchangably in a lot of the API. Think about tidying that up.

I'm being fairly lax with testing a lot of cynic - might be good to get some snapshot tests in place for query_dsl output, other derive output, the queries we generate etc.

We use serde::Serialize for sending types to a server. However, this is a pain to get to work right now, as cynic does transforms on data that serde doesn't know about.

Though the underlying implementation could possibly still use serde we should either make it easy, or come up with another way of serializing data. For enums this would be trivial, for input objects maybe not so much.

#44 updated the query_dsl to use the builder pattern for specifying arguments. Might be nice to look at unwrapping the Option from optional arguments so that we don't have to do an_option(Some(xyz)) everywhere.

Though we probably do want to support people providing None so maybe need to use Into<Option<T>>?

When using query_dsl on a schema with a large number of scalars, it's a pain to have to define types for all of them. The error messages when you do so are also pretty terrible.

Wonder if it'd be possible to use some default type for scalars in the query_dsl. Perhaps either serde_json::Value, a string or some custom type. Then in the QueryFragment derive we can do some sort of transformation to custom/built in types.

This would provide the same flexibility but without having to explicitly define types for a bunch of scalars that you might not even use.

Decoding a query result is currently lightly clunky because it's a two step process... wonder if there's a way to add a Result extension or similar that'd make it a little more elegant.

#12 introduced querygen, but skipped over a few features:

• Query fragments
• Union types
• Interfaces
• Deduplication of types (if a type appears at multiple places it'll be emitted multiple times)
• Handling multiple queries
• There's a number of unwraps that should return actual errors
• Possibly other bits (do some exploration and see what else is missing)