Bit confused on getDirectiveDeclaration

[bradennapier]

NOTE: I am fully aware I am misunderstanding something here! I made the issue as I think the docs should do a better job of defining this static method.

So according to the documentation it seems to make that by defining the static getDirectiveDeclaration it would mean that the directive should work even if the schema does not declare it at all.

This appears to be untrue and the example appears to be pointless for the most part. I am not sure I understand any part of the purpose of it. I am sure I am just missing a point.

First of all, I see the reasoning why a valid declaration would be required and that something like getDirectiveDeclaration would be used simply to validate the users declared directive and throw an error during startup should it be invalid.

However, according to the documentation in the example:

// If a previous directive with this name was not found in the schema,
// there are several options:
//
// 1. Construct a new GraphQLDirective (see below).
// 2. Throw an exception to force the client to declare the directive.
// 3. Return null, and forget about declaring this directive.
//
// All three are valid options, since the visitor will still work without
// any declared directives. In fact, unless you're publishing a directive
// implementation for public consumption, you can probably just ignore
// getDirectiveDeclaration altogether.

This makes it seem like if the directive were not declared in the typeDefs that it would allow constructing a new directive that would be used in this case. This is not true and doesn't work.

Error: Unknown directive "resolve".

Unknown directive "resolve".

when

makeExecutableSchema({
  typeDefs,
  resolvers,
  schemaDirectives: {
    resolve: ResolveDirective,
  },
});

Which in this case I am trying to log and just play with the idea, in practice I believe it makes more sense to allow for this but for this to be strictly used to validate the user declared it properly.

class ResolveDirective extends SchemaDirectiveVisitor {
  static getDirectiveDeclaration(directiveName, schema) {
    console.log('Get Directive: ', directiveName);
    const previousDirective = schema.getDirective(directiveName);
    if (previousDirective) {
      return previousDirective;
    }

    return new GraphQLDirective({
      name: directiveName,
      locations: [DirectiveLocation.FIELD_DEFINITION],
      args: {
        key: {
          type: GraphQLString,
        },
      },
    });
  }
  
   // other methods...
}

I guess with how it works now (only working if the directive is actually declared) means I can't see why the last part of this example where a new directive is built makes any sense at all to exist since the previous directive should ALWAYS exist?

In addition I can't see how this is a good idea:

const previousDirective = schema.getDirective(directiveName);
    if (previousDirective) {
      // If a previous directive declaration exists in the schema, it may be
      // better to modify it than to return a new GraphQLDirective object.
      previousDirective.args.forEach(arg => {
        if (arg.name === 'requires') {
          // Lower the default minimum Role from ADMIN to REVIEWER.
          arg.defaultValue = 'REVIEWER';
        }
      });

      return previousDirective;
    }

Overwriting the declared type silently and reducing auth permissions without the user knowing sounds like a terrible idea.

Is this only meant to be used in the case there are cascading duplicate declarations of the directive when something like merge is being doing? This would make a lot more sense but isn't defined too clearly in the docs if it is the case.

[bradennapier]

Just tested and doesnt appear to be the multiple declaration thing as it only ever executes once and a prev declaration (the one made in my file) is always present - so this example seems to be incorrect and dated since the final branch that declares a new directive will never occur as an error will happen if the schema did not already declare the directive.

[gerwinbrunner]

I'm running into the same issue. Any new about that?

[ravangen]

I have the same problem and was able to find this comment from graphql-tools issue #957.

I currently believe getDirectiveDeclaration no longer has much of a role when used with graphql 14.

From the changelog:

NOTE: graphql 14 includes breaking changes. We're bumping the major version of graphql-tools to accommodate those breaking changes. If you're planning on using graphql 14 with graphql-tools 4.0.0, please make sure you've reviewed the graphql breaking changes list.

This is likely caused by the fact that graphql-js now requires you to define your directives in your schema, before you attempt to use them. For example:

directive @upper on FIELD_DEFINITION

type TestObject {
  hello: String @upper
}

You can likely work around this by pre-defining your directives in your schema, but I'd like to confirm this. If this works, we'll need to update the docs.

Source: https://stackoverflow.com/a/52921858/9128503

[Sytten]

Is this solved? I just tried and the function is not called...

[yaacovCR]

This should be fixable by adding in typedefs from getDirectiveDeclaration prior to calling buildSchema. In general, the declaration might depend on additional input types, but support for that can be added later. The latter is akin to adding true schema merging capability into makeExecutableSchema (as opposed to the stitching/proxying done by the confusingly named mergeSchema).

Folding into #1306

[yaacovCR]

Moving to v5.1, reopening to move any relevant discussion here.

[yaacovCR]

Hi!

Closing this issue for now as within v6, as the difficulty is not that getDirectiveDeclaration is not working, but just that graphql-js v14 and above, buildSchema requires directives to be declared. One can reverse this requirement by telling buildSchema to assume the SDL is invalid or passing custom rules, but this seems like a lot of rearchitecting when existing workarounds are not too bad:

1. authors of packages containing the older reusable schema directive classes or the newer schema transform function authors can:
   A) publish instructions on how to include the directive declaration
   B) export the required directive declaration and encourage users to pass an array of typeDefs that includes the export rather than a single string to makeExecutableSchema
   C) a combination of above as necessary

2. users of packages can choose to bypass altogether the need to declare directives by using stitchSchemas (or mergeSchemas in versions < v6) which does not require declared directives. Using stitchSchemas (or mergeSchemas in versions < v6) without specifying subschemas but with inclusion of typeDefs, resolvers, and the other makeExecutableSchema options should give you an essentially identical schema as when using makeExecutableSchema, but does not use buildSchema -- it builds types on its own rather than using buildSchema from graphql-js and therefore does not require directives to be declared.

Rather than changing makeExecutableSchema to act more like stitchSchemas, users should be able to specifically choose which approach to take.

[Sytten]

@yaacovCR Could you please document that somewhere (maybe in the readme or directly in the code), because I would never have known as a regular user if I was not watching this issue and its a very confusing thing currently.

[yaacovCR]

Good idea! I believe #1463 documents it for the newer schema transforms.

We can definitely accept PRs to clarify retained legacy class based schema directive documentation around this, and after v6 lands would be a good time to fill in docs in general.

Maybe you could create a separate issue after v6 lands to track whatever docs deficiency there still is...