From e7ebffaa26ed7b79285b2ccc644887d335f2df44 Mon Sep 17 00:00:00 2001 From: Ivan Goncharov Date: Fri, 22 Jan 2021 12:48:47 +0200 Subject: [PATCH] Drop support for non-iteratable ArrayLike objects --- src/execution/execute.js | 4 +- src/jsutils/__tests__/isCollection-test.js | 71 ------------------- .../__tests__/isIteratableObject-test.js | 71 +++++++++++++++++++ src/jsutils/isCollection.js | 39 ---------- src/jsutils/isIteratableObject.js | 27 +++++++ .../__tests__/coerceInputValue-test.js | 16 ++++- src/utilities/astFromValue.js | 4 +- src/utilities/coerceInputValue.js | 4 +- 8 files changed, 119 insertions(+), 117 deletions(-) delete mode 100644 src/jsutils/__tests__/isCollection-test.js create mode 100644 src/jsutils/__tests__/isIteratableObject-test.js delete mode 100644 src/jsutils/isCollection.js create mode 100644 src/jsutils/isIteratableObject.js diff --git a/src/execution/execute.js b/src/execution/execute.js index 66c9bdefd5..1447edfa4d 100644 --- a/src/execution/execute.js +++ b/src/execution/execute.js @@ -7,10 +7,10 @@ import { invariant } from '../jsutils/invariant'; import { devAssert } from '../jsutils/devAssert'; import { isPromise } from '../jsutils/isPromise'; import { isObjectLike } from '../jsutils/isObjectLike'; -import { isCollection } from '../jsutils/isCollection'; import { promiseReduce } from '../jsutils/promiseReduce'; import { promiseForObject } from '../jsutils/promiseForObject'; import { addPath, pathToArray } from '../jsutils/Path'; +import { isIteratableObject } from '../jsutils/isIteratableObject'; import type { GraphQLFormattedError } from '../error/formatError'; import { GraphQLError } from '../error/GraphQLError'; @@ -821,7 +821,7 @@ function completeListValue( path: Path, result: mixed, ): PromiseOrValue<$ReadOnlyArray> { - if (!isCollection(result)) { + if (!isIteratableObject(result)) { throw new GraphQLError( `Expected Iterable, but did not find one for field "${info.parentType.name}.${info.fieldName}".`, ); diff --git a/src/jsutils/__tests__/isCollection-test.js b/src/jsutils/__tests__/isCollection-test.js deleted file mode 100644 index 683e56805f..0000000000 --- a/src/jsutils/__tests__/isCollection-test.js +++ /dev/null @@ -1,71 +0,0 @@ -import { expect } from 'chai'; -import { describe, it } from 'mocha'; - -import { identityFunc } from '../identityFunc'; -import { isCollection } from '../isCollection'; - -describe('isCollection', () => { - it('should return `true` for collections', () => { - expect(isCollection([])).to.equal(true); - expect(isCollection(new Int8Array(1))).to.equal(true); - - // eslint-disable-next-line no-new-wrappers - expect(isCollection(new String('ABC'))).to.equal(true); - - function getArguments() { - return arguments; - } - expect(isCollection(getArguments())).to.equal(true); - - const arrayLike = {}; - arrayLike[0] = 'Alpha'; - arrayLike[1] = 'Bravo'; - arrayLike[2] = 'Charlie'; - arrayLike.length = 3; - - expect(isCollection(arrayLike)).to.equal(true); - - const iterator = { [Symbol.iterator]: identityFunc }; - expect(isCollection(iterator)).to.equal(true); - - // istanbul ignore next (Never called and use just as a placeholder) - function* generatorFunc() { - /* do nothing */ - } - expect(isCollection(generatorFunc())).to.equal(true); - - // But generator function itself is not iteratable - expect(isCollection(generatorFunc)).to.equal(false); - }); - - it('should return `false` for non-collections', () => { - expect(isCollection(null)).to.equal(false); - expect(isCollection(undefined)).to.equal(false); - - expect(isCollection('ABC')).to.equal(false); - expect(isCollection('0')).to.equal(false); - expect(isCollection('')).to.equal(false); - - expect(isCollection(1)).to.equal(false); - expect(isCollection(0)).to.equal(false); - expect(isCollection(NaN)).to.equal(false); - // eslint-disable-next-line no-new-wrappers - expect(isCollection(new Number(123))).to.equal(false); - - expect(isCollection(true)).to.equal(false); - expect(isCollection(false)).to.equal(false); - // eslint-disable-next-line no-new-wrappers - expect(isCollection(new Boolean(true))).to.equal(false); - - expect(isCollection({})).to.equal(false); - expect(isCollection({ iterable: true })).to.equal(false); - - const iteratorWithoutSymbol = { next: identityFunc }; - expect(isCollection(iteratorWithoutSymbol)).to.equal(false); - - const invalidIteratable = { - [Symbol.iterator]: { next: identityFunc }, - }; - expect(isCollection(invalidIteratable)).to.equal(false); - }); -}); diff --git a/src/jsutils/__tests__/isIteratableObject-test.js b/src/jsutils/__tests__/isIteratableObject-test.js new file mode 100644 index 0000000000..828e3ada56 --- /dev/null +++ b/src/jsutils/__tests__/isIteratableObject-test.js @@ -0,0 +1,71 @@ +import { expect } from 'chai'; +import { describe, it } from 'mocha'; + +import { identityFunc } from '../identityFunc'; +import { isIteratableObject } from '../isIteratableObject'; + +describe('isIteratableObject', () => { + it('should return `true` for collections', () => { + expect(isIteratableObject([])).to.equal(true); + expect(isIteratableObject(new Int8Array(1))).to.equal(true); + + // eslint-disable-next-line no-new-wrappers + expect(isIteratableObject(new String('ABC'))).to.equal(true); + + function getArguments() { + return arguments; + } + expect(isIteratableObject(getArguments())).to.equal(true); + + const iterator = { [Symbol.iterator]: identityFunc }; + expect(isIteratableObject(iterator)).to.equal(true); + + // istanbul ignore next (Never called and use just as a placeholder) + function* generatorFunc() { + /* do nothing */ + } + expect(isIteratableObject(generatorFunc())).to.equal(true); + + // But generator function itself is not iteratable + expect(isIteratableObject(generatorFunc)).to.equal(false); + }); + + it('should return `false` for non-collections', () => { + expect(isIteratableObject(null)).to.equal(false); + expect(isIteratableObject(undefined)).to.equal(false); + + expect(isIteratableObject('ABC')).to.equal(false); + expect(isIteratableObject('0')).to.equal(false); + expect(isIteratableObject('')).to.equal(false); + + expect(isIteratableObject(1)).to.equal(false); + expect(isIteratableObject(0)).to.equal(false); + expect(isIteratableObject(NaN)).to.equal(false); + // eslint-disable-next-line no-new-wrappers + expect(isIteratableObject(new Number(123))).to.equal(false); + + expect(isIteratableObject(true)).to.equal(false); + expect(isIteratableObject(false)).to.equal(false); + // eslint-disable-next-line no-new-wrappers + expect(isIteratableObject(new Boolean(true))).to.equal(false); + + expect(isIteratableObject({})).to.equal(false); + expect(isIteratableObject({ iterable: true })).to.equal(false); + + const iteratorWithoutSymbol = { next: identityFunc }; + expect(isIteratableObject(iteratorWithoutSymbol)).to.equal(false); + + const invalidIteratable = { + [Symbol.iterator]: { next: identityFunc }, + }; + expect(isIteratableObject(invalidIteratable)).to.equal(false); + + const arrayLike = {}; + arrayLike[0] = 'Alpha'; + arrayLike[1] = 'Bravo'; + arrayLike[2] = 'Charlie'; + arrayLike.length = 3; + + expect(isIteratableObject(arrayLike)).to.equal(false); + }); +}); diff --git a/src/jsutils/isCollection.js b/src/jsutils/isCollection.js deleted file mode 100644 index 99128f4fea..0000000000 --- a/src/jsutils/isCollection.js +++ /dev/null @@ -1,39 +0,0 @@ -/** - * Returns true if the provided object is an Object (i.e. not a string literal) - * and is either Iterable or Array-like. - * - * This may be used in place of [Array.isArray()][isArray] to determine if an - * object should be iterated-over. It always excludes string literals and - * includes Arrays (regardless of if it is Iterable). It also includes other - * Array-like objects such as NodeList, TypedArray, and Buffer. - * - * @example - * - * isCollection([ 1, 2, 3 ]) // true - * isCollection('ABC') // false - * isCollection({ length: 1, 0: 'Alpha' }) // true - * isCollection({ key: 'value' }) // false - * isCollection(new Map()) // true - * - * @param obj - * An Object value which might implement the Iterable or Array-like protocols. - * @return {boolean} true if Iterable or Array-like Object. - */ -declare function isCollection(value: mixed): boolean %checks(value instanceof - Iterable); - -// eslint-disable-next-line no-redeclare -export function isCollection(obj) { - if (obj == null || typeof obj !== 'object') { - return false; - } - - // Is Array like? - const length = obj.length; - if (typeof length === 'number' && length >= 0 && length % 1 === 0) { - return true; - } - - // Is Iterable? - return typeof obj[Symbol.iterator] === 'function'; -} diff --git a/src/jsutils/isIteratableObject.js b/src/jsutils/isIteratableObject.js new file mode 100644 index 0000000000..603da9f7ca --- /dev/null +++ b/src/jsutils/isIteratableObject.js @@ -0,0 +1,27 @@ +/** + * Returns true if the provided object is an Object (i.e. not a string literal) + * and implements the Iterator protocol. + * + * This may be used in place of [Array.isArray()][isArray] to determine if + * an object should be iterated-over e.g. Array, Map, Set, Int8Array, + * TypedArray, etc. but excludes string literals. + * + * @example + * + * isIteratableObject([ 1, 2, 3 ]) // true + * isIteratableObject(new Map()) // true + * isIteratableObject('ABC') // false + * isIteratableObject({ key: 'value' }) // false + * isIteratableObject({ length: 1, 0: 'Alpha' }) // false + */ +declare function isIteratableObject( + value: mixed, +): boolean %checks(value instanceof Iterable); + +// eslint-disable-next-line no-redeclare +export function isIteratableObject(maybeIteratable: mixed): boolean { + return ( + typeof maybeIteratable === 'object' && + typeof maybeIteratable?.[Symbol.iterator] === 'function' + ); +} diff --git a/src/utilities/__tests__/coerceInputValue-test.js b/src/utilities/__tests__/coerceInputValue-test.js index 07d1514200..61046b82d9 100644 --- a/src/utilities/__tests__/coerceInputValue-test.js +++ b/src/utilities/__tests__/coerceInputValue-test.js @@ -8,8 +8,8 @@ import { GraphQLInt } from '../../type/scalars'; import { GraphQLList, GraphQLNonNull, - GraphQLScalarType, GraphQLEnumType, + GraphQLScalarType, GraphQLInputObjectType, } from '../../type/definition'; @@ -335,6 +335,20 @@ describe('coerceInputValue', () => { expectValue(result).to.deep.equal([42]); }); + it('returns a list for a non-list object value', () => { + const TestListOfObjects = new GraphQLList( + new GraphQLInputObjectType({ + name: 'TestObject', + fields: { + length: { type: GraphQLInt }, + }, + }), + ); + + const result = coerceValue({ length: 100500 }, TestListOfObjects); + expectValue(result).to.deep.equal([{ length: 100500 }]); + }); + it('returns an error for a non-list invalid value', () => { const result = coerceValue('INVALID', TestList); expectErrors(result).to.deep.equal([ diff --git a/src/utilities/astFromValue.js b/src/utilities/astFromValue.js index e9bfbb2668..0c95bfef3f 100644 --- a/src/utilities/astFromValue.js +++ b/src/utilities/astFromValue.js @@ -3,7 +3,7 @@ import { objectValues } from '../polyfills/objectValues'; import { inspect } from '../jsutils/inspect'; import { invariant } from '../jsutils/invariant'; import { isObjectLike } from '../jsutils/isObjectLike'; -import { isCollection } from '../jsutils/isCollection'; +import { isIteratableObject } from '../jsutils/isIteratableObject'; import type { ValueNode } from '../language/ast'; import { Kind } from '../language/kinds'; @@ -62,7 +62,7 @@ export function astFromValue(value: mixed, type: GraphQLInputType): ?ValueNode { // the value is not an array, convert the value using the list's item type. if (isListType(type)) { const itemType = type.ofType; - if (isCollection(value)) { + if (isIteratableObject(value)) { const valuesNodes = []; // Since we transpile for-of in loose mode it doesn't support iterators // and it's required to first convert iteratable into array diff --git a/src/utilities/coerceInputValue.js b/src/utilities/coerceInputValue.js index 37fa1b475a..fb2a598ab9 100644 --- a/src/utilities/coerceInputValue.js +++ b/src/utilities/coerceInputValue.js @@ -5,10 +5,10 @@ import { inspect } from '../jsutils/inspect'; import { invariant } from '../jsutils/invariant'; import { didYouMean } from '../jsutils/didYouMean'; import { isObjectLike } from '../jsutils/isObjectLike'; -import { isCollection } from '../jsutils/isCollection'; import { suggestionList } from '../jsutils/suggestionList'; import { printPathArray } from '../jsutils/printPathArray'; import { addPath, pathToArray } from '../jsutils/Path'; +import { isIteratableObject } from '../jsutils/isIteratableObject'; import { GraphQLError } from '../error/GraphQLError'; @@ -77,7 +77,7 @@ function coerceInputValueImpl( if (isListType(type)) { const itemType = type.ofType; - if (isCollection(inputValue)) { + if (isIteratableObject(inputValue)) { return Array.from(inputValue, (itemValue, index) => { const itemPath = addPath(path, index, undefined); return coerceInputValueImpl(itemValue, itemType, onError, itemPath);