isSpecifiedDirective should not assume Directive type (#1837)

These can now both be used as standalone tests.

Also added some unit tests for these predicates.

Author: Cito

src/type/__tests__/predicate-test.js

@@ -12,20 +12,25 @@ import { expect } from 'chai';
 
 import {
   GraphQLScalarType,
+  GraphQLBoolean,
+  GraphQLID,
+  GraphQLInt,
+  GraphQLFloat,
+  GraphQLString,
   GraphQLEnumType,
   GraphQLInputObjectType,
   GraphQLInterfaceType,
   GraphQLObjectType,
   GraphQLUnionType,
   GraphQLList,
   GraphQLNonNull,
-  GraphQLString,
   GraphQLDirective,
   GraphQLIncludeDirective,
   GraphQLSkipDirective,
   GraphQLDeprecatedDirective,
   isType,
   isScalarType,
+  isSpecifiedScalarType,
   isObjectType,
   isInterfaceType,
   isUnionType,
@@ -82,6 +87,10 @@ const ScalarType = new GraphQLScalarType({
   name: 'Scalar',
   serialize() {},
 });
+const Directive = new GraphQLDirective({
+  name: 'Directive',
+  locations: ['QUERY'],
+});
 
 describe('Type predicates', () => {
   describe('isType', () => {
@@ -119,6 +128,11 @@ describe('Type predicates', () => {
       expect(() => assertScalarType(ScalarType)).not.to.throw();
     });
 
+    it('returns false for scalar class (rather than instance)', () => {
+      expect(isScalarType(GraphQLScalarType)).to.equal(false);
+      expect(() => assertScalarType(GraphQLScalarType)).to.throw();
+    });
+
     it('returns false for wrapped scalar', () => {
       expect(isScalarType(GraphQLList(ScalarType))).to.equal(false);
       expect(() => assertScalarType(GraphQLList(ScalarType))).to.throw();
@@ -127,6 +141,56 @@ describe('Type predicates', () => {
     it('returns false for non-scalar', () => {
       expect(isScalarType(EnumType)).to.equal(false);
       expect(() => assertScalarType(EnumType)).to.throw();
+      expect(isScalarType(Directive)).to.equal(false);
+      expect(() => assertScalarType(Directive)).to.throw();
+    });
+
+    it('returns false for random garbage', () => {
+      expect(isScalarType({ what: 'is this' })).to.equal(false);
+      expect(() => assertScalarType({ what: 'is this' })).to.throw();
+    });
+  });
+
+  describe('isSpecifiedScalarType', () => {
+    it('returns true for specified scalars', () => {
+      expect(isSpecifiedScalarType(GraphQLString)).to.equal(true);
+      expect(isSpecifiedScalarType(GraphQLInt)).to.equal(true);
+      expect(isSpecifiedScalarType(GraphQLFloat)).to.equal(true);
+      expect(isSpecifiedScalarType(GraphQLBoolean)).to.equal(true);
+      expect(isSpecifiedScalarType(GraphQLID)).to.equal(true);
+    });
+
+    it('returns false for custom scalar', () => {
+      expect(isSpecifiedScalarType(ScalarType)).to.equal(false);
+    });
+
+    it('returns false for scalar class (rather than specified instance)', () => {
+      expect(isSpecifiedScalarType(GraphQLScalarType)).to.equal(false);
+    });
+
+    it('returns false for wrapped specified scalar', () => {
+      expect(isSpecifiedScalarType(GraphQLList(GraphQLString))).to.equal(false);
+    });
+
+    it('returns false for non-scalar', () => {
+      expect(isSpecifiedScalarType(EnumType)).to.equal(false);
+      expect(isSpecifiedScalarType(Directive)).to.equal(false);
+    });
+
+    it('returns false for spec defined directive', () => {
+      expect(isSpecifiedScalarType(GraphQLSkipDirective)).to.equal(false);
+    });
+
+    it('returns false for object type named like specified scalar', () => {
+      const ObjectNamedLikeScalar = new GraphQLObjectType({
+        name: 'String',
+        fields: { serialize: { type: GraphQLString } },
+      });
+      expect(isSpecifiedScalarType(ObjectNamedLikeScalar)).to.equal(false);
+    });
+
+    it('returns false for random garbage', () => {
+      expect(isSpecifiedScalarType({ what: 'is this' })).to.equal(false);
     });
   });
 
@@ -593,30 +657,26 @@ describe('Type predicates', () => {
 
 describe('Directive predicates', () => {
   describe('isDirective', () => {
-    it('returns true for directives', () => {
-      const directive = new GraphQLDirective({
-        name: 'Foo',
-        locations: ['QUERY'],
-      });
-      expect(isDirective(directive)).to.equal(true);
-      expect(() => assertDirective(directive)).not.to.throw();
+    it('returns true for spec defined directive', () => {
       expect(isDirective(GraphQLSkipDirective)).to.equal(true);
       expect(() => assertDirective(GraphQLSkipDirective)).not.to.throw();
     });
 
+    it('returns true for custom directive', () => {
+      expect(isDirective(Directive)).to.equal(true);
+      expect(() => assertDirective(Directive)).not.to.throw();
+    });
+
     it('returns false for directive class (rather than instance)', () => {
       expect(isDirective(GraphQLDirective)).to.equal(false);
       expect(() => assertDirective(GraphQLDirective)).to.throw();
     });
 
-    it('returns false for object type', () => {
-      expect(isDirective(ObjectType)).to.equal(false);
-      expect(() => assertDirective(ObjectType)).to.throw();
-    });
-
-    it('returns false for scalar type', () => {
-      expect(isDirective(GraphQLString)).to.equal(false);
-      expect(() => assertDirective(GraphQLString)).to.throw();
+    it('returns false for non-directive', () => {
+      expect(isDirective(EnumType)).to.equal(false);
+      expect(() => assertDirective(EnumType)).to.throw();
+      expect(isDirective(ScalarType)).to.equal(false);
+      expect(() => assertDirective(ScalarType)).to.throw();
     });
 
     it('returns false for random garbage', () => {
@@ -632,30 +692,31 @@ describe('Directive predicates', () => {
     });
 
     it('returns false for custom directive', () => {
-      const directive = new GraphQLDirective({
-        name: 'Foo',
-        locations: ['QUERY'],
-      });
-      expect(isSpecifiedDirective(directive)).to.equal(false);
+      expect(isSpecifiedDirective(Directive)).to.equal(false);
     });
 
     it('returns false for directive class (rather than specified instance)', () => {
-      // $DisableFlowOnNegativeTest
       expect(isSpecifiedDirective(GraphQLDirective)).to.equal(false);
     });
 
-    it('returns false for object type', () => {
-      // $DisableFlowOnNegativeTest
-      expect(isSpecifiedDirective(ObjectType)).to.equal(false);
+    it('returns false for non-directive', () => {
+      expect(isSpecifiedDirective(EnumType)).to.equal(false);
+      expect(isSpecifiedDirective(ScalarType)).to.equal(false);
     });
 
     it('returns false for spec defined scalar type', () => {
-      // $DisableFlowOnNegativeTest
       expect(isSpecifiedDirective(GraphQLString)).to.equal(false);
     });
 
+    it('returns false for scalar type named like specified directive', () => {
+      const ScalarNamedLikeDirective = new GraphQLScalarType({
+        name: 'deprecated',
+        serialize: () => null,
+      });
+      expect(isSpecifiedDirective(ScalarNamedLikeDirective)).to.equal(false);
+    });
+
     it('returns false for random garbage', () => {
-      // $DisableFlowOnNegativeTest
       expect(isSpecifiedDirective({ what: 'is this' })).to.equal(false);
     });
   });

src/type/directives.js

@@ -187,10 +187,13 @@ export const specifiedDirectives: $ReadOnlyArray<*> = [
   GraphQLDeprecatedDirective,
 ];
 
-export function isSpecifiedDirective(
-  directive: GraphQLDirective,
-): boolean %checks {
-  return specifiedDirectives.some(
-    specifiedDirective => specifiedDirective.name === directive.name,
+export function isSpecifiedDirective(directive: mixed): boolean %checks {
+  return (
+    isDirective(directive) &&
+    // Would prefer to use specifiedDirectives.some(), however %checks needs
+    // a simple expression.
+    (directive.name === GraphQLIncludeDirective.name ||
+      directive.name === GraphQLSkipDirective.name ||
+      directive.name === GraphQLDeprecatedDirective.name)
   );
 }

src/type/scalars.js

@@ -10,7 +10,7 @@
 import isFinite from '../polyfills/isFinite';
 import isInteger from '../polyfills/isInteger';
 import inspect from '../jsutils/inspect';
-import { GraphQLScalarType, isNamedType } from './definition';
+import { GraphQLScalarType, isScalarType } from './definition';
 import { Kind } from '../language/kinds';
 
 // As per the GraphQL Spec, Integers are only treated as valid when a valid
@@ -255,7 +255,7 @@ export const specifiedScalarTypes: $ReadOnlyArray<*> = [
 
 export function isSpecifiedScalarType(type: mixed): boolean %checks {
   return (
-    isNamedType(type) &&
+    isScalarType(type) &&
     // Would prefer to use specifiedScalarTypes.some(), however %checks needs
     // a simple expression.
     (type.name === GraphQLString.name ||