Deprecate includeDeprecated, add __schema(includeDeprecated:)#1230
Open
benjie wants to merge 2 commits into
Open
Deprecate includeDeprecated, add __schema(includeDeprecated:)#1230benjie wants to merge 2 commits into
__schema(includeDeprecated:)#1230benjie wants to merge 2 commits into
Conversation
✅ Deploy Preview for graphql-spec-draft ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
|
Could we go further and not add the |
Member
Author
|
That would change the meaning of any existing queries that use introspection and don't specify |
Contributor
|
Ahh yeah of course. What I really want is for |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Introspection should describe a consistent schema. The rules of the GraphQL schema validation aim that there are at least two consistent schemas (PR to follow to make this more explicit...): the source schema, and a derived schema that has all deprecated elements removed. Currently, however, an inconsistent schema can be described by mixing different values into
includeDeprecatedarguments throughout introspection. Worse, becauseincludeDeprecateddefaults tofalse, every time a new feature of the schema can be deprecated, existing schema introspection queries can start to represent an invalid schema as newly deprecated entities may be excluded even thoughincludeDeprecated: trueis specified for all previous introspection fields.This PR aims to eliminate this inconsistency by deprecating the old
includeDeprecated:system and replacing it with a__schema-levelincludeDeprecatedargument that applies recursively. This however requires some further changes:includeDeprecatedarguments would conflict with the__schema-levelincludeDeprecated, so they are forbidden when it's specified.includeDeprecatedisfalseby default, specifying__schema(includeDeprecated: true)would still describe the un-deprecated portions of the schema; thus the existingincludeDeprecatedarguments must lose their default value and become optional (which currently means nullable).Were it the case that
includeDeprecatedhad had the default valuetruethroughout history, this change could have been accomplished by simply adding__schema(includeDeprecated: Boolean! = true), since{__schema(includeDeprecated: false) {...}}would represent a schema with all deprecated elements removed independent of any nestedincludeDeprecatedarguments. Alas, we defaulted tofalseand thus to allowincludeDeprecated: trueat the root level we need to put in more effort.Possible expansion: we could make the validation rule stricter: forbid literal null, and require that a variable in that position is defined as non-nullable. I didn't see sufficient value in this to justify adding it, but I'm happy to do so if others think it's worthwhile.