Repository: graphql/graphql-js
Pull request #3754 (2022-09-30 08:38:51)

enhancement: remove extra ticks
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
https://github.com/tc39/ecma262/issues/2770
https://github.com/tc39/ecma262/pull/2772
https://github.com/tc39/ecma262/pull/1250
https://v8.dev/blog/fast-async

Depends on #3793

netlify[bot]: ### <span aria-hidden="true">👷</span> Deploy Preview for *compassionate-pike-271cb3* processing.


|  Name | Link |
|---------------------------------|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | f98471eb3c623b35e40a51284379306c2f2c7c28 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/sites/compassionate-pike-271cb3/deploys/6336ab1d745edf00093e440d |
yaacovCR: This PR has been pruned to only include the changes the result in tick decreases -- and is now ready for review!
yaacovCR: In terms of comparing the number of ticks, see the following, adapted from https://github.com/tc39/ecma262/issues/2770

```js
const resolved = Promise.resolve();

function dummyAsync(n) {
  return new Promise(resolve => resolve(n+1));
}

function returnPromiseWithThen() {
  return result.then(r => dummyAsync(r));
}

function returnPromiseWithThenAndCatch() {
  return result.then(r => dummyAsync(r)).then(undefined, () => 0);
}

async function asyncHelperWith2Awaits(p) {
  try {
    const result = await p;
    return await dummyAsync(result);
  } catch (err) {
    return 0;
  }
}

function returnAsyncWith2Awaits() {
  return asyncHelperWith2Awaits(result);
}

async function test(fn) {
  let done = false;
  let count = 0;
  fn().then(() => { done = true; });

  function counter() {
    if (done) {
      console.log(`${fn.name} took ${count} ticks`);
    } else {
      resolved.then(() => {
        count++;
        counter();
      });
    }
  }
  counter();
}

async function tests() {
  await resolved;
  await test(returnAsyncWith2Awaits);
  await test(returnPromiseWithThen);
  await test(returnPromiseWithThenAndCatch);
}

tests();
```

which emits on my instance of chrome:

```
returnAsyncWith2Awaits took 3 ticks
returnPromiseWithThen took 4 ticks
returnPromiseWithThenAndCatch took 5 ticks
```

Additional async helpers may be of use elsewhere; I have focused here so far on value completion.

NOTE:  The commits within this PR are designed to be added individually (rebased, rather than squashed).
yaacovCR: in particular, the `completeValueCatchingErrors` helper could be improved!
yaacovCR: I added [89521b3](https://github.com/graphql/graphql-js/pull/3754/commits/89521b3acffc3a87479a36a9fcb7ce5f44cdb691)
 to "optimize" `completeValueCatchingErrors`.

Benchmarking would presumably tell us whether the tick reduction is outweighted by the number of small helpers.

Awaiting the return of our friendly github-action-bot.  @IvanGoncharov 
yaacovCR: Somewhat old write up: https://v8.dev/blog/fast-async but seems to still be true?
yaacovCR: @github-actions run-benchmark
github-actions[bot]: > @github-actions run-benchmark

@yaacovCR Please, see benchmark results here: https://github.com/graphql/graphql-js/actions/runs/3297051171/jobs/5437417123#step:6:1
yaacovCR: @github-actions run-benchmark
github-actions[bot]: > @github-actions run-benchmark

@yaacovCR Something went wrong, [please check log](https://github.com/graphql/graphql-js/actions/runs/3623925617).
yaacovCR: I removed the sync helper as it doesn't really help anything.
yaacovCR: ![image](https://user-images.githubusercontent.com/4108384/206013897-69507530-1252-4ffc-9c8d-d621155a6a48.png)

If I'm running this right, tests are a bit noisy (running on WSL on my laptop), but looks like a speed bump of 58%....

Which is good! Note that this comes entire from this PR rather than #3793 which is `HEAD~1`
yaacovCR: #3796 formalizes the approach in this PR across the code base using eslint rules to prefer async functions over then/catch. It removes another extra tick.

Patch:
diff --git a/src/execution/execute.ts b/src/execution/execute.ts
index 655ed3b3..ebb9116b 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 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;


Test patch:
diff --git a/src/execution/__tests__/nonnull-test.ts b/src/execution/__tests__/nonnull-test.ts
index a136a2a4..d0b1b614 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 3a4f6ec1..aed5211a 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,
       },
     ]);