-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Allow deprecation of input values #525
Conversation
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
spec/Section 3 -- Type System.md
Outdated
@@ -1778,3 +1778,15 @@ type ExampleType { | |||
oldField: String @deprecated(reason: "Use `newField`.") | |||
} | |||
``` | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you also need to expand above text:
The @deprecated directive is used within the type system definition language to indicate deprecated portions of a GraphQL service’s schema, such as deprecated fields on a type or deprecated enum values.
spec/Section 4 -- Introspection.md
Outdated
@@ -332,6 +334,8 @@ Fields | |||
* `name` must return a String. | |||
* `description` may return a String or {null}. | |||
* `inputFields`: a list of `InputValue`. | |||
* Accepts the argument `includeDeprecated` which defaults to {false}. If | |||
{true}, deprecated enum values are also returned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{true}, deprecated enum values are also returned. | |
{true}, deprecated fields are also returned. |
spec/Section 4 -- Introspection.md
Outdated
@@ -392,6 +396,9 @@ Fields | |||
* `defaultValue` may return a String encoding (using the GraphQL language) of the | |||
default value used by this input value in the condition a value is not | |||
provided at runtime. If this input value has no default value, returns {null}. | |||
* `isDeprecated` returns {true} if this field or argument should no longer be used, | |||
otherwise {false}. | |||
* `deprecationReason` optionally provides a reason why this field or argument is deprecated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* `deprecationReason` optionally provides a reason why this field or argument is deprecated. | |
* `deprecationReason` optionally provides a reason why this input field or argument is deprecated. |
@smitt04 Looks good so far, I left a few review feedbacks. Also, we need to discuss it on WG, here is the next one: |
@IvanGoncharov I have update the PR with suggested fixes. I have also submitted a PR to graphql-wg. I can start looking into graphql-js, although I haven't worked in javascript graphql for a couple years. I am mostly working with golang graphql as of late. |
NP, you can just open an issue on |
I just want to cheer you on and add my hopes that this will be merged and added soon. 🙌 It's quite a wonderful feeling to realize you're missing a feature, and when you start looking for discussions about it you find this (on graphql/graphql-js#1560): |
Should we only allow the deprecated directive on nullable arguments? This can not be expressed in the directive definition, but logically that might make sense. |
@IvanGoncharov |
I'm also really looking forward to this feature! |
@Anufant We discussed it on the last WG and everyone agreed that this issue should be fast-tracked so it definitely going into next spec release. Not sure about the exact date since there are a few things we need to discuss and implement in |
Awesome, glad this is moving! Does that mean the "Next Stage?" tag can be updated to a later stage? :) |
What is the next step here, it seems it has a champion with smitt04 having written both the propsal here and a (now stale) implementation in graphql-js "discussed it on the last WG and everyone agreed .. definitely going into next spec release" sounds like acceptance by WG so stage 3. Is there something a community member can do to get this merged in on the fast tracked path ? |
wow |
Co-authored-by: Ivan Goncharov <ivan.goncharov.ua@gmail.com>
FYI for those tracking the issue, it has not officially merged into the spec yet, it has just moved to the next stage. See the actual changes here: #805 |
Co-authored-by: Ivan Goncharov <ivan.goncharov.ua@gmail.com>
Started the @deprecated proposal in reference to #197
Please let me know if there is anything i missed or you would like changed. This was just a quick update to get the ball rolling again.
@IvanGoncharov Are you still interested in reviewing this?