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

WIP: Input field deprecation support #3015

Merged
merged 3 commits into from
Sep 4, 2020

Conversation

jturkel
Copy link
Contributor

@jturkel jturkel commented Jul 9, 2020

This PR implements the RFC for input field deprecation support. The RFC was moved to stage 2 at the last working group. I don't think we want to merge this PR until the corresponding graphql-js PR merges (or the RFC moves to the accepted state) but I wanted to open a WIP PR in case there's an early feedback on my implementation and to prevent anyone from doing duplicate work.

@jturkel jturkel mentioned this pull request Jul 9, 2020
Copy link
Owner

@rmosolgo rmosolgo left a comment

Choose a reason for hiding this comment

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

This looks perfect, thanks so much for knocking it out.

If the RFC is moved along and I miss it, just ping me here and I'll merge. (Honestly, this is a solid feature. I'd be happy to move faster than the RFC whenever you're ready to start using it!)

@Marthyn
Copy link

Marthyn commented Aug 3, 2020

Thanks for this PR! Was just about to use it but then found out it's being worked on.

@rgraff
Copy link
Contributor

rgraff commented Aug 3, 2020

Could really use this as well.

@jturkel
Copy link
Contributor Author

jturkel commented Aug 3, 2020

@Marthyn / @rgraff - I wasn't in a rush to merge this since none of my clients currently leverage this information. Also the interaction with introspection is still undergoing some discussion. As written here and in graphql-js, marking an argument as deprecated will hide it from introspection queries (even if it's required) unless clients pass includeDeprecated: true. This is challenging for generic clients since they don't know if the server does or doesn't support argument deprecation so a two phase introspection process is required. All that said, I'd be happy to work on getting this merged sooner rather than later if you're going to use it now.

@kroehre
Copy link

kroehre commented Aug 11, 2020

@jturkel I have a case for this as well, would love to see this merged!

@jturkel jturkel force-pushed the feature/argument-deprecation branch from 43df8d3 to 253a8c5 Compare August 11, 2020 17:50
@jturkel
Copy link
Contributor Author

jturkel commented Aug 11, 2020

@rmosolgo - I've rebased this branch to resolve the merge conflict. We're going to hold off using this feature until graphql-js merges/releases graphql/graphql-js#1560 but I'm totally fine merging the graphql-ruby PR as-is if other folks want to use the feature sooner.

@kroehre
Copy link

kroehre commented Aug 17, 2020

@rmosolgo Any blockers here? I would love to use this feature. Thanks!

@rusterholz
Copy link

@jturkel Looks like that PR you linked to has been open for two years. Any chance that this PR could get merged before that one?

@bilby91
Copy link

bilby91 commented Aug 20, 2020

Looking forward on this feature!!

# Conflicts:
#	spec/graphql/schema/argument_spec.rb
@jturkel
Copy link
Contributor Author

jturkel commented Sep 3, 2020

Looks like the graphql-js change merged in graphql/graphql-js#2733. The only change from what I implemented here is that you can't deprecate required arguments. I'll make that change tomorrow.

@rmosolgo
Copy link
Owner

rmosolgo commented Sep 4, 2020

👍 Thanks for the update, @jturkel . Could you @-me after pushing those changes? Sorry I've been slow to merge, I'll go ahead and merge it when that's taken care of.

@jturkel jturkel force-pushed the feature/argument-deprecation branch from 9520f98 to 4e2c13a Compare September 4, 2020 14:53
@jturkel
Copy link
Contributor Author

jturkel commented Sep 4, 2020

@rmosolgo - I added validation that deprecated arguments aren't required so this should be ready to go. I made that change in a separate commit for easier review but I'd be happy to rebase/squash to keep the master commit history clean if you want.

@rmosolgo rmosolgo added this to the 1.11.5 milestone Sep 4, 2020
@rmosolgo
Copy link
Owner

rmosolgo commented Sep 4, 2020

Awesome, thanks again for your work on this!

@rmosolgo rmosolgo merged commit d8eb11e into rmosolgo:master Sep 4, 2020
@rmosolgo
Copy link
Owner

rmosolgo commented Sep 4, 2020

Anyone who wants to give this a try before the next release can pull the gem from github:

gem "graphql", github: "rmosolgo/graphql-ruby"

@Marthyn
Copy link

Marthyn commented Sep 5, 2020

Awesome! Thank you!

@@ -743,10 +743,16 @@ def assert_schema_and_compare_output(definition)
VALUE
}

input MyInput {
int: Int @deprecated(reason: "This is not the argument you're looking for")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the argument I'm looking for.

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.

8 participants