Skip to content

WIP: Input field deprecation support - #3015

Merged
rmosolgo merged 3 commits into
rmosolgo:masterfrom
jturkel:feature/argument-deprecation
Sep 4, 2020
Merged

WIP: Input field deprecation support#3015
rmosolgo merged 3 commits into
rmosolgo:masterfrom
jturkel:feature/argument-deprecation

Conversation

@jturkel

@jturkel jturkel commented Jul 9, 2020

Copy link
Copy Markdown
Contributor

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

@rmosolgo rmosolgo left a comment

Copy link
Copy Markdown
Owner

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

Marthyn commented Aug 3, 2020

Copy link
Copy Markdown

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

@rgraff

rgraff commented Aug 3, 2020

Copy link
Copy Markdown
Contributor

Could really use this as well.

@jturkel

jturkel commented Aug 3, 2020

Copy link
Copy Markdown
Contributor Author

@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

kroehre commented Aug 11, 2020

Copy link
Copy Markdown

@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

jturkel commented Aug 11, 2020

Copy link
Copy Markdown
Contributor Author

@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

kroehre commented Aug 17, 2020

Copy link
Copy Markdown

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

@rusterholz

Copy link
Copy Markdown

@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

bilby91 commented Aug 20, 2020

Copy link
Copy Markdown

Looking forward on this feature!!

# Conflicts:
#	spec/graphql/schema/argument_spec.rb
@jturkel

jturkel commented Sep 3, 2020

Copy link
Copy Markdown
Contributor Author

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

rmosolgo commented Sep 4, 2020

Copy link
Copy Markdown
Owner

👍 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

jturkel commented Sep 4, 2020

Copy link
Copy Markdown
Contributor Author

@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

rmosolgo commented Sep 4, 2020

Copy link
Copy Markdown
Owner

Awesome, thanks again for your work on this!

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

rmosolgo commented Sep 4, 2020

Copy link
Copy Markdown
Owner

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

Marthyn commented Sep 5, 2020

Copy link
Copy Markdown

Awesome! Thank you!

}

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

Copy link
Copy Markdown
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