Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow to specify scalar and context (GraphQLEnum) #621

Merged
merged 15 commits into from Apr 18, 2020

Conversation

jmpunkt
Copy link
Contributor

@jmpunkt jmpunkt commented Apr 14, 2020

Motivation

Currently the derive enum macro does not support to set a custom context nor does it allow to set a specific scalar. In general this is not required since an enum does not benefit from such features. However, the impl macros of Juniper set the context and scalar to a fixed type. Therefore, it is not always possible to use enums with objects that are generated by impl macros. A workaround is to specify a context and scalar via the grapql(...) annotation. Since derive enum does not support this, it is not possible to use them with objects that are created by the impl macro. This PR enables these features.

Side Note

With the feature in #618 the following example does not work properly. Consider the small example, which does not compile on #618.

#[derive(juniper::GraphQLEnum)]
pub enum TestEnum {
    Ok,
    NotOk,
}

#[derive(juniper::GraphQLObject)]
#[graphql(Context = CustomContext)]
pub struct HumanContext {
    id: String,
    home_planet: String,
}

#[derive(juniper::GraphQLUnion)]
#[graphql(Context = CustomContext)]
pub enum CharacterContext {
    One(HumanContext),
    Three(TestEnum),
}
// Context Test
pub struct CustomContext {
    is_left: bool,
}

impl juniper::Context for CustomContext {}
type mismatch resolving `<TestEnum as juniper::types::base::GraphQLType<__S>>::Context == CustomContext`

Implementation

The implementation utilizes already existing functionality provided by the object implementation. In the future shared code can be refactored. Features which are supported by objects, but are not by enums, are disabled with panic messages.

Result

  • graphql(Scalar = T) support
  • graphql(Context = T) support
  • Async can be turned off
  • Shares code with existing implementation -> allows easy refactor

Copy link
Member

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much! Can we get a test with the added functionality (a custom scalar and context) to make sure it doesn't regress? Adding a note to the book would be great too.

@jmpunkt
Copy link
Contributor Author

jmpunkt commented Apr 16, 2020

Documentation

I added an support matrix for GraphQLEnum. Maybe this matrix can be used for all book pages to provide a quick overview.

Idea

Each possible attribute has a overview on different book page. These attributes most of the time behave the same. Differences I encountered where different conversions of the name (enums use a different string case than objects).

Implementation

The original motivation for this PR has slightly changed. The reason why the error appeared was because of the my previous PR which introduced unions. The code was not able converting the context per variant. Therefore, all variants had to had the same context. Scalars are not affected by this. I previously thought this is common and GraphQLObject had the same behavior. But GraphQLObject allows each field having its own context, if the field's context is compatible with the container context (IntoResolvable?). Now GraphQLUnion has the same behavior and converts the context for each variant. Therefore, it is not mandatory having a custom scalar nor context for enums. However, the PR should be an improvement anyway.

Testing

Testing the custom context is possible, but testing the scalar is not. Therefore, I would recommend removing the support for custom scalars on GraphQLEnum and use generics like before. The generic parameter for a scalar should always be able to adapt a specific scalar?!

@jmpunkt jmpunkt requested a review from LegNeato April 16, 2020 16:59
Copy link
Member

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see there is a lot of overlap withhttps://github.com/graphql-rust/juniper/blob/a05f4e55c41d4a5a912334a105a73bd33f0ff904/juniper_codegen/src/util/mod.rs#L659. While refactoring can wait, we should make sure that anything in there is leveraged here.

juniper_codegen/src/util/mod.rs Outdated Show resolved Hide resolved
@LegNeato
Copy link
Member

I'm also not really familiar with the GraphQLEnum implementation or enums in graphql in general so I'll trust your judgment for what to do about scalars.

@jmpunkt
Copy link
Contributor Author

jmpunkt commented Apr 17, 2020

Thanks for your review. Your feedback was applied.

Another side note. The current implementation adds additional constrains like unique naming. Before it was possible to have something like the following:

#[derive(juniper::GraphQLEnum)]
enum MyEnum {
    A,
    #[graphql(name = "A")]
    B,
}

Now this is not possible anymore. Notice that this is required by the GraphQL spec. The other added constraints were not possible before, but now provide better feedback for the user.

This should be ready to merge, if you do not have anything to add.

@jmpunkt jmpunkt requested a review from LegNeato April 17, 2020 18:56
Copy link
Member

@LegNeato LegNeato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thank you again for writing such a high-quality PR and being very detail oriented. I appreciate it!

@LegNeato LegNeato merged commit fe99e1c into graphql-rust:master Apr 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants