Add support for directives on directive definitions#4521
Add support for directives on directive definitions#4521BoD wants to merge 18 commits intographql:16.x.xfrom
Conversation
|
@BoD is attempting to deploy a commit to the The GraphQL Foundation Team on Vercel. A member of the Team first needs to authorize it. |
|
Update includeDeprecated arg per @benjie 's comments, then we'll merge |
|
Just made the change for |
jerelmiller
left a comment
There was a problem hiding this comment.
Had a few suggestions, mostly around types, but otherwise looks good from my end!
You probably want to get a proper review from someone who knows the conventions in this codebase better than I do, but wanted to try and help move this along in some way!
| * Directives are used by the GraphQL runtime as a way of modifying execution | ||
| * behavior. Type system creators will usually not create these directly. | ||
| */ | ||
| export class GraphQLDirective { |
There was a problem hiding this comment.
Would it make sense to add an isDeprecated getter on this class? Not sure if other types do the same, but since we use isDeprecated in other places (like introspection), this might be a nice shortcut for those defining directives instances.
get isDeprecated() {
return this.deprecationReason == null;
}Which means you can use this in places like introspection.ts
- .filter((directive) => directive.deprecationReason == null),
+ .filter((directive) => directive.isDeprecated),
// ...
- resolve: (directive) => directive.deprecationReason != null,
+ resolve: (directive) => directive.isDeprecated,Would love for someone with more knowledge of the codebase to chime in on whether this is a good idea or against the design principles of this library.
There was a problem hiding this comment.
I think that would be a good change, but we'd want to do the same to other types like fields, arguments, enum values etc. for consistency. Although doing that would be a bit out of scope in this PR.
Co-authored-by: Jerel Miller <jerelmiller@gmail.com>
Co-authored-by: Jerel Miller <jerelmiller@gmail.com>
Co-authored-by: Jerel Miller <jerelmiller@gmail.com>
Co-authored-by: Jerel Miller <jerelmiller@gmail.com>
Co-authored-by: Jerel Miller <jerelmiller@gmail.com>
|
I've not fully reviewed this PR, but the changes since Lee's review all look fine to me - thanks @BoD! 👍 |
Allow directives on directive definitions, based on this spec PR which introduces this syntax:
Disclaimer
First time on this codebase and am also not a JS/TS person, so obvious mistakes and/or missing pieces are very likely 😅 Any help and feedback to improve this PR are very welcome 🙏. The tests seem to pass but additional tests may be needed.