diff --git a/.eslintrc.yml b/.eslintrc.yml index 4049462418..a75213e82d 100644 --- a/.eslintrc.yml +++ b/.eslintrc.yml @@ -9,6 +9,7 @@ plugins: - node - import - simple-import-sort + - promise settings: node: tryExtensions: ['.js', '.ts', '.jsx', '.json', '.node', '.d.ts'] @@ -177,6 +178,27 @@ rules: - ["^\\./"] simple-import-sort/exports: off # TODO + ############################################################################## + # `eslint-plugin-promise` rule list based on `v6.1.x` + # https://github.com/eslint-community/eslint-plugin-promise/blob/main/README.md + ############################################################################## + + promise/always-return: error + promise/avoid-new: off + promise/catch-or-return: error + promise/no-callback-in-promise: error + promise/no-multiple-resolved: error + promise/no-native: off + promise/no-nesting: error + promise/no-new-statics: error + promise/no-promise-in-callback: off + promise/no-return-in-finally: error + promise/no-return-wrap: error + promise/param-names: error + promise/prefer-await-to-callbacks: off + promise/prefer-await-to-then: error + promise/valid-params: error + ############################################################################## # ESLint builtin rules list based on `v8.27.x` ############################################################################## @@ -629,7 +651,7 @@ overrides: ] '@typescript-eslint/no-useless-constructor': error '@typescript-eslint/require-await': error - '@typescript-eslint/return-await': error + '@typescript-eslint/return-await': [error, 'always'] # Disable for JS and TS '@typescript-eslint/init-declarations': off diff --git a/package-lock.json b/package-lock.json index 1f295999b7..f27b15c14f 100644 --- a/package-lock.json +++ b/package-lock.json @@ -31,6 +31,7 @@ "eslint-plugin-import": "2.26.0", "eslint-plugin-internal-rules": "file:./resources/eslint-internal-rules", "eslint-plugin-node": "11.1.0", + "eslint-plugin-promise": "6.1.1", "eslint-plugin-react": "7.31.10", "eslint-plugin-react-hooks": "4.6.0", "eslint-plugin-simple-import-sort": "8.0.0", @@ -8208,6 +8209,18 @@ "semver": "bin/semver.js" } }, + "node_modules/eslint-plugin-promise": { + "version": "6.1.1", + "resolved": "https://registry.npmjs.org/eslint-plugin-promise/-/eslint-plugin-promise-6.1.1.tgz", + "integrity": "sha512-tjqWDwVZQo7UIPMeDReOpUgHCmCiH+ePnVT+5zVapL0uuHnegBUs2smM13CzOs2Xb5+MHMRFTs9v24yjba4Oig==", + "dev": true, + "engines": { + "node": "^12.22.0 || ^14.17.0 || >=16.0.0" + }, + "peerDependencies": { + "eslint": "^7.0.0 || ^8.0.0" + } + }, "node_modules/eslint-plugin-react": { "version": "7.31.10", "resolved": "https://registry.npmjs.org/eslint-plugin-react/-/eslint-plugin-react-7.31.10.tgz", @@ -24233,6 +24246,13 @@ } } }, + "eslint-plugin-promise": { + "version": "6.1.1", + "resolved": "https://registry.npmjs.org/eslint-plugin-promise/-/eslint-plugin-promise-6.1.1.tgz", + "integrity": "sha512-tjqWDwVZQo7UIPMeDReOpUgHCmCiH+ePnVT+5zVapL0uuHnegBUs2smM13CzOs2Xb5+MHMRFTs9v24yjba4Oig==", + "dev": true, + "requires": {} + }, "eslint-plugin-react": { "version": "7.31.10", "resolved": "https://registry.npmjs.org/eslint-plugin-react/-/eslint-plugin-react-7.31.10.tgz", diff --git a/package.json b/package.json index 88c2668dca..e717b4df1a 100644 --- a/package.json +++ b/package.json @@ -77,6 +77,7 @@ "eslint-plugin-import": "2.26.0", "eslint-plugin-internal-rules": "file:./resources/eslint-internal-rules", "eslint-plugin-node": "11.1.0", + "eslint-plugin-promise": "6.1.1", "eslint-plugin-react": "7.31.10", "eslint-plugin-react-hooks": "4.6.0", "eslint-plugin-simple-import-sort": "8.0.0", diff --git a/resources/gen-changelog.ts b/resources/gen-changelog.ts index 9d106fbacc..309ba35675 100644 --- a/resources/gen-changelog.ts +++ b/resources/gen-changelog.ts @@ -294,7 +294,7 @@ async function getPRsInfo( let prNumbers = await splitBatches(commits, batchCommitToPR); prNumbers = Array.from(new Set(prNumbers)); // Remove duplicates - return splitBatches(prNumbers, batchPRInfo); + return await splitBatches(prNumbers, batchPRInfo); } // Split commits into batches of 50 to prevent timeouts diff --git a/src/__testUtils__/__tests__/resolveOnNextTick-test.ts b/src/__testUtils__/__tests__/resolveOnNextTick-test.ts index 96e618b2db..9ee798e961 100644 --- a/src/__testUtils__/__tests__/resolveOnNextTick-test.ts +++ b/src/__testUtils__/__tests__/resolveOnNextTick-test.ts @@ -7,12 +7,13 @@ describe('resolveOnNextTick', () => { it('resolves promise on the next tick', async () => { const output = []; - const promise1 = resolveOnNextTick().then(() => { - output.push('second'); - }); - const promise2 = resolveOnNextTick().then(() => { - output.push('third'); - }); + async function outputOnNextTick(message: string) { + await resolveOnNextTick(); + output.push(message); + } + + const promise1 = outputOnNextTick('second'); + const promise2 = outputOnNextTick('third'); output.push('first'); await Promise.all([promise1, promise2]); diff --git a/src/__testUtils__/expectEqualPromisesOrValues.ts b/src/__testUtils__/expectEqualPromisesOrValues.ts index 6911fb32c0..468a058e0a 100644 --- a/src/__testUtils__/expectEqualPromisesOrValues.ts +++ b/src/__testUtils__/expectEqualPromisesOrValues.ts @@ -5,13 +5,18 @@ import type { PromiseOrValue } from '../jsutils/PromiseOrValue.js'; import { expectMatchingValues } from './expectMatchingValues.js'; +async function expectMatchingPromises(items: Promise>) { + const values = await items; + return expectMatchingValues(values); +} + export function expectEqualPromisesOrValues( items: ReadonlyArray>, ): PromiseOrValue { const [firstItem, ...remainingItems] = items; if (isPromise(firstItem)) { if (remainingItems.every(isPromise)) { - return Promise.all(items).then(expectMatchingValues); + return expectMatchingPromises(Promise.all(items)); } } else if (remainingItems.every((item) => !isPromise(item))) { return expectMatchingValues(items); diff --git a/src/execution/__tests__/executor-test.ts b/src/execution/__tests__/executor-test.ts index a945b4d0b5..3642b7dfb4 100644 --- a/src/execution/__tests__/executor-test.ts +++ b/src/execution/__tests__/executor-test.ts @@ -419,7 +419,7 @@ describe('Execute: Handles basic execution tasks', () => { return new Promise((resolve) => resolve('async')); }, asyncReject() { - return new Promise((_, reject) => + return new Promise((_resolve, reject) => reject(new Error('Error getting asyncReject')), ); }, diff --git a/src/execution/__tests__/simplePubSub-test.ts b/src/execution/__tests__/simplePubSub-test.ts index 48d45afbc0..82209dd94f 100644 --- a/src/execution/__tests__/simplePubSub-test.ts +++ b/src/execution/__tests__/simplePubSub-test.ts @@ -22,9 +22,13 @@ describe('SimplePubSub', () => { value: 'Banana', }); + async function getNextItem(i: AsyncIterator) { + return await i.next(); + } + // Read ahead - const i3 = iterator.next().then((x) => x); - const i4 = iterator.next().then((x) => x); + const i3 = getNextItem(iterator); + const i4 = getNextItem(iterator); // Publish expect(pubsub.emit('Coconut')).to.equal(true); @@ -35,7 +39,7 @@ describe('SimplePubSub', () => { expect(await i3).to.deep.equal({ done: false, value: 'Coconut' }); // Read ahead - const i5 = iterator.next().then((x) => x); + const i5 = getNextItem(iterator); // Terminate queue await iterator.return(); diff --git a/src/execution/__tests__/stream-test.ts b/src/execution/__tests__/stream-test.ts index aed5211ae1..b6ad81278d 100644 --- a/src/execution/__tests__/stream-test.ts +++ b/src/execution/__tests__/stream-test.ts @@ -126,7 +126,7 @@ async function completeAsync( for (let i = 0; i < numCalls; i++) { promises.push(iterator.next()); } - return Promise.all(promises); + return await Promise.all(promises); } function createResolvablePromise(): [Promise, (value?: T) => void] { @@ -531,11 +531,6 @@ describe('Execute: stream directive', () => { }, ], }, - ], - hasNext: true, - }, - { - incremental: [ { items: [{ name: 'Leia', id: '3' }], path: ['friendList', 2], @@ -984,11 +979,6 @@ describe('Execute: stream directive', () => { }, ], }, - ], - hasNext: true, - }, - { - incremental: [ { items: [{ nonNullName: 'Han' }], path: ['friendList', 2], diff --git a/src/execution/execute.ts b/src/execution/execute.ts index a7ff5ce607..e8512626bb 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -1,3 +1,4 @@ +import { after } from '../jsutils/after.js'; import { inspect } from '../jsutils/inspect.js'; import { invariant } from '../jsutils/invariant.js'; import { isAsyncIterable } from '../jsutils/isAsyncIterable.js'; @@ -291,7 +292,7 @@ export function execute(args: ExecutionArgs): PromiseOrValue { return result; } - return result.then((incrementalResult) => { + return after(result, (incrementalResult) => { if ('initialResult' in incrementalResult) { return { errors: [new GraphQLError(UNEXPECTED_MULTIPLE_PAYLOADS)], @@ -345,7 +346,8 @@ function executeImpl( try { const result = executeOperation(exeContext); if (isPromise(result)) { - return result.then( + return after( + result, (data) => { const initialResult = buildResponse(data, exeContext.errors); if (exeContext.subsequentPayloads.size > 0) { @@ -597,7 +599,7 @@ function executeFieldsSerially( return results; } if (isPromise(result)) { - return result.then((resolvedResult) => { + return after(result, (resolvedResult) => { results[responseName] = resolvedResult; return results; }); @@ -645,10 +647,15 @@ function executeFields( } } catch (error) { if (containsPromise) { - // Ensure that any promises returned by other fields are handled, as they may also reject. - return promiseForObject(results).finally(() => { - throw error; - }); + return after( + promiseForObject(results), + () => { + throw error; + }, + () => { + throw error; + }, + ); } throw error; } @@ -737,9 +744,7 @@ function executeField( ); if (isPromise(completed)) { - // Note: we don't rely on a `catch` method, but we do expect "thenable" - // to take a second callback for the error case. - return completed.then(undefined, (rawError) => { + return after(completed, undefined, (rawError) => { const error = locatedError(rawError, fieldNodes, pathToArray(path)); const handledError = handleFieldError(error, returnType, errors); filterSubsequentPayloads(exeContext, path, asyncPayloadRecord); @@ -1070,7 +1075,9 @@ async function completeAsyncIteratorValue( } index += 1; } - return containsPromise ? Promise.all(completedResults) : completedResults; + return containsPromise + ? await Promise.all(completedResults) + : completedResults; } /** @@ -1211,7 +1218,7 @@ function completeListItemValue( // Note: we don't rely on a `catch` method, but we do expect "thenable" // to take a second callback for the error case. completedResults.push( - completedItem.then(undefined, (rawError) => { + after(completedItem, undefined, (rawError) => { const error = locatedError( rawError, fieldNodes, @@ -1273,7 +1280,7 @@ function completeAbstractValue( const runtimeType = resolveTypeFn(result, contextValue, info, returnType); if (isPromise(runtimeType)) { - return runtimeType.then((resolvedRuntimeType) => + return after(runtimeType, (resolvedRuntimeType) => completeObjectValue( exeContext, ensureValidRuntimeType( @@ -1385,7 +1392,7 @@ function completeObjectValue( const isTypeOf = returnType.isTypeOf(result, exeContext.contextValue, info); if (isPromise(isTypeOf)) { - return isTypeOf.then((resolvedIsTypeOf) => { + return after(isTypeOf, (resolvedIsTypeOf) => { if (!resolvedIsTypeOf) { throw invalidReturnTypeError(returnType, result, fieldNodes); } @@ -1502,7 +1509,7 @@ export const defaultTypeResolver: GraphQLTypeResolver = } if (promisedIsTypeOfResults.length) { - return Promise.all(promisedIsTypeOfResults).then((isTypeOfResults) => { + return after(Promise.all(promisedIsTypeOfResults), (isTypeOfResults) => { for (let i = 0; i < isTypeOfResults.length; i++) { if (isTypeOfResults[i]) { return possibleTypes[i].name; @@ -1566,7 +1573,7 @@ export function subscribe( > { const maybePromise = experimentalSubscribeIncrementally(args); if (isPromise(maybePromise)) { - return maybePromise.then((resultOrIterable) => + return after(maybePromise, (resultOrIterable) => isAsyncIterable(resultOrIterable) ? mapAsyncIterable(resultOrIterable, ensureSingleExecutionResult) : resultOrIterable, @@ -1648,7 +1655,7 @@ export function experimentalSubscribeIncrementally( const resultOrStream = createSourceEventStreamImpl(exeContext); if (isPromise(resultOrStream)) { - return resultOrStream.then((resolvedResultOrStream) => + return after(resultOrStream, (resolvedResultOrStream) => mapSourceToResponse(exeContext, resolvedResultOrStream), ); } @@ -1755,7 +1762,7 @@ function createSourceEventStreamImpl( try { const eventStream = executeSubscription(exeContext); if (isPromise(eventStream)) { - return eventStream.then(undefined, (error) => ({ errors: [error] })); + return after(eventStream, undefined, (error) => ({ errors: [error] })); } return eventStream; @@ -1826,7 +1833,7 @@ function executeSubscription( const result = resolveFn(rootValue, args, contextValue, info); if (isPromise(result)) { - return result.then(assertEventStream).then(undefined, (error) => { + return after(result, assertEventStream, (error) => { throw locatedError(error, fieldNodes, pathToArray(path)); }); } @@ -1880,13 +1887,13 @@ function executeDeferredFragment( ); if (isPromise(promiseOrData)) { - promiseOrData = promiseOrData.then(null, (e) => { - asyncPayloadRecord.errors.push(e); + promiseOrData = after(promiseOrData, undefined, (error) => { + asyncPayloadRecord.errors.push(error); return null; }); } - } catch (e) { - asyncPayloadRecord.errors.push(e); + } catch (error) { + asyncPayloadRecord.errors.push(error); promiseOrData = null; } asyncPayloadRecord.addData(promiseOrData); @@ -1910,7 +1917,7 @@ function executeStreamField( exeContext, }); if (isPromise(item)) { - const completedItems = completePromisedValue( + const completedItem = completePromisedValue( exeContext, itemType, fieldNodes, @@ -1918,8 +1925,10 @@ function executeStreamField( itemPath, item, asyncPayloadRecord, - ).then( - (value) => [value], + ); + const completedItems = after( + completedItem, + (resolved) => [resolved], (error) => { asyncPayloadRecord.errors.push(error); filterSubsequentPayloads(exeContext, path, asyncPayloadRecord); @@ -1960,25 +1969,30 @@ function executeStreamField( } if (isPromise(completedItem)) { - const completedItems = completedItem - .then(undefined, (rawError) => { - const error = locatedError(rawError, fieldNodes, pathToArray(itemPath)); - const handledError = handleFieldError( - error, - itemType, - asyncPayloadRecord.errors, - ); - filterSubsequentPayloads(exeContext, itemPath, asyncPayloadRecord); - return handledError; - }) - .then( - (value) => [value], - (error) => { + const completedItems = after( + completedItem, + (resolved) => [resolved], + (rawError) => { + try { + const error = locatedError( + rawError, + fieldNodes, + pathToArray(itemPath), + ); + const handledError = handleFieldError( + error, + itemType, + asyncPayloadRecord.errors, + ); + filterSubsequentPayloads(exeContext, itemPath, asyncPayloadRecord); + return [handledError]; + } catch (error) { asyncPayloadRecord.errors.push(error); filterSubsequentPayloads(exeContext, path, asyncPayloadRecord); return null; - }, - ); + } + }, + ); asyncPayloadRecord.addItems(completedItems); return asyncPayloadRecord; @@ -2024,7 +2038,7 @@ async function executeStreamIteratorItem( ); if (isPromise(completedItem)) { - completedItem = completedItem.then(undefined, (rawError) => { + completedItem = after(completedItem, undefined, (rawError) => { const error = locatedError(rawError, fieldNodes, pathToArray(itemPath)); const handledError = handleFieldError( error, @@ -2086,6 +2100,7 @@ async function executeStreamIterator( asyncPayloadRecord.addItems(null); // entire stream has errored and bubbled upwards if (iterator?.return) { + // eslint-disable-next-line promise/prefer-await-to-then iterator.return().catch(() => { // ignore errors }); @@ -2097,8 +2112,9 @@ async function executeStreamIterator( let completedItems: PromiseOrValue | null>; if (isPromise(completedItem)) { - completedItems = completedItem.then( - (value) => [value], + completedItems = after( + completedItem, + (resolved) => [resolved], (error) => { asyncPayloadRecord.errors.push(error); filterSubsequentPayloads(exeContext, path, asyncPayloadRecord); @@ -2138,6 +2154,7 @@ function filterSubsequentPayloads( } // asyncRecord path points to nulled error field if (isStreamPayload(asyncRecord) && asyncRecord.iterator?.return) { + // eslint-disable-next-line promise/prefer-await-to-then asyncRecord.iterator.return().catch(() => { // ignore error }); @@ -2205,7 +2222,7 @@ function yieldSubsequentPayloads( const hasNext = exeContext.subsequentPayloads.size > 0; if (!incremental.length && hasNext) { - return next(); + return await next(); } if (!hasNext) { @@ -2248,7 +2265,7 @@ function yieldSubsequentPayloads( ): Promise> { await returnStreamIterators(); isDone = true; - return Promise.reject(error); + return await Promise.reject(error); }, }; } @@ -2279,20 +2296,23 @@ class DeferredFragmentRecord { this._exeContext.subsequentPayloads.add(this); this.isCompleted = false; this.data = null; - this.promise = new Promise | null>((resolve) => { - this._resolve = (promiseOrValue) => { - resolve(promiseOrValue); - }; - }).then((data) => { - this.data = data; - this.isCompleted = true; - }); + this.promise = after( + new Promise | null>((resolve) => { + this._resolve = (promiseOrValue) => { + resolve(promiseOrValue); + }; + }), + (data) => { + this.data = data; + this.isCompleted = true; + }, + ); } addData(data: PromiseOrValue | null>) { const parentData = this.parentContext?.promise; if (parentData) { - this._resolve?.(parentData.then(() => data)); + this._resolve?.(after(parentData, () => data)); return; } this._resolve?.(data); @@ -2330,20 +2350,23 @@ class StreamRecord { this._exeContext.subsequentPayloads.add(this); this.isCompleted = false; this.items = null; - this.promise = new Promise | null>((resolve) => { - this._resolve = (promiseOrValue) => { - resolve(promiseOrValue); - }; - }).then((items) => { - this.items = items; - this.isCompleted = true; - }); + this.promise = after( + new Promise | null>((resolve) => { + this._resolve = (promiseOrValue) => { + resolve(promiseOrValue); + }; + }), + (items) => { + this.items = items; + this.isCompleted = true; + }, + ); } addItems(items: PromiseOrValue | null>) { const parentData = this.parentContext?.promise; if (parentData) { - this._resolve?.(parentData.then(() => items)); + this._resolve?.(after(parentData, () => items)); return; } this._resolve?.(items); diff --git a/src/execution/flattenAsyncIterable.ts b/src/execution/flattenAsyncIterable.ts index 22bdb02338..456eff2a8a 100644 --- a/src/execution/flattenAsyncIterable.ts +++ b/src/execution/flattenAsyncIterable.ts @@ -38,8 +38,8 @@ export function flattenAsyncIterable( } // Nobody else is getting it. We should! let resolve: () => void; - waitForCurrentNestedIterator = new Promise((r) => { - resolve = r; + waitForCurrentNestedIterator = new Promise((_resolve) => { + resolve = _resolve; }); const topIteratorResult = await topIterator.next(); if (topIteratorResult.done) { diff --git a/src/execution/mapAsyncIterable.ts b/src/execution/mapAsyncIterable.ts index 0f6fd78c2d..a60d1c829b 100644 --- a/src/execution/mapAsyncIterable.ts +++ b/src/execution/mapAsyncIterable.ts @@ -36,17 +36,17 @@ export function mapAsyncIterable( return { async next() { - return mapResult(await iterator.next()); + return await mapResult(await iterator.next()); }, async return(): Promise> { // If iterator.return() does not exist, then type R must be undefined. return typeof iterator.return === 'function' - ? mapResult(await iterator.return()) + ? await mapResult(await iterator.return()) : { value: undefined as any, done: true }; }, async throw(error?: unknown) { if (typeof iterator.throw === 'function') { - return mapResult(await iterator.throw(error)); + return await mapResult(await iterator.throw(error)); } throw error; }, diff --git a/src/jsutils/after.ts b/src/jsutils/after.ts new file mode 100644 index 0000000000..6216d87be6 --- /dev/null +++ b/src/jsutils/after.ts @@ -0,0 +1,35 @@ +import { isPromise } from './isPromise.js'; +import type { PromiseOrValue } from './PromiseOrValue.js'; + +/** + * Async Helper Function that avoids `.then()` + * + * It is faster to await a promise prior to returning it from an async function + * than to return a promise with `.then()`. + * + * see: https://github.com/tc39/proposal-faster-promise-adoption + */ +export async function after( + promise: Promise, + onFulfilled?: (value: T) => PromiseOrValue, + onError?: (error: any) => U, +): Promise { + try { + if (onFulfilled === undefined) { + return (await promise) as R; + } + + const result = onFulfilled(await promise); + + if (isPromise(result)) { + return await result; + } + + return result; + } catch (error) { + if (onError === undefined) { + throw error; + } + return onError(error); + } +} diff --git a/src/jsutils/promiseReduce.ts b/src/jsutils/promiseReduce.ts index 871b9c3cf7..6838fff3bc 100644 --- a/src/jsutils/promiseReduce.ts +++ b/src/jsutils/promiseReduce.ts @@ -1,3 +1,4 @@ +import { after } from './after.js'; import { isPromise } from './isPromise.js'; import type { PromiseOrValue } from './PromiseOrValue.js'; @@ -16,7 +17,7 @@ export function promiseReduce( let accumulator = initialValue; for (const value of values) { accumulator = isPromise(accumulator) - ? accumulator.then((resolved) => callbackFn(resolved, value)) + ? after(accumulator, (resolved) => callbackFn(resolved, value)) : callbackFn(accumulator, value); } return accumulator;