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.
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
Spec edits for incremental delivery, Section 3 & 7 only #1124
base: incremental-integration
Are you sure you want to change the base?
Spec edits for incremental delivery, Section 3 & 7 only #1124
Changes from 3 commits
abafb76
0310656
fcf898f
ffb8cad
c7c3ee1
2934a59
5d72416
03adfb9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Can we clarify what is meant by implementation here? My current understanding is that an "implementation" is typically a library, such as
graphql-js
orgraphql-java
while a "service" is typically a server or a collection of servers, such as the GitHub or Shopify APIs.If that's the case, the "not required" part could be confusing. If I'm writing the
graphql-cobol
library, can I decide to leave@defer
out and still claim compatibility with the spec?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.
@martinbonnin thanks for reviewing. I updated this to say
services
instead ofimplementations
as I was referring to servers hereThere 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.
Thanks for the clarification 🙏
Would it make sense to use language similar to
@specifiedBy
and@oneOf
:This is the closest to feature discovery we have in GraphQL and using common language pattern would make the spec more palatable. Or is there something fundamentally different about
@defer
and@stream
?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'm not sure this applies to
@defer
and@stream
, because they are directives that apply to "executable locations", whereas@specifiedBy
and@oneOf
only apply to "type system location".I think a GraphQL server could not support the "type system definition language" but still support
@defer
and@stream
. Clients could determine if@defer
and@stream
are supported by using the introspection query:I think the inverse could also be true. A GraphQL Server that supports the type system definition language may have no desire to support
@defer
/@stream
and that's ok since we want this feature to be strictly optional.It's possible I'm not thinking about this correctly, let me know what you think.
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.
Apologies in advance, a bunch of nitpicking on the vocabulary/glossary is coming below.
I don't think it makes sense for a GraphQL server to support the "type system definition language". A server reads request in and writes responses out. SDL is an implementation detail for the server.
But maybe not for its implementation (==lib?). A lib takes SDL in and generates a compliant service out of it. I always thought of
implementation
in the spec as being the "lib" but re-reading it, I'm not sure any more.Regardless, if we use this service vs implementation distinction, I would rephrase like so:
I think this is the same for
@oneOf
and@specifiedBy
:That's why I'm tempted to put them in the same bucket of features even if
@defer
is obviously quite important for clients to be aware of.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.
@martinbonnin I am definitely open to changing this, just want to talk it through to make sure I fully understand.
Is this statement true for
@oneOf
and@specifiedBy
?Since
@oneOf
and@specifiedBy
are directives that can only be applied to grammar that is part of the "type system definition language", how can an implementation support this directive without supporting the grammar that is required to place the directive? I agree thatspecifiedByURL
introspection field may be supported but this is not the same as the directive.The same is not true for
@defer
and@stream
because these directives are applied to the execution grammar.Again, I could be wrong about this but my understand for
@specifiedBy
is that ideally all GraphQL servers would support it, but that's not possible for servers written before they were added to the spec, so RFC 2119should
is used. Additionally, we further restrict it to "implementations that support the type system definition language" since@specifiedBy
can only be placed in the type system grammar.For
@defer
and@stream
we don't want to use RFC 2119should
/recommended
language since it is optional and we expect some servers will choose not to implement it for reasons other than being behind on the latest spec changes.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.
Gotcha, thanks! So we've got:
must
for stuff that all services must support (@include
for an example)should
to allow older services to still be compatible but newer services should support (@specifiedBy
for an example)@defer
for an example)Did I get that right?
If yes, I was missing the 2. vs 3. distinction, thanks for pointing that out 🙏
I'm still not 100% sold on it though. I kind of like that other directives all use the same sentence and feels a bit weird that
@defer
is not following the pattern (or maybe it's just my OCD...).What about using RFC 2119
may
to denote that it's "more" optional than@specifiedBy
? Was that ever mentioned?I don't want to bikeshed this too much especially if it was discussed already. Is there any reading material and/or pointers I could catch up on?
It can't but it's fine?
My reading of
A GraphQL implementation that does not support the "type system definition language"
is a code-first GraphQL implementation likegraphql-kotlin
for an example. It has nothing to do with supporting the grammar or anything like this. A code-first implementation can implement@specifiedBy
.Supporting the "type system definition language" is a feature of some GraphQL libs that has nothing to do with how the grammar is internally implemented.
I read all of these sentences as: "if a lib reads a SDL written by a user, then the lib must/should/may provide the built in directive definitions":
The way I see it the directive location (executable or type system) is irrelevant. These sentences are about the directive definitions that must/should/may be added to the resulting schema.
But again this is my very personal understanding of it and maybe PTSD from the full schemas discussion. Thanks for diving into this with me!
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.
@martinbonnin I agree that we should probably rewrite this to use RFC 2119
may
oroptional
to clearly state that it's an optional feature instead of recommended.I still don't think we should include
that support the type system definition language
for anything related to@defer
or@stream
because they are features that are unrelated to the SDL.I think a GraphQL server that creates everything programmatically fits the definition of an implementation that does not support the type system definition. My interpretation is that the spec is not providing any sort of guidance for the
@specifiedBy
directive for this type of implementation since it is excluded by the phraseGraphQL implementations that support the type system definition language ...
.But for
@defer
and@stream
it is important that this type of implementation follow this guidance regardless of SDL support so we should not include the same phrase.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 see it now! Thanks for explaining 🙏
Agreed here as well 👍 But I also think this may be a problem?
Do we have somewhere in the spec where we say what built-in directives are required in a valid full/final schema and which ones are not? I initially thought this was the place but since it excludes code-first implementations it can't reasonnably be it?
Right 👍 I actually like the sentence as it is now because it applies to services. I think using
service
here is correct since SDL support is irrelevant for a service.Finally, I'd say there should be a similar sentence for other directives?
@include
/@skip
come to mind for an example because they are used in executable locations. Can a service ignore them? But I would probably do it for all directives for symmetry and consistency reasons.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.
For
@include
/@skip
, the spec does have the following sentence at the beginning of this sectionI do think this is the correct place, and with the above sentence for
@skip
and@include
we are covered for all SDL and non-SDL directives. But even@skip
and@include
are not required since the spec is using RFC 2119should
.Perhaps! Maybe it's worth a WG discussion to clarify?
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.
Interesting, I didn't realise we had applied this to initialCount - I thought it was either non-deferred (i.e. give you the whole list) or deferred with initialCount supplied.
I think we should revisit this discussion, it's quite different to the general "don't defer" and "don't stream" optimizations in my mind - specifically if you specify
initialCount: 2
I'd argue that at least 2 results should be supplied, or the entire thing should not be deferred (e.g. if there are fewer than 2 results). Skipping@defer
/@include
result in the client getting more data up front, but ignoringinitialCount
allows for the client to get less data up front, and that's a problem to my mind.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.
Happy to revisit this discussion. If I am remembering correctly, the arguments for this version were: