From c81f866604de77d7dc936519dcf6d20cbe14c5e6 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Mon, 5 Dec 2022 22:37:17 +0200 Subject: [PATCH 1/2] introduce completePromise helper use it within executeField, completeListItemValue, executeStreamField this takes advantage of some quirks involving promises/await --- src/execution/__tests__/nonnull-test.ts | 30 +++--- src/execution/__tests__/stream-test.ts | 34 ++++--- src/execution/execute.ts | 124 ++++++++++++------------ 3 files changed, 100 insertions(+), 88 deletions(-) diff --git a/src/execution/__tests__/nonnull-test.ts b/src/execution/__tests__/nonnull-test.ts index a136a2a4f0..d0b1b614b3 100644 --- a/src/execution/__tests__/nonnull-test.ts +++ b/src/execution/__tests__/nonnull-test.ts @@ -259,6 +259,16 @@ describe('Execute: handles non-nullable types', () => { path: ['syncNest', 'syncNest', 'sync'], locations: [{ line: 6, column: 22 }], }, + { + message: promiseError.message, + path: ['syncNest', 'promise'], + locations: [{ line: 5, column: 11 }], + }, + { + message: promiseError.message, + path: ['syncNest', 'syncNest', 'promise'], + locations: [{ line: 6, column: 27 }], + }, { message: syncError.message, path: ['syncNest', 'promiseNest', 'sync'], @@ -274,21 +284,6 @@ describe('Execute: handles non-nullable types', () => { path: ['promiseNest', 'syncNest', 'sync'], locations: [{ line: 12, column: 22 }], }, - { - message: promiseError.message, - path: ['syncNest', 'promise'], - locations: [{ line: 5, column: 11 }], - }, - { - message: promiseError.message, - path: ['syncNest', 'syncNest', 'promise'], - locations: [{ line: 6, column: 27 }], - }, - { - message: syncError.message, - path: ['promiseNest', 'promiseNest', 'sync'], - locations: [{ line: 13, column: 25 }], - }, { message: promiseError.message, path: ['syncNest', 'promiseNest', 'promise'], @@ -304,6 +299,11 @@ describe('Execute: handles non-nullable types', () => { path: ['promiseNest', 'syncNest', 'promise'], locations: [{ line: 12, column: 27 }], }, + { + message: syncError.message, + path: ['promiseNest', 'promiseNest', 'sync'], + locations: [{ line: 13, column: 25 }], + }, { message: promiseError.message, path: ['promiseNest', 'promiseNest', 'promise'], diff --git a/src/execution/__tests__/stream-test.ts b/src/execution/__tests__/stream-test.ts index 3a4f6ec1f4..aed5211ae1 100644 --- a/src/execution/__tests__/stream-test.ts +++ b/src/execution/__tests__/stream-test.ts @@ -1174,6 +1174,9 @@ describe('Execute: stream directive', () => { ], }, ], + hasNext: true, + }, + { hasNext: false, }, ]); @@ -1197,19 +1200,25 @@ describe('Execute: stream directive', () => { } /* c8 ignore stop */, }, }); - expectJSON(result).toDeepEqual({ - errors: [ - { - message: - 'Cannot return null for non-nullable field NestedObject.nonNullScalarField.', - locations: [{ line: 4, column: 11 }], - path: ['nestedObject', 'nonNullScalarField'], + expectJSON(result).toDeepEqual([ + { + errors: [ + { + message: + 'Cannot return null for non-nullable field NestedObject.nonNullScalarField.', + locations: [{ line: 4, column: 11 }], + path: ['nestedObject', 'nonNullScalarField'], + }, + ], + data: { + nestedObject: null, }, - ], - data: { - nestedObject: null, + hasNext: true, }, - }); + { + hasNext: false, + }, + ]); }); it('Filters payloads that are nulled by a later synchronous error', async () => { const document = parse(` @@ -1350,6 +1359,9 @@ describe('Execute: stream directive', () => { ], }, ], + hasNext: true, + }, + { hasNext: false, }, ]); diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 655ed3b3fd..3415fc90de 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -715,25 +715,15 @@ function executeField( const result = resolveFn(source, args, contextValue, info); if (isPromise(result)) { - const completed = result.then((resolved) => - completeValue( - exeContext, - returnType, - fieldNodes, - info, - path, - resolved, - asyncPayloadRecord, - ), + return completePromise( + exeContext, + returnType, + fieldNodes, + info, + path, + result, + asyncPayloadRecord, ); - // 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) => { - const error = locatedError(rawError, fieldNodes, pathToArray(path)); - const handledError = handleFieldError(error, returnType, errors); - filterSubsequentPayloads(exeContext, path, asyncPayloadRecord); - return handledError; - }); } const completed = completeValue( @@ -922,6 +912,41 @@ function completeValue( ); } +async function completePromise( + exeContext: ExecutionContext, + returnType: GraphQLOutputType, + fieldNodes: ReadonlyArray, + info: GraphQLResolveInfo, + path: Path, + result: Promise, + asyncPayloadRecord?: AsyncPayloadRecord, +): Promise { + try { + const resolved = await result; + let completed = completeValue( + exeContext, + returnType, + fieldNodes, + info, + path, + resolved, + asyncPayloadRecord, + ); + if (isPromise(completed)) { + // see: https://github.com/tc39/proposal-faster-promise-adoption + // it is faster to await a promise prior to returning it from an async function + completed = await completed; + } + return completed; + } catch (rawError) { + const errors = asyncPayloadRecord?.errors ?? exeContext.errors; + const error = locatedError(rawError, fieldNodes, pathToArray(path)); + const handledError = handleFieldError(error, returnType, errors); + filterSubsequentPayloads(exeContext, path, asyncPayloadRecord); + return handledError; + } +} + /** * Returns an object containing the `@stream` arguments if a field should be * streamed based on the experimental flag, stream directive present and @@ -1156,29 +1181,18 @@ function completeListItemValue( asyncPayloadRecord?: AsyncPayloadRecord, ): boolean { if (isPromise(item)) { - const completedItem = item.then((resolved) => - completeValue( + completedResults.push( + completePromise( exeContext, itemType, fieldNodes, info, itemPath, - resolved, + item, asyncPayloadRecord, ), ); - // 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) => { - const error = locatedError(rawError, fieldNodes, pathToArray(itemPath)); - const handledError = handleFieldError(error, itemType, errors); - filterSubsequentPayloads(exeContext, itemPath, asyncPayloadRecord); - return handledError; - }), - ); - return true; } @@ -1897,36 +1911,22 @@ function executeStreamField( exeContext, }); if (isPromise(item)) { - const completedItems = item - .then((resolved) => - completeValue( - exeContext, - itemType, - fieldNodes, - info, - itemPath, - resolved, - asyncPayloadRecord, - ), - ) - .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) => { - asyncPayloadRecord.errors.push(error); - filterSubsequentPayloads(exeContext, path, asyncPayloadRecord); - return null; - }, - ); + const completedItems = completePromise( + exeContext, + itemType, + fieldNodes, + info, + itemPath, + item, + asyncPayloadRecord, + ).then( + (value) => [value], + (error) => { + asyncPayloadRecord.errors.push(error); + filterSubsequentPayloads(exeContext, path, asyncPayloadRecord); + return null; + }, + ); asyncPayloadRecord.addItems(completedItems); return asyncPayloadRecord; From b19a684ae7786db5004c346be0490b32d7aaf1c8 Mon Sep 17 00:00:00 2001 From: Yaacov Rydzinski Date: Wed, 14 Dec 2022 22:48:14 +0200 Subject: [PATCH 2/2] rename completePromise to completePromisedValue ...integrating review feedback --- src/execution/execute.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/execution/execute.ts b/src/execution/execute.ts index 3415fc90de..ebb9116b23 100644 --- a/src/execution/execute.ts +++ b/src/execution/execute.ts @@ -715,7 +715,7 @@ function executeField( const result = resolveFn(source, args, contextValue, info); if (isPromise(result)) { - return completePromise( + return completePromisedValue( exeContext, returnType, fieldNodes, @@ -912,7 +912,7 @@ function completeValue( ); } -async function completePromise( +async function completePromisedValue( exeContext: ExecutionContext, returnType: GraphQLOutputType, fieldNodes: ReadonlyArray, @@ -1182,7 +1182,7 @@ function completeListItemValue( ): boolean { if (isPromise(item)) { completedResults.push( - completePromise( + completePromisedValue( exeContext, itemType, fieldNodes, @@ -1911,7 +1911,7 @@ function executeStreamField( exeContext, }); if (isPromise(item)) { - const completedItems = completePromise( + const completedItems = completePromisedValue( exeContext, itemType, fieldNodes,