-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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
feat(schema): Inference controls and improvements #13028
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe I grasped around 70% of this PR. Mostly it was hard to follow which composer we were working on definedComposer
, inferedComposer
, typeComposer
, schemaComposer
, ...
It mighte be handy to put jsdoc above all function or most functions to explain what this function does.
@@ -423,12 +398,12 @@ describe(`dateResolver`, () => { | |||
expect(fields.invalidDate5.resolve).toBeUndefined() | |||
expect(fields.invalidDate6.resolve).toBeUndefined() | |||
expect(fields.invalidDate7.resolve).toBeUndefined() | |||
expect(fields.invalidDate8.resolve).toBeUndefined() | |||
expect(fields.invalidDate8).toBeUndefined() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this invalidDate8 undefined? (could you point me to the code where we don't handle nullable values? might be https://github.com/gatsbyjs/gatsby/pull/13028/files#diff-bdb45e489c83b58c411947869567afa8R291).
Not 100% where I can see where this behaviour changed. Any pointers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test was funky cause it didn't really test anything - it just assigned everything to Date. This actually tests that invalid dates don't get types. In addition, fields that are all null/undefined don't get graphql fields at all.
Why aren't we deprecating things now and then removing them in v3? |
@KyleAMathews that's what we are doing, i phrased it awkwardly. Will fix. |
I have tried to simplify some things a bit in #13557, feel free to cherry-pick.
|
@stefanprobst I think some of the changes to third party types could have been caused by some regression in graphql-compose. If it works without them (as in passes tests), then it's fine. |
* Add another test * Add thirdparty types and fix relay classic weirdness * Remove unused deps * Simplify inference
docs/blog/2019-04-23-improvements-to-schema-customization/index.md
Outdated
Show resolved
Hide resolved
* Allow registering field extensions * Update packages/gatsby/src/utils/api-node-docs.js Co-Authored-By: stefanprobst <stefan.probst@univie.ac.at> * Update packages/gatsby/src/schema/schema.js Co-Authored-By: stefanprobst <stefan.probst@univie.ac.at> * Rename to createFieldExtension * Apply review suggestion * Remove addResolver extension * Add test * Remove public createFieldExtension API * Lint
@stefanprobst Could you do last review? @DSchau Could you do blog post review? |
Published |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff! I added some comments on the blog post.
docs/blog/2019-05-17-improvements-to-schema-customization/index.md
Outdated
Show resolved
Hide resolved
docs/blog/2019-05-17-improvements-to-schema-customization/index.md
Outdated
Show resolved
Hide resolved
docs/blog/2019-05-17-improvements-to-schema-customization/index.md
Outdated
Show resolved
Hide resolved
Co-Authored-By: Mike Allanson <michael.allanson@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Evolution of schema customization
tl/dr
Try it now with
@schema-customization
tag.Use resolver directives/extensions to add default arguments or/and resolvers to fields.
Use
@dontInfer
and enjoy better performance with no inferrence.As usual, report issues in #12272
Summary
After about a month of testing schema customization both here and in pre-release we determined a couple of issues. We set up to do this because we wanted to remove uncertainty in people's schemas, when the data changes. However, the original design allowed some uncertainties anyway. In addition, we have made inferrence a more heavy process, trading performance for consistency and didn't really provide a way to opt out of it completely. To summarize:
@dontInfer
flag was onTherefore we have some proposed changes to the way we do inferrence and some deprecations.
noDefaultResolvers and inferrence modes
First of all, we are deprecating
noDefaultResolvers
argument ofinfer
anddontInfer
. We feel it was confusing and very catch all and didn't even add resolvers at the end. We will supportnoDefaultResolvers
until version 3, after which@infer
behaviour will become a default andnoDefaultResolvers
will be removed.We didn't want to break things, so we keep old default behaviour, even though we think it's not optimal. Add explicit
@infer
and resolver extensions to fields to be future proof.Default (deprecated in v3)
Applies with no
@infer
or there is@infer(noDefaultResolvers: false)
on type.Type gets all inferred fields added. If type has defined fields of types
Date
,File
and any other node, and we inferred that they should have resolvers/args, resolvers/args will be added to type with a warning.Strict inference (future default in v3)
Applies with
@infer
or@infer(noDefaultResolvers: true)
.Type gets all inferred fields added. Existing fields won't automatically get resolvers
No inferrence
Applies with
@dontInfer
or@dontInfer(noDefaultResolvers: true)
.Inferrence won't run at all. Existing fields won't automatically get resolvers
No new fields with default resolvers (deprecated, removed in v3)
Applies with
@dontInfer(noDefaultResolvers: false)
Inferrence will run, but fields won't be added. If type has defined fields of types
Date
,File
and any other node, and we inferred that they should have resolvers/args, resolvers/args will be added to type with a warning.Resolver extensions
To add an explicit workraround to not getting resolvers when there is no inferred ones, one can now use explicit resolver directives.
Type Builders and extensions
You can now apply configuration to type builder types through extension property on them.