review changes

Author: IvanGoncharov

src/execution/__tests__/executor-test.ts

@@ -2,6 +2,7 @@ import { expect } from 'chai';
 import { describe, it } from 'mocha';
 
 import { expectJSON } from '../../__testUtils__/expectJSON';
+import { resolveOnNextTick } from '../../__testUtils__/resolveOnNextTick';
 
 import { inspect } from '../../jsutils/inspect';
 import { invariant } from '../../jsutils/invariant';
@@ -626,57 +627,52 @@ describe('Execute: Handles basic execution tasks', () => {
   });
 
   it('handles sync errors combined with rejections', async () => {
-    const catchUnhandledRejections = (reason: any) => {
-      throw new Error('Unhandled rejection: ' + reason.message);
-    };
-
-    process.on('unhandledRejection', catchUnhandledRejections);
+    let isAsyncResolverCalled = false;
 
-    try {
-      const schema = new GraphQLSchema({
-        query: new GraphQLObjectType({
-          name: 'Type',
-          fields: {
-            syncNullError: {
-              type: new GraphQLNonNull(GraphQLString),
-              resolve: () => null,
-            },
-            asyncNullError: {
-              type: new GraphQLNonNull(GraphQLString),
-              resolve: () => Promise.resolve(null),
+    const schema = new GraphQLSchema({
+      query: new GraphQLObjectType({
+        name: 'Query',
+        fields: {
+          syncNullError: {
+            type: new GraphQLNonNull(GraphQLString),
+            resolve: () => null,
+          },
+          asyncNullError: {
+            type: new GraphQLNonNull(GraphQLString),
+            async resolve() {
+              await resolveOnNextTick();
+              await resolveOnNextTick();
+              await resolveOnNextTick();
+              isAsyncResolverCalled = true;
+              return Promise.resolve(null);
             },
           },
-        }),
-      });
+        },
+      }),
+    });
 
-      // Order is important here, as the promise has to be created before the synchronous error is thrown
-      const document = parse(`
-        {
-          asyncNullError
-          syncNullError
-        }
-      `);
-
-      const rootValue = {};
-
-      const result = await execute({ schema, document, rootValue });
-      expectJSON(result).toDeepEqual({
-        data: null,
-        errors: [
-          {
-            message:
-              'Cannot return null for non-nullable field Type.asyncNullError.',
-            locations: [{ line: 3, column: 11 }],
-            path: ['asyncNullError'],
-          },
-        ],
-      });
+    // Order is important here, as the promise has to be created before the synchronous error is thrown
+    const document = parse(`
+      {
+        asyncNullError
+        syncNullError
+      }
+    `);
 
-      // Allow time for the asyncReject field to be rejected
-      await new Promise((resolve) => process.nextTick(resolve));
-    } finally {
-      process.off('unhandledRejection', catchUnhandledRejections);
-    }
+    const result = await execute({ schema, document });
+
+    expect(isAsyncResolverCalled).to.equal(true);
+    expectJSON(result).toDeepEqual({
+      data: null,
+      errors: [
+        {
+          message:
+            'Cannot return null for non-nullable field Query.syncNullError.',
+          locations: [{ line: 4, column: 9 }],
+          path: ['syncNullError'],
+        },
+      ],
+    });
   });
 
   it('Full response path is included for non-nullable fields', () => {

src/execution/execute.ts

@@ -444,11 +444,10 @@ function executeFields(
 ): PromiseOrValue<ObjMap<unknown>> {
   const results = Object.create(null);
   let containsPromise = false;
-  let error: Error | undefined;
 
-  for (const [responseName, fieldNodes] of fields.entries()) {
-    const fieldPath = addPath(path, responseName, parentType.name);
-    try {
+  try {
+    for (const [responseName, fieldNodes] of fields.entries()) {
+      const fieldPath = addPath(path, responseName, parentType.name);
       const result = executeField(
         exeContext,
         parentType,
@@ -463,34 +462,26 @@ function executeFields(
           containsPromise = true;
         }
       }
-    } catch (err) {
-      error = err;
     }
-
-    if (error) {
-      break;
+  } 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;
+      });
     }
+    throw error;
   }
 
   // If there are no promises, we can just return the object
   if (!containsPromise) {
-    if (error) {
-      throw error;
-    } else {
-      return results;
-    }
+    return results;
   }
 
   // Otherwise, results is a map from field name to the result of resolving that
   // field, which is possibly a promise. Return a promise that will return this
   // same map, but with any promises replaced with the values they resolved to.
-  const promise = promiseForObject(results);
-  if (error) {
-    // Ensure that any promises returned by other fields are handled, as they may also reject.
-    return promise.then(() => Promise.reject(error));
-  }
-
-  return promise;
+  return promiseForObject(results);
 }
 
 /**