-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Add deprecated directive to arguments and input values #1560
Conversation
@smitt04 Great 🎉 Please, ping me when you will be ready for review. |
@IvanGoncharov I think I am ready to be reviewed. Let me know if there are more tests that need to be implemented. |
I'm not familiar with the graphql repo library architecture. Once this feature is added into graphql-js, will it be available in express-graphql? Or will that require a new PR to duplicate the feature? |
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.
@IvanGoncharov this looks good to me, but I think you have more context on how deprecation currently fits in the type definitions, so I think you're a better reviewer for this.
@smitt04 I will take another look at this in the next week if needed.
i attempted the rebase/merge here - one spec is still failing though. |
I tried updating the pr |
218e1bc
to
05717ee
Compare
src/type/definition.js
Outdated
deprecationReason: arg.deprecationReason, | ||
isDeprecated: arg.deprecationReason != null, | ||
extensions: arg.extensions, | ||
astNode: arg.astNode, |
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.
nit: can we make sure these arguments are always in the same order?
@@ -1547,6 +1563,7 @@ export type GraphQLInputFieldConfig = {| | |||
description?: ?string, | |||
type: GraphQLInputType, | |||
defaultValue?: mixed, | |||
deprecationReason?: ?string, |
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.
isDeprecated missing here?
Should getIntrospectionQuery be updated to include deprecated input fields and arguments to be consistent with the current behavior of including deprecated fields and enum values? |
05717ee
to
08374e7
Compare
This is a WIP and is the start for supporting
deprecated
directive in arguments and input values.This is in relation to RFC Proposal