-
Notifications
You must be signed in to change notification settings - Fork 2k
Add 'getAppliedDirectives' function #904
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
Conversation
src/execution/values.js
Outdated
* directive definitions and AST node. | ||
*/ | ||
export function getAppliedDirectives( | ||
directiveDefs: Array<GraphQLDirective>, |
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.
How about accepting a Schema instance instead? Those define all relevant directives
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.
Answered below ⬇️
src/execution/values.js
Outdated
* Prepares an object map of directive name to argument values given a list of | ||
* directive definitions and AST node. | ||
*/ | ||
export function getAppliedDirectives( |
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.
Or - what do you think about having this be a singular function - getAppliedDirective()
which accepts a GraphQLDirective
and a node and returns void | { [string]: mixed }
?
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.
How about accepting a Schema instance instead? Those define all relevant directives
@leebyron I needed this function to only get directives relevant for my tool/library so it doesn't make sense to return all of them, e.g. buildASTSchema
only needs the value of @deprecated
and doesn't care about the rest of directives.
So I changed it to accept a GraphQLDirective
as you suggested.
However, I renamed it to getDirectiveArgs
since I want to keep getAppliedDirectives
name for the function that you suggested above (accepts GraphQLSchema
), which in addition to returning arguments will report unknown directives but it's a topic for a separate PR.
30bab14
to
ab5f5ca
Compare
This PR adds utility function that provides a convenient way to get arguments of one or more applied directives.