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

add __directive meta field parallel to __type #1114

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

yaacovCR
Copy link
Contributor

Use case: inquiring about specific directives such as @defer and @stream

@yaacovCR yaacovCR changed the title add __directive meta field parallel to type add __directive meta field parallel to __type Sep 25, 2024
Copy link

netlify bot commented Sep 25, 2024

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 2196942
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/66f401a944072500083a7389
😎 Deploy Preview https://deploy-preview-1114--graphql-spec-draft.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Thanks @benjie

Co-authored-by: Benjie <[email protected]>
as we haven't yet introduced the schema so nullable types might be confusing
@yaacovCR
Copy link
Contributor Author

Implemented by graphql/graphql-js#4203

@benjie
Copy link
Member

benjie commented Nov 7, 2024

I've not reviewed if these spec changes are sufficient, but I'm supportive of the aim 👍

@benjie benjie added the 💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md) label Nov 7, 2024
@yaacovCR
Copy link
Contributor Author

yaacovCR commented Nov 8, 2024

This was discussed at the November 2024 WG => this small change prompted an interesting discussion about "The Future of Introspection."

To my understanding, @leebyron 's feedback:

Perhaps __type should have always been nested as a type argument under __schema, so that similarly __directive should be nested as directive under __schema.

This general change might unlock general __schema arguments like:

  __schema(includeDeprecated: true) { ... }

that would apply to all nested fields.

As this change is not a "must-have" it will probably be paused to make sure it fits into whatever introspection future is planned. We do not have to actually implement that entire new future to move forward with this small feature change, but we have to make sure that this smaller change aligns with the plan.

@benjie
Copy link
Member

benjie commented Nov 21, 2024

I'm generally in favour of Lee's proposal; but I do have one concern that I wanted to think about: unless we're careful the way in which we execute this may set a bad precedent of passing down implicit values.

I've written about the problem of referencing ancestors before, but essentially if you can think of anything under __Schema as being an identifiable node in a graph then implicitly passing down settings like isDeprecated through the tree would break normalized caching. Take the following query for example:

{
  a: __schema(includeDeprecated: true) {
    type(name: "MyType") { name fields { name } }
  }
  b: __schema(includeDeprecated: false) {
    type(name: "MyType") { name fields { name } }
  }
}

here the objects a.type and b.type represent the same type (MyType):

flowchart TD
  Query --> a["__schema(includeDeprecated: true)"]
  Query --> b["__schema(includeDeprecated: false)"]
  a --> MyType
  b --> MyType
Loading

So when we access fields (with no arguments) we'd expect the same results:

flowchart TD
  Query --> a["__schema(includeDeprecated: true)"]
  Query --> b["__schema(includeDeprecated: false)"]
  a --> MyType
  b --> MyType
  MyType --> fields
  classDef hide fill:#fff,stroke:#aaa,color:#aaa;
  class Query,a,b,MyType hide;
Loading

Contradiction: but a.type.fields and b.type.fields are desired to be different if some fields are deprecated!


Despite raising this, I'd argue that this is in fact not an issue for the proposed changes, since none of the entries below __schema are identifiable nodes in the graph - requesting __schema(includeDeprecated: false) would actually yield a different schema with deprecated entities omitted (let's call it __Schema*), so the types returned from a.type and b.type are concretely different types - let's call them MyType and MyType*:

flowchart TD
  Query --> a["__Schema"]
  Query --> b["__Schema*"]
  a --> MyTypeA["MyType"]
  b --> MyTypeB["MyType*"]
  MyTypeA --> fieldsA["fields"]
  MyTypeB --> fieldsB["fields*"]
Loading

Importantly __Type does not have a reliably identifier. You might think __Type.name is an identifier, but it does not identify wrapper types (list/non-null have null name) so it isn't. Similarly no other nodes under __Schema have reliable globally unique identifiers (__Field has name but it is not unique - multiple types may define a field with the same name; etc). Therefore normalized caches are safe.

The caveat here is that we must be willing to commit to this always being true before we adopt this change.

@martinbonnin
Copy link
Contributor

Rereading the wg notes:

Right now it’s possible to have an inconsistent schema by passing different includeDeprecated values

Trying to list the cases where this may happen, the only place I found is if an input value has a default value that is a deprecated enum value that was not fetched. Are there others?

This has the potential to become much more of an issue with deprecated object types but as it is now it's okay-ish?

All in all, I agree with @benjie's point that this is setting a precedent about accessing ancestors that might not be worth the tradeoff.

Personnal anecdote: I have been tempted to do something similar to #144 in the Confetti API. The idea was to have a top-level conference(conferenceId) field that would filter out the content per-conference and that would effectively break normalized caching as Benjie described in his ancestors article. In retrospect, I'm glad I didn't do it. But were this pattern used in more places, I might have bought into it without really realizing all the consequences...

Another question (that deviates quite a bunch from the original issue): do we really need includeDeprecated? Is saving a few bytes really worth the complexity here? Put it otherwise: could we deprecated includeDeprecated 🤪 ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💭 Strawman (RFC 0) RFC Stage 0 (See CONTRIBUTING.md)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants