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

Breaking change with deprecated fields not being included by default #2834

Closed
Betree opened this issue Oct 30, 2020 · 12 comments · Fixed by #2855
Closed

Breaking change with deprecated fields not being included by default #2834

Betree opened this issue Oct 30, 2020 · 12 comments · Fixed by #2855
Labels

Comments

@Betree
Copy link

Betree commented Oct 30, 2020

In 15.4.0 release, this change was merged which prevents deprecated field from being returned when fetching the schema by default. Because these fields were returned before, this is a breaking change for projects that use this to update a local file for their schema.

In our case, the schema is used by a linter and our CI is now blocked because some fields are not in the schema anymore, yet we still use them in some places.

Proposed solutions:

  • Change includeDeprecated's defaultValue to true
  • Or mark this change as breaking in the release notes

Happy to contribute to one of these options if the maintainers think it's the way to go.
Ping @IvanGoncharov since you worked on this recently.

@Betree
Copy link
Author

Betree commented Oct 30, 2020

Related: having a includeDeprecated param on IntrospectionOptions could help to update existing tools

@IvanGoncharov
Copy link
Member

In 15.4.0 release, this change was merged which prevents deprecated field from being returned when fetching the schema by default. Because these fields were returned before, this is a breaking change for projects that use this to update a local file for their schema.

@Betree You pointed to this line:
https://github.com/APIs-guru/graphql-js/blob/ca0ed9902ff48da9a0e8bd1a9eac30d1c15b09c1/src/type/introspection.js#L341

And this line adds includeDeprecated to args and set defaults to false.
It's not a breaking change since prior to 15.4.0 you weren't able to deprecate arguments thus you should get exactly the same introspection as before.

In our case, the schema is used by a linter and our CI is now blocked because some fields are not in the schema anymore, yet we still use them in some places.

It's strange since #2733 should be strictly additive change.
Can you please share both introspection dumps before and after the update?

@robrichard
Copy link
Contributor

This might be a similar issue to what I ran into when upgrading to 15.4.0.

Some developers working on our schema did not realize it was not possible to deprecate arguments in older versions of graphql-js, and had added a deprecationReason which was ignored in all versions prior to 15.4.0.

In 15.4.0, the depreciationReason causes arguments to not be returned by the default introspection query, giving the effect of these arguments "disappearing" from schema introspection when upgrading to 15.4.0.

@Betree
Copy link
Author

Betree commented Nov 2, 2020

Can you please share both introspection dumps before and after the update?

On this codebase: https://github.com/opencollective/opencollective-api. Schema is generated thanks to get-graphql-schema.
I've pushed this commit for demonstration: opencollective/opencollective-api@53dbf7a

To reproduce:

Note: server needs to be restarted manually after updating dependencies.

The API is fully opensource, so feel free to look at the code for the schema. One of the removed fields for example is this one: https://github.com/opencollective/opencollective-api/blob/master/server/graphql/v2/input/AccountReferenceInput.js#L15

@IvanGoncharov
Copy link
Member

@Betree Thanks for reproduce the scenario 👍
It looks like it's the same scenario as @robrichard reported but in your case, you used deprecatedReason on input fields (unsupported in 15.3.0).

@Betree @robrichard So how critical this issue for your project?

@robrichard
Copy link
Contributor

It's not critical for my project, but it would be nice to be able to query these deprecated input fields and arguments with the introspection query

@IvanGoncharov
Copy link
Member

@robrichard Yes, you are able to do that if you use getIntrospectionQuery function from graphql@15.4.0 to generate an introspection query.

@Betree
Copy link
Author

Betree commented Nov 4, 2020

In our case it's preventing the update to 15.4.0 at the moment because we use some deprecated fields in our frontend and the linter will fail if they're not in the schema.

Maybe we can open a PR on https://github.com/prisma-labs/get-graphql-schema to include deprecated fields by default? I'm not sure how to achieve this with getInstrospectionQuery though, the prototype for IntrospectionOptions doesn't mention anything related to deprecated fields.

@Betree
Copy link
Author

Betree commented Nov 19, 2020

@IvanGoncharov Any suggestion on what we should do to resolve this issue?

@IvanGoncharov
Copy link
Member

Maybe we can open a PR on prisma-labs/get-graphql-schema to include deprecated fields by default? I'm not sure how to achieve this with getInstrospectionQuery though, the prototype for IntrospectionOptions doesn't mention anything related to deprecated fields.

Yes, I forget to update getInstrospectionQuery in my original PR and was to stubborn to actually check 🤦‍♂️
Working on a fix right now.

@IvanGoncharov
Copy link
Member

@Betree I added a flag to include deprecated args in #2855
Plan to release it this week.

@Betree
Copy link
Author

Betree commented Dec 16, 2020

(Late) thank you @IvanGoncharov! I'll make the change for us to use this new parameter.

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

Successfully merging a pull request may close this issue.

3 participants