enhancement: remove extra ticks (#3754)

Promise resolution order changes in some instances, resulting in different orders for some errors within the errors array, as well as in different values of hasNext within incremental delivery payloads. This PR introduces an async `completePromisedValue` helper function rather than using a promise chain (see below links). https://github.com/tc39/proposal-faster-promise-adoption tc39/ecma262#2770 tc39/ecma262#2772 tc39/ecma262#1250 https://v8.dev/blog/fast-async Depends on #3793
graphql · Dec 14, 2022 · 1564174 · 1564174
1 parent 7fd1ddb
commit 1564174
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 completePromisedValue(
+        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 completePromisedValue(
+  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(
+      completePromisedValue(
         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 = completePromisedValue(
+      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;