introduce completePromiseCatchingErrors helpers

use it within executeField, completeListItemValue, executeStreamField this takes advantage of some quirks involving promises/await
graphql · Dec 6, 2022 · bac9ad6 · bac9ad6
1 parent 500b458
commit bac9ad6
Show file tree

Hide file tree

Showing 3 changed files with 100 additions and 88 deletions.
diff --git 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
@@ -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
@@ -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 completePromiseCatchingErrors(
+        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 completePromiseCatchingErrors(
+  exeContext: ExecutionContext,
+  returnType: GraphQLOutputType,
+  fieldNodes: ReadonlyArray<FieldNode>,
+  info: GraphQLResolveInfo,
+  path: Path,
+  result: Promise<unknown>,
+  asyncPayloadRecord?: AsyncPayloadRecord,
+): Promise<unknown> {
+  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(
+      completePromiseCatchingErrors(
         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 = completePromiseCatchingErrors(
+      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;