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 a note about omitting introspection type definitions #1036

Closed
wants to merge 3 commits into from

Conversation

martinbonnin
Copy link
Contributor

There is a similar note

for scalars:

When representing a GraphQL schema using the type system definition language, all built-in scalars 
must be omitted for brevity.

for directives:

When representing a GraphQL schema using the type system definition language any built-in directive 
may be omitted for brevity.

I didn't find anything for introspection types.

@netlify
Copy link

netlify bot commented Aug 2, 2023

Deploy Preview for graphql-spec-draft ready!

Name Link
🔨 Latest commit 350a140
🔍 Latest deploy log https://app.netlify.com/sites/graphql-spec-draft/deploys/64cb67c66dd0bd0008766a6f
😎 Deploy Preview https://deploy-preview-1036--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.

benjie
benjie previously requested changes Aug 3, 2023
Copy link
Member

@benjie benjie left a comment

Choose a reason for hiding this comment

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

Typo?

spec/Section 4 -- Introspection.md Outdated Show resolved Hide resolved
@IvanGoncharov
Copy link
Member

Based on Aug 3th WG discussion: I think we can untangle this change from scalars and direcives by saying that you have two options:

  1. Status quo: Query root doesn't have __schema and __type so you shouldn't include Introspection types.
  2. Query root has __schema and __type then you should include all the introspection type reachable from those two fields.

We can't tangle it with scalars because currently below is a valid behaviours for scalars:
You print scalars used only by introspection or standard directives even if you omitted those directives. Moreover graphql-js does that and we can't make such documents invalid or broken.

@IvanGoncharov
Copy link
Member

However, it creates a slightly different problem since introspection for the Query root type doesn't contain __type and __schema.
So technically you wouldn't be able to convert introspection to SDL with introspection types.

@martinbonnin
Copy link
Contributor Author

martinbonnin commented Aug 3, 2023

Thanks for circling back on this!

Query root doesn't have __schema and __type so you shouldn't include Introspection types.

That sounds like a short term thing that doesn't have too many implications, I like this :) 👍.

Edit, I'm re-reading myself and I think I got it wrong. I actually want introspection types even if __schema and __type are not present

You print scalars used only by introspection or standard directives even if you omitted those directives.

I'm confused. The spec says:

When representing a GraphQL schema using the type system definition language, all built-in scalars 
must be omitted for brevity.

It's a "must" so my understanding is (in the current form at least), scalar type definitions must never be present in a SDL document?

graphql-js has the printIntrospectionSchema() that (I think) outputs them but it's an exception to the spec?

@benjie
Copy link
Member

benjie commented Aug 21, 2023

Feels like we could do this by adding an includeMeta/similar option. We could either go with boolean like includeDeprecated, or we could be more granular to differentiate between whether __typename should or should not be included:

+ enum __IncludeMeta { NONE ALL ALL_EXCEPT_TYPENAME }
  type __Type {
    # ...
    fields(
      includeDeprecated: Boolean = false
+     includeMeta: __IncludeMeta = NONE
    ): [__Field!]

@martinbonnin
Copy link
Contributor Author

Closing in favor of #1049

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants