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

findBreakingChanges yields TYPE_REMOVED when a single instance of a scalar is removed #2197

Closed
maxuuell opened this issue Sep 24, 2019 · 4 comments · Fixed by #2204
Closed

Comments

@maxuuell
Copy link

An interesting behavior was found when playing around with findBreakingChanges between two schemas.

When two schemas are compared, with one of them having only one Int type, and the second removes the only Int type, the breaking changes array will show:

[ { type: 'TYPE_REMOVED',
    description: 'Int was removed.' },
{ type: 'FIELD_REMOVED',
description: 'User.age was removed.' } ]

Should this be two breaking changes? It seems odd to say removing the last instance of a scalar type would be a breaking change, as opposed to removing a defined object type.

Does that mean when a schema is created, it will strip out any scalars that aren't being used?

@IvanGoncharov
Copy link
Member

Does that mean when a schema is created, it will strip out any scalars that aren't being used?

Yes, exactly that. We are in process of clarifying it in the spec:
https://github.com/graphql/graphql-spec/pull/597/files#diff-86a7cb172ae1d944ca3719bd848b1d8bR356-R359

When returning the set of types from the __Schema introspection type, all
referenced built-in scalars must be included. If a built-in scalar type is not
referenced anywhere in a schema (there is no field, argument, or input field of
that type) then it must not be included in the schema.

So technically you removed Int from your schema by removing User.age.
I don't think we should hide this fact but instead, I created #2204 to change this message to:

Standard scalar Int was removed because it is not referenced anymore.

@maxuuell What do you think?

@wtrocki
Copy link
Contributor

wtrocki commented Sep 27, 2019

So I can create a schema with only my types and once schema is created some additional scalars etc. will be added. Is there a method to strip out this additional element? Getting a config from existing schema will still contain them and I saw people doing this manually in community libraries.

It that is possible then this schema can be passed to compare method and it will produce diff only for what is specified in schema string, which some people might expect.

Without that filtering on changes is needed - an assessment that this scalar was my own or it was comming from core lib.,

IvanGoncharov added a commit to IvanGoncharov/graphql-js that referenced this issue Sep 27, 2019
@maxuuell
Copy link
Author

@IvanGoncharov Thank you for clarifying and circling back. I think the newer message is fine.

@IvanGoncharov
Copy link
Member

@wtrocki SDL is not a single representation of the spec, for example introspection contains definitions for all types (including standard scalars).
It would be strange if something suddenly disappears from introspection but wasn't reflected in findBreakingChanges. I agree that filtering would be a great addition but it should be disscuss in context of #2181

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

Successfully merging a pull request may close this issue.

3 participants