Graphql-js: Outstanding breaking changes in buildClientSchema

[trevor-scheer]

This issue is a follow-up to an ongoing problem with breaking changes in buildClientSchema.

A breakdown of events

• v<14.2.0: buildClientSchema pulls in only the built-in scalar types that are referenced within the schema.
• v14.2.0: buildClientSchema doesn't pull in _any_ built-in scalar types. (original breaking change)
• v14.2.1-14.3.0: buildClientSchema pulls in _all_ of the built-in scalar types, whether they're referenced within the schema or not. The fix addressed one issue, but introduced another and added unintended behavior.
• 14.3.1-14.4.1 (latest): buildClientSchema doesn't add any built-in scalars, meaning the schema must be provided complete. We previously depended on pre-14.2.0 behavior, this is a break that we could only work around by pulling in the missing scalars ourselves before calling buildClientSchema.

History and related links

• #1677 - (introduced breaking change)
• #1808 - attempt to correct breaking change, but introduced new behavior
• #1809 - undoes previous behavior, but doesn't pull in missing scalars as before
• Lots of discussion around the issue: https://github.com/graphql/graphql-js/commit/183ff32bee4bc23eb23657f79f414c2400ecb06a#r32971387
• https://github.com/apollographql/apollo-tooling/pull/1355

Reproduction

https://github.com/trevor-scheer/buildClientSchema-changes

Install and run

yarn
node index.js

The included schema for demonstration intentionally leaves out all built-in types in order to show how behavior changed over time. There is a field in the schema which depends on the Int type.

[IvanGoncharov]

@trevor-scheer Thanks for a detailed description and especially for the repo 👍
Now I finally understand what happened here.

The included schema for demonstration intentionally leaves out all built-in types in order to show how behavior changed over time. There is a field in the schema which depends on the Int type.

So every introspection result should include all types it references even standard scalars.
Especially since we can add new standard scalars in future (e.g. OffsetDateTime RFC) so we can't rely on both client and server have the exact same set of standard scalars.

So technically it wasn't a breaking change since your example introspection is invalid and util 14.2.0 we had a bug that was just masking this problem.

That said can you please describe how did you get this introspection?
Did you use some GraphQL library on your server that has this bug?
Or do you do remove standard scalars from introspection in your code?

[IvanGoncharov]

@trevor-scheer I just added a test to check for the current behavior, see #2006

[trevor-scheer]

Hey @IvanGoncharov, apologies it's taken so long to get back to your question.

Our own schema allows a consumer to request an introspection _with or without builtin types_, which is where I think the invalid introspections are coming from. I understand how this is a mistake on our part, and that we previously depended on a bug.

I'm unsure what to recommend going forward, but it will be some time before I'm able to revisit this issue anyhow (and it's not requiring our immediate attention). If you'd like to close this issue for now, please feel free to - I'm happy to reopen it another time if necessary. You may leave it open if you prefer to, though I don't suspect any other users depended on this bug.

[IvanGoncharov]

@trevor-scheer No problem. Your investigation was exceptionally helpful and finally clarified what was going on.
I think the current behavior is fully spec-compliant.
Since we stop active development on 14.x.x and 15.0.0 should be spec-compliant in that regard I'm closing this issue.

[sungam3r]

@IvanGoncharov @trevor-scheer Could you explain to me the current accepted behavior regarding builtin scalars? I have no programming experience in JS. This implementation is considered the main one and I would like to do the same for .NET. Now I am working on graphql-dotnet/graphql-dotnet/#1423 . Also I asked about this in graphql/graphql-spec#648 .

I will try to break my question into several statements/assertions. I assume that everything should be answered yes. Is it so?

1. graphql.js SDL does not contain unused (unreachable) custom types, including custom scalars.
2. graphql.js SDL does not contain built-in scalars (String, Int, Boolean, ID, Float).
3. graphql.js introspection result does not contain unused (unreachable) custom types, including custom scalars.
4. graphql.js introspection result does not contain built-in scalars (Float for example) only when schema does not refer to them.
5. graphql.js introspection result always contains String and Boolean builtin scalars since introspection refers to them.
6. GraphiQL and GraphQL Playground work properly with schemas corresponding to the above statements.

Thus, I expect that the current state is such that neither the SDL nor the introspection result contain more than the minimum data that is needed for functioning (there is no unnecessary, redundant information).

I would be grateful for the comments.