Pass GraphQLSchema instead of GraphQLDirective to getDirectiveDeclara…

…tion.

It turns out you have to have the schema available to reconstruct any
GraphQLDirective that refers to existing schema types, and given the
schema you can always just call schema.getDirective(directiveName) if you
care about reusing previously defined directives.

docs/source/schema-directives.md

@@ -116,26 +116,67 @@ Enforcing the requirements of the declaration is something a `SchemaDirectiveVis
 However, if you're attempting to implement a reusable `SchemaDirectiveVisitor`, you may not be the one writing the SDL syntax, so you may not have control over which directives the schema author decides to declare, and how. That's why a well-implemented, reusable `SchemaDirectiveVisitor` should consider overriding the `getDirectiveDeclaration` method:
 
 ```typescript
+import {
+  DirectiveLocation,
+  GraphQLDirective,
+  GraphQLEnumType,
+} from "graphql";
+
 class AuthDirectiveVisitor extends SchemaDirectiveVisitor {
   public visitObject(object: GraphQLObjectType) {...}
   public visitFieldDefinition(field: GraphQLField<any, any>) {...}
 
   public static getDirectiveDeclaration(
     directiveName: string,
-    previousDirective?: GraphQLDirective,
+    schema: GraphQLSchema,
   ): GraphQLDirective {
-    previousDirective.args.forEach(arg => {
-      if (arg.name === 'requires') {
-        // Lower the default minimum Role from ADMIN to REVIEWER.
-        arg.defaultValue = 'REVIEWER';
-      }
+    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;
+    }
+
+    // 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.
+
+    return new GraphQLDirective({
+      name: directiveName,
+      locations: [
+        DirectiveLocation.OBJECT,
+        DirectiveLocation.FIELD_DEFINITION,
+      ],
+      args: {
+        requires: {
+          // Having the schema available here is important for obtaining
+          // references to existing type objects, such as the Role enum.
+          type: (schema.getType('Role') as GraphQLEnumType),
+          // Set the default minimum Role to REVIEWER.
+          defaultValue: 'REVIEWER',
+        }
+      }]
     });
-    return previousDirective;
   }
 }
 ```
 
-Since the `getDirectiveDeclaration` method receives not only the name of the directive but also any previous declaration found in the schema, it can either return a totally new `GraphQLDirective` object, or simply modify the `previousDirective` and return it. Either way, if the visitor returns a non-null `GraphQLDirective` from `getDirectiveDeclaration`, that declaration will be used to check arguments and permissible locations.
+Since the `getDirectiveDeclaration` method receives not only the name of the directive but also the `GraphQLSchema` object, it can modify and/or reuse previous declarations found in the schema, as an alternative to returning a totally new `GraphQLDirective` object. Either way, if the visitor returns a non-null `GraphQLDirective` from `getDirectiveDeclaration`, that declaration will be used to check arguments and permissible locations.
 
 ## Examples
 

src/schemaVisitor.ts

@@ -298,9 +298,9 @@ export class SchemaDirectiveVisitor extends SchemaVisitor {
   // appear. By default, any declaration found in the schema will be returned.
   public static getDirectiveDeclaration(
     directiveName: string,
-    previousDeclaration?: GraphQLDirective,
+    schema: GraphQLSchema,
   ): GraphQLDirective {
-    return previousDeclaration;
+    return schema.getDirective(directiveName);
   }
 
   // Call SchemaDirectiveVisitor.visitSchemaDirectives to visit every
@@ -433,15 +433,7 @@ export class SchemaDirectiveVisitor extends SchemaVisitor {
     // goes to the trouble of implementing getDirectiveDeclaration, it should
     // be able to rely on that implementation.
     each(directiveVisitors, (visitorClass, directiveName) => {
-      const decl = visitorClass.getDirectiveDeclaration(
-        directiveName,
-        // Give the getDirectiveDeclaration method a chance to look at the
-        // existing declaration, in case it only needs to make minor tweaks.
-        // Ironically, this grants the SchemaDirectiveVisitor subclass the
-        // ability to visit/inspect/modify directive declarations found in
-        // the schema, even though they can't be decorated with @directives!
-        declaredDirectives[directiveName] || null,
-      );
+      const decl = visitorClass.getDirectiveDeclaration(directiveName, schema);
       if (decl) {
         declaredDirectives[directiveName] = decl;
       }

src/test/testDirectives.ts

@@ -18,7 +18,6 @@ import {
   GraphQLInputObjectType,
   GraphQLObjectType,
   GraphQLSchema,
-  GraphQLDirective,
   GraphQLString,
   GraphQLID,
   GraphQLScalarType,
@@ -454,7 +453,10 @@ describe('@directives', () => {
 
   it('can also handle declared arguments', () => {
     const schemaText = `
-    directive @oyez(times: Int = 5) on OBJECT | FIELD_DEFINITION
+    directive @oyez(
+      times: Int = 5,
+      party: Party = IMPARTIAL,
+    ) on OBJECT | FIELD_DEFINITION
 
     schema {
       query: Courtroom
@@ -463,16 +465,12 @@ describe('@directives', () => {
     type Courtroom @oyez {
       judge: String @oyez(times: 0)
       marshall: String @oyez
-      lawyers(
-        # Should @oyez be disallowed here, since it wasn't declared with
-        # the ARGUMENT_DEFINITION location, or simply ignored?
-        party: Party @oyez(times: 0)
-      ): [String]
     }
 
     enum Party {
       DEFENSE
       PROSECUTION
+      IMPARTIAL
     }`;
 
     const schema = makeExecutableSchema({ typeDefs: schemaText });
@@ -485,8 +483,10 @@ describe('@directives', () => {
       oyez: class extends SchemaDirectiveVisitor {
         public static getDirectiveDeclaration(
           name: string,
-          prev: GraphQLDirective,
+          theSchema: GraphQLSchema,
         ) {
+          assert.strictEqual(theSchema, schema);
+          const prev = schema.getDirective(name);
           prev.args.some(arg => {
             if (arg.name === 'times') {
               // Override the default value of the times argument to be 3
@@ -510,6 +510,7 @@ describe('@directives', () => {
           } else if (field.name === 'marshall') {
             assert.strictEqual(this.args.times, 3);
           }
+          assert.strictEqual(this.args.party, 'IMPARTIAL');
         }
       }
     }, context);