Resolver directives continuation

[stubailo]

#518 continuation

TODO:

• If this PR is a new feature, reference an issue where a consensus about the design was reached (not necessary for small changes)
• Make sure all of the significant new logic is covered by tests
• Rebase your changes on master so that they can be merged easily
• Make sure all tests and linter rules pass
• Update CHANGELOG.md with your change

[stubailo]

@giautm opened a new PR so we can both push to the same one; I invited you to be a contributor! https://github.com/apollographql/graphql-tools/invitations

I'm going to rename directiveResolvers to resolverDirectives since these are "directives that apply to resolvers"

Never mind. I do think the directive should be on FIELD_DEFINITION though according to this: https://github.com/facebook/graphql/pull/90/files#diff-c9d195396161bb690c5fd8899af11d78R280

[gregberge]

@giautm thanks for your work! @stubailo thanks for merging it!

I found several problems in the current implementation:

• Using directives in query is not supported
• We do not have any error if resolver is not defined
• We do not check if directive has the location "FIELD_DEFINITION"
• The naming is not consistent with existing API (addResolveFunctionsToSchema != attachDirectiveResolvers, it should be addDirectiveResolveFunctionsToSchema)

I fixed all of these issues in a separated package: graphql-directive.

If we want Apollo Client to be compatible with directives in query we need to merge this PR. It fixes a cache issue.

I think we could improve the current function by using graphql-directive implementation.

What do you think?

[giautm]

Hi @neoziro, there is my response:

• Maybe in the next PR. It's not a problem.
• An FIELD_DEFINITION directive maybe change the field meta info like @deprecated. So it doesn't need to have a resolver function.
• Maybe directives in another location can have a resolver too. So I don't check the location.
• attachDirectiveResolvers maybe rename to addDirectiveResolveFunctionsToSchema if @stubailo agree.

● addDirectiveResolveFunctionsToSchema › FIELD (schema) › should throw an error if resolver is not defined

    Directive @deprecated has no resolver.Please define one using createFieldExecutionResolver().

Yep, your test throws an error for built-in directives.

PS: I don't understand why you did not contribute to #518 while waiting to merge rather than creating a new module?

[gregberge]

@giautm thanks!

I have to fix it for @deprecated, I think we could not throw it for "deprecated" and only for others. Built-in directives are well-known and it is easy to maintain a white-list.

I did not contribute to #518 because I wanted to experiment and try it before merging it in this project. grahpql-tools is massively used and I want to be sure it is good before adding a new feature in this project. Also, I will talk about this in a Meetup and I wanted it to be public and usable before my talk.

[giautm]

Built-in directives are well-known and it is easy to maintain a white-list.

No, I don't think it's a good idea. Because there are a lot of directives from User if they want to add new. Your module will block them to do that. (by throwing an error).

[gregberge]

@giautm I am not sure. I think most of people are not using directives at all and it makes it easier to have a coherent setup. Do you know some projects that are already using custom directives?

[stubailo]

I did not contribute to #518 because I wanted to experiment and try it before merging it in this project. grahpql-tools is massively used and I want to be sure it is good before adding a new feature in this project. Also, I will talk about this in a Meetup and I wanted it to be public and usable before my talk.

Please don't feel that this means we have to move slowly! I think as long as the feature isn't documented we can keep working on changing it. I'll add something to it that says "experimental" for now.