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.

Sign up for GitHub

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

Jump to bottom

Default to generic ScalarValue in #[graphql_object] macro #779

Merged
merged 33 commits into from
Nov 7, 2020
Merged

Default to generic ScalarValue in #[graphql_object] macro #779

merged 33 commits into from
Nov 7, 2020

Conversation

tyranron
Copy link
Member

@tyranron tyranron commented Oct 8, 2020 •
edited
Loading

Fixes #769
Fixes #580

This PR makes #[graphql_object] macro to generate code which is generic over S: ScalarValue by default. Previously, DefaultScalarValue has been used.

Checklist

  • Tests updated
    • integration tests
    • codegen failure tests
    • juniper crate tests
    • juniper_codegen crate tests
    • third-party integrations tests
    • Book tests
  • Documentation in Book is updated
  • CHANGELOG entry is added

@tyranron tyranron added enhancement Improvement of existing features or bugfix semver::breaking Breaking change in terms of SemVer k::api Related to API (application interface) labels Oct 8, 2020
@tyranron tyranron self-assigned this Oct 8, 2020
@tyranron tyranron marked this pull request as draft October 8, 2020 13:20
@tyranron tyranron changed the title Default to generic ScalarValue in #[graphql_object] macro Draft: Default to generic ScalarValue in #[graphql_object] macro Oct 8, 2020
@tyranron tyranron force-pushed the fix-generic-scalar-in-objects branch from f10bea0 to 86d768b Compare October 19, 2020 11:03
@tyranron tyranron changed the title Draft: Default to generic ScalarValue in #[graphql_object] macro Default to generic ScalarValue in #[graphql_object] macro Oct 20, 2020
@tyranron tyranron requested review from LegNeato and ccbrown October 20, 2020 16:18
@tyranron
Copy link
Member Author

@LegNeato @ccbrown I've made it, but I'm unsure because of consequences it does. Please, review how examples were transformed. The main pain point goes from fallible fields, because FieldError is parametrized with S: ScalarValue. But if user uses custom type implementing IntoFieldError<S> then everythig works like a charm.

LegNeato
LegNeato reviewed Oct 21, 2020
View reviewed changes
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.

This is great work. I'm worried...it seems like this change bleeds into the developer experience heavily. Before, you basically had one place where you had to deal with scalar values (mixing the derive and the proc macro). Now, it shows up in a bunch of function signatures and turbofish calls by default.

@ccbrown
Copy link
Contributor

ccbrown commented Oct 21, 2020 •
edited
Loading

With some trickery to avoid conflicting with the existing reflexive implementation, it may be possible to implement IntoFieldError<S> for FieldError<DefaultScalarValue>. ScalarValue requires From implementations for all of the DefaultScalarValue variants, so a DefaultScalarValue should be convertible to any other scalar value type. If that can be done, users could just return FieldError in typical cases. (This might require specialization or a separate type though.)

One of the weirdest things about this PR to me is that there's an increasing number of places where a type named DefaultScalarValue is no longer the default. That line in the quickstart that says "specify DefaultScalarValue if you don't want the default behavior" might be a bit of a head scratcher. Makes me wonder if a name like "StandardScalarValue" would be more appropriate.

@tyranron
Copy link
Member Author

tyranron commented Oct 21, 2020 •
edited
Loading

@ccbrown @LegNeato yeah... you've confirmed my worries 🙃

it may be possible to implement IntoFieldError<S> for FieldError<DefaultScalarValue>. ScalarValue requires From implementations for all of the DefaultScalarValue variants, so a DefaultScalarValue should be convertible to any other scalar value type.

What a nice idea! Thank you! I'll try to poke with it more.

@tyranron
Copy link
Member Author

tyranron commented Oct 22, 2020 •
edited
Loading

@ccbrown yeah... it's very quickly hits into the specialization problem. But I've succeed to imitate the specialization in this situation with a TypeId + transmute hack, which with a little help of optimizer should have no noticable performance impact (TypeId is a const info, so after monomorphization an optimizer should just cut off always irrelevant if branches).

Now I should re-refactor the changes in tests, examples, etc.

@tyranron
Copy link
Member Author

tyranron commented Oct 29, 2020 •
edited
Loading

@ccbrown @LegNeato please look, now it's much more convenient. Fallible fields and subscription coerce nicely.

The only place left, where I cannot hide the type parameter is the GraphQL interface, when the trait object is used (enums are OK). Though it's very rare case, and I may poke with it later.

@tyranron tyranron requested a review from LegNeato October 29, 2020 13:36
@tyranron tyranron marked this pull request as ready for review October 29, 2020 13:38
ccbrown
ccbrown approved these changes Nov 7, 2020
View reviewed changes
Copy link
Contributor

@ccbrown ccbrown left a comment

Choose a reason for hiding this comment

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

Yeah this is definitely looking a lot nicer. 👍

LegNeato
LegNeato approved these changes Nov 7, 2020
View reviewed changes
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.

Looks great!

@LegNeato LegNeato merged commit a487188 into master Nov 7, 2020
@LegNeato LegNeato deleted the fix-generic-scalar-in-objects branch November 7, 2020 02:15
davidpdrsn added a commit to davidpdrsn/juniper-from-schema that referenced this pull request Nov 7, 2020
@davidpdrsn
Support juniper no longer defaulting to DefaultScalarValue for `#[g…
4fee85c 
…raphql_object]`

graphql-rust/juniper#779
davidpdrsn added a commit to davidpdrsn/juniper-from-schema that referenced this pull request Nov 7, 2020
@davidpdrsn
Support generating code from build.rs
0b30c69 
Prepare for making code generation usable from a build script

Move code generation into its own module

Re-enable the lints

Move lots of stuff into code_gen module

Flatten some modules

Rename juniper-from-schema-code-gen to juniper-from-schema-proc-macro

Move code generation stuff into juniper-from-schema-code-gen

Clean up dependencies

Add `juniper-from-schema-build` to compiling schema in "build.rs"

Update build script

Support juniper no longer defaulting to `DefaultScalarValue` for `#[graphql_object]`

graphql-rust/juniper#779

Update changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ccbrown ccbrown ccbrown approved these changes

@LegNeato LegNeato LegNeato approved these changes

Assignees

@tyranron tyranron

Labels
enhancement Improvement of existing features or bugfix k::api Related to API (application interface) semver::breaking Breaking change in terms of SemVer
Projects
None yet
Milestone
No milestone
3 participants
@tyranron @ccbrown @LegNeato