NaN values should be checked inside scalar's serialize method (#2438)

We shouldn't have special casing for NaN since it masks programmer mistakes.
Also it makes it imposible to implement custom scalar that can encode NaN.

Author: IvanGoncharov

src/execution/execute.js

@@ -6,8 +6,6 @@ import inspect from '../jsutils/inspect';
 import memoize3 from '../jsutils/memoize3';
 import invariant from '../jsutils/invariant';
 import devAssert from '../jsutils/devAssert';
-import isInvalid from '../jsutils/isInvalid';
-import isNullish from '../jsutils/isNullish';
 import isPromise from '../jsutils/isPromise';
 import { type ObjMap } from '../jsutils/ObjMap';
 import isObjectLike from '../jsutils/isObjectLike';
@@ -833,8 +831,8 @@ function completeValue(
     return completed;
   }
 
-  // If result value is null-ish (null, undefined, or NaN) then return null.
-  if (isNullish(result)) {
+  // If result value is null or undefined then return null.
+  if (result == null) {
     return null;
   }
 
@@ -940,7 +938,7 @@ function completeListValue(
  */
 function completeLeafValue(returnType: GraphQLLeafType, result: mixed): mixed {
   const serializedResult = returnType.serialize(result);
-  if (isInvalid(serializedResult)) {
+  if (serializedResult === undefined) {
     throw new Error(
       `Expected a value of type "${inspect(returnType)}" but ` +
         `received: ${inspect(result)}`,

src/jsutils/isInvalid.js

@@ -34,8 +34,6 @@ describe('astFromValue', () => {
 
     expect(astFromValue(undefined, GraphQLBoolean)).to.deep.equal(null);
 
-    expect(astFromValue(NaN, GraphQLInt)).to.deep.equal(null);
-
     expect(astFromValue(null, GraphQLBoolean)).to.deep.equal({
       kind: 'NullValue',
     });
@@ -83,6 +81,10 @@ describe('astFromValue', () => {
     expect(() => astFromValue(1e40, GraphQLInt)).to.throw(
       'Int cannot represent non 32-bit signed integer value: 1e+40',
     );
+
+    expect(() => astFromValue(NaN, GraphQLInt)).to.throw(
+      'Int cannot represent non-integer value: NaN',
+    );
   });
 
   it('converts Float values to Int/Float ASTs', () => {
@@ -205,7 +207,9 @@ describe('astFromValue', () => {
       value: 'value',
     });
 
-    expect(astFromValue(NaN, passthroughScalar)).to.equal(null);
+    expect(() => astFromValue(NaN, passthroughScalar)).to.throw(
+      'Cannot convert value to AST: NaN.',
+    );
     expect(() => astFromValue(Infinity, passthroughScalar)).to.throw(
       'Cannot convert value to AST: Infinity.',
     );

src/jsutils/isNullish.js

@@ -53,6 +53,8 @@ describe('valueFromASTUntyped', () => {
       { a: ['foo'] },
     );
     testCaseWithVars('$testVariable', { testVariable: null }, null);
+    testCaseWithVars('$testVariable', { testVariable: NaN }, NaN);
     testCaseWithVars('$testVariable', {}, undefined);
+    testCaseWithVars('$testVariable', null, undefined);
   });
 });

src/utilities/__tests__/astFromValue-test.js

@@ -6,8 +6,6 @@ import objectValues from '../polyfills/objectValues';
 
 import inspect from '../jsutils/inspect';
 import invariant from '../jsutils/invariant';
-import isNullish from '../jsutils/isNullish';
-import isInvalid from '../jsutils/isInvalid';
 import isObjectLike from '../jsutils/isObjectLike';
 import isCollection from '../jsutils/isCollection';
 
@@ -59,8 +57,8 @@ export function astFromValue(value: mixed, type: GraphQLInputType): ?ValueNode {
     return { kind: Kind.NULL };
   }
 
-  // undefined, NaN
-  if (isInvalid(value)) {
+  // undefined
+  if (value === undefined) {
     return null;
   }
 
@@ -107,7 +105,7 @@ export function astFromValue(value: mixed, type: GraphQLInputType): ?ValueNode {
     // Since value is an internally represented value, it must be serialized
     // to an externally represented value before converting into an AST.
     const serialized = type.serialize(value);
-    if (isNullish(serialized)) {
+    if (serialized == null) {
       return null;
     }
 

src/utilities/__tests__/valueFromASTUntyped-test.js

@@ -5,7 +5,6 @@ import objectValues from '../polyfills/objectValues';
 import keyMap from '../jsutils/keyMap';
 import inspect from '../jsutils/inspect';
 import invariant from '../jsutils/invariant';
-import isInvalid from '../jsutils/isInvalid';
 import { type ObjMap } from '../jsutils/ObjMap';
 
 import { Kind } from '../language/kinds';
@@ -52,7 +51,7 @@ export function valueFromAST(
 
   if (valueNode.kind === Kind.VARIABLE) {
     const variableName = valueNode.name.value;
-    if (!variables || isInvalid(variables[variableName])) {
+    if (variables == null || variables[variableName] === undefined) {
       // No valid return value.
       return;
     }
@@ -92,7 +91,7 @@ export function valueFromAST(
           coercedValues.push(null);
         } else {
           const itemValue = valueFromAST(itemNode, itemType, variables);
-          if (isInvalid(itemValue)) {
+          if (itemValue === undefined) {
             return; // Invalid: intentionally return no value.
           }
           coercedValues.push(itemValue);
@@ -101,7 +100,7 @@ export function valueFromAST(
       return coercedValues;
     }
     const coercedValue = valueFromAST(valueNode, itemType, variables);
-    if (isInvalid(coercedValue)) {
+    if (coercedValue === undefined) {
       return; // Invalid: intentionally return no value.
     }
     return [coercedValue];
@@ -124,7 +123,7 @@ export function valueFromAST(
         continue;
       }
       const fieldValue = valueFromAST(fieldNode.value, field.type, variables);
-      if (isInvalid(fieldValue)) {
+      if (fieldValue === undefined) {
         return; // Invalid: intentionally return no value.
       }
       coercedObj[field.name] = fieldValue;
@@ -157,6 +156,6 @@ export function valueFromAST(
 function isMissingVariable(valueNode, variables) {
   return (
     valueNode.kind === Kind.VARIABLE &&
-    (!variables || isInvalid(variables[valueNode.name.value]))
+    (variables == null || variables[valueNode.name.value] === undefined)
   );
 }

src/utilities/astFromValue.js

@@ -3,7 +3,6 @@
 import inspect from '../jsutils/inspect';
 import invariant from '../jsutils/invariant';
 import keyValMap from '../jsutils/keyValMap';
-import isInvalid from '../jsutils/isInvalid';
 import { type ObjMap } from '../jsutils/ObjMap';
 
 import { Kind } from '../language/kinds';
@@ -48,12 +47,8 @@ export function valueFromASTUntyped(
         field => field.name.value,
         field => valueFromASTUntyped(field.value, variables),
       );
-    case Kind.VARIABLE: {
-      const variableName = valueNode.name.value;
-      return variables && !isInvalid(variables[variableName])
-        ? variables[variableName]
-        : undefined;
-    }
+    case Kind.VARIABLE:
+      return variables?.[valueNode.name.value];
   }
 
   // Not reachable. All possible value nodes have been considered.

src/utilities/valueFromAST.js

@@ -4,7 +4,6 @@ import objectValues from '../../polyfills/objectValues';
 
 import keyMap from '../../jsutils/keyMap';
 import inspect from '../../jsutils/inspect';
-import isInvalid from '../../jsutils/isInvalid';
 import didYouMean from '../../jsutils/didYouMean';
 import suggestionList from '../../jsutils/suggestionList';
 
@@ -130,7 +129,7 @@ function isValidValueNode(context: ValidationContext, node: ValueNode): void {
   // may throw or return an invalid value to indicate failure.
   try {
     const parseResult = type.parseLiteral(node, undefined /* variables */);
-    if (isInvalid(parseResult)) {
+    if (parseResult === undefined) {
       const typeStr = inspect(locationType);
       context.reportError(
         new GraphQLError(