-
-
Notifications
You must be signed in to change notification settings - Fork 816
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
Bring attachDirectiveResolvers to graphql-tools #518
Bring attachDirectiveResolvers to graphql-tools #518
Conversation
@giautm hi, thanks for opening the PR! This looks like a very useful feature. I think we should make sure we agree on the API and what the directives should do. There’s been a lot of effort already put in on the code and it looks good, but we also need to go over the developer experience of using the new feature. Can you add in your PR description:
|
@stubailo: I just update PR description and example. Please take a look. I'm not good for writing document. So can you help me writing it? Thanks. |
Wow awesome, this is exactly what I was looking for! The docs are actually awesome since the best documentation is examples :) Can you tell me a bit more what the "QUERY | FIELD" thing means? |
From tutorial: https://medium.com/the-graphqlhub/graphql-tour-directives-558dee4fa903 |
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.
Hmm, is it true that all of these examples are only FIELD_DEFINITION
? I don't think these directives are used on queries anywhere in the tests.
According to the new schema definition language spec @leebyron is working on, here are the acceptable locations:
DirectiveLocation : one of QUERY SCHEMA ENUM MUTATION SCALAR ENUM_VALUE SUBSCRIPTION OBJECT INPUT_OBJECT FIELD FIELD_DEFINITION INPUT_FIELD_DEFINITION FRAGMENT_DEFINITION ARGUMENT_DEFINITION FRAGMENT_SPREAD INTERFACE INLINE_FRAGMENT UNION
Would it be fair to say that this PR specifically adds support for directives on field definitions? Since the implementation is resolver-based, it feels like it wouldn't work for any other use case.
Here's an interesting alternative design:
Directives could be allowed to operate on the entire unit that they are attached to. For example, a field directive would get to modify everything about the field it's on, using the graphql-js
GraphQLField
: https://github.com/graphql/graphql-js/blob/9e6d32af108abc979d07f4aac3b52808b019dabd/src/type/definition.js#L675
That way, you could actually implement stuff like the @deprecated
directive using this feature (but with the current version, you couldn't).
So based on the above generalization, maybe these should be called "resolver directives" since they are specifically directives that modify how the resolver function works.
I think I need another day or so to think about this. Directives are a core part of GraphQL, and are being added to the schema language in the spec right now, so it feels important to get it right.
src/schemaGenerator.ts
Outdated
* Attach directive resolvers to Schema | ||
* https://github.com/apollographql/graphql-tools/issues/212#issuecomment-324447078 | ||
* | ||
* @author Agon Bina <agon_bina@hotmail.com> |
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.
We usually don't track authors in this way, since we don't have them for any other functions. Would you prefer to have some attribution other than just Git/GitHub's tracking of committers?
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.
Removed
src/test/testSchemaGenerator.ts
Outdated
|
||
describe('attachDirectives', () => { | ||
const testSchemaWithDirectives = ` | ||
directive @upper on QUERY | FIELD |
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 this should just be on FIELD_DEFINITION
.
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.
Edited
@stubailo hi, are you done with any changes? |
I'm waiting for this merging too :) |
Close-and-reopen for CI deploy |
Waiting for merging |
@giautm One of the most wanted features, thanks! Hope it will be merged soon. 🙏 |
Wow, this is amazing! Thanks! |
* Added some doc for #518 * Update correct link to example * Remove tabs
Follow-up to this PR, for anyone interested in visiting other kinds of schema types/fields/values: #640 |
Description: This PR allows implementing custom directives that can be used for translation, authentication and error tracking.
API:
Example:
@upper
directive to upper string from resolver:Implement of
@upper
directive.@auth
directive check roles before run resolver:Implement of
@auth
directive.Implement of
@errorCatch
directive.Issue: #212
TODO: