Avoid Promise.all

...and instead mutate original object

Also addresses:
graphql#2974

src/execution/__tests__/lists-test.ts

@@ -193,6 +193,68 @@ describe('Execute: Handles list nullability', () => {
     });
   });
 
+  it('Contains multiple errors', async () => {
+    const listField = [1, new Error('bad'), new Error('bad again')];
+    const badError = {
+      message: 'bad',
+      locations: [{ line: 1, column: 3 }],
+      path: ['listField', 1],
+    };
+    const badAgainError = {
+      message: 'bad again',
+      locations: [{ line: 1, column: 3 }],
+      path: ['listField', 2],
+    };
+
+    expectJSON(await complete({ listField, as: '[Int]' })).toDeepEqual({
+      data: { listField: [1, null, null] },
+      errors: [badError, badAgainError],
+    });
+    expectJSON(await complete({ listField, as: '[Int]!' })).toDeepEqual({
+      data: { listField: [1, null, null] },
+      errors: [badError, badAgainError],
+    });
+    expectJSON(await complete({ listField, as: '[Int!]' })).toDeepEqual({
+      data: { listField: null },
+      errors: [badError],
+    });
+    expectJSON(await complete({ listField, as: '[Int!]!' })).toDeepEqual({
+      data: null,
+      errors: [badError],
+    });
+  });
+
+  it('Contains multiple errors', async () => {
+    const listField = [1, new Error('bad'), new Error('bad again')];
+    const badError = {
+      message: 'bad',
+      locations: [{ line: 1, column: 3 }],
+      path: ['listField', 1],
+    };
+    const badAgainError = {
+      message: 'bad again',
+      locations: [{ line: 1, column: 3 }],
+      path: ['listField', 2],
+    };
+
+    expectJSON(await complete({ listField, as: '[Int]' })).toDeepEqual({
+      data: { listField: [1, null, null] },
+      errors: [badError, badAgainError],
+    });
+    expectJSON(await complete({ listField, as: '[Int]!' })).toDeepEqual({
+      data: { listField: [1, null, null] },
+      errors: [badError, badAgainError],
+    });
+    expectJSON(await complete({ listField, as: '[Int!]' })).toDeepEqual({
+      data: { listField: null },
+      errors: [badError],
+    });
+    expectJSON(await complete({ listField, as: '[Int!]!' })).toDeepEqual({
+      data: null,
+      errors: [badError],
+    });
+  });
+
   it('Results in error', async () => {
     const listField = new Error('bad');
     const errors = [
@@ -220,4 +282,70 @@ describe('Execute: Handles list nullability', () => {
       errors,
     });
   });
+
+  describe('Waits for all list promises to settle before returning error', () => {
+    const schema = buildSchema('type Query { listField: [Int!] }');
+    const document = parse('{ listField }');
+
+    async function completeSlowAndFast(args?: { reverse?: boolean }) {
+      const fastItem = Promise.resolve(new Error('fastError'));
+
+      let slowSettled = false;
+      const slowItem = new Promise((resolve) =>
+        setTimeout(() => {
+          slowSettled = true;
+          resolve(new Error('slowError'));
+        }, 500),
+      );
+
+      const listField = [slowItem, fastItem];
+
+      if (args?.reverse) {
+        listField.reverse();
+      }
+
+      const result = await execute({
+        schema,
+        document,
+        rootValue: { listField },
+      });
+
+      return {
+        slowSettled,
+        result,
+      };
+    }
+
+    it('Waits if the slow item is first', async () => {
+      const { slowSettled, result } = await completeSlowAndFast();
+      expect(slowSettled).to.equal(true);
+      expectJSON(result).toDeepEqual({
+        data: { listField: null },
+        errors: [
+          {
+            message: 'fastError',
+            locations: [{ line: 1, column: 3 }],
+            path: ['listField', 1],
+          },
+        ],
+      });
+    });
+
+    it('Waits if the slow item is last', async () => {
+      const { slowSettled, result } = await completeSlowAndFast({
+        reverse: true,
+      });
+      expect(slowSettled).to.equal(true);
+      expectJSON(result).toDeepEqual({
+        data: { listField: null },
+        errors: [
+          {
+            message: 'fastError',
+            locations: [{ line: 1, column: 3 }],
+            path: ['listField', 0],
+          },
+        ],
+      });
+    });
+  });
 });

src/execution/__tests__/nonnull-test.ts

@@ -525,6 +525,69 @@ describe('Execute: handles non-nullable types', () => {
     });
   });
 
+  describe('waits for all resolvers to settle prior to returning', () => {
+    const schemaWithSlowAndFast = buildSchema(
+      'type Query { slowError: String! fastError: String! }',
+    );
+
+    async function completeSlowAndFast(query: string) {
+      let slowSettled = false;
+      const result = await execute({
+        schema: schemaWithSlowAndFast,
+        document: parse(query),
+        rootValue: {
+          fastError: () => Promise.resolve(new Error('fastError')),
+          slowError: () =>
+            new Promise((resolve) =>
+              setTimeout(() => {
+                slowSettled = true;
+                resolve(new Error('slowError'));
+              }, 500),
+            ),
+        },
+      });
+
+      return {
+        slowSettled,
+        result,
+      };
+    }
+
+    it('waits if slowError is listed first', async () => {
+      const { slowSettled, result } = await completeSlowAndFast(
+        '{ slowError, fastError }',
+      );
+      expect(slowSettled).to.equal(true);
+      expectJSON(result).toDeepEqual({
+        data: null,
+        errors: [
+          {
+            message: 'fastError',
+            path: ['fastError'],
+            locations: [{ line: 1, column: 14 }],
+          },
+        ],
+      });
+    });
+
+    it('waits if slowError is listed last', async () => {
+      const { slowSettled, result } = await completeSlowAndFast(
+        '{ fastError, slowError }',
+      );
+      expect(slowSettled).to.equal(true);
+      expectJSON(result).toDeepEqual({
+        data: null,
+        errors: [
+          {
+            message: 'fastError',
+            path: ['fastError'],
+            locations: [{ line: 1, column: 3 }],
+          },
+        ],
+      });
+    });
+  });
+
   describe('Handles non-null argument', () => {
     const schemaWithNonNullArg = new GraphQLSchema({
       query: new GraphQLObjectType({

src/execution/executor.ts

@@ -43,10 +43,10 @@ import { devAssert } from '../jsutils/devAssert';
 import { isPromise } from '../jsutils/isPromise';
 import { isObjectLike } from '../jsutils/isObjectLike';
 import { promiseReduce } from '../jsutils/promiseReduce';
-import { promiseForObject } from '../jsutils/promiseForObject';
 import { addPath, pathToArray } from '../jsutils/Path';
 import { isAsyncIterable } from '../jsutils/isAsyncIterable';
 import { isIterableObject } from '../jsutils/isIterableObject';
+import { resolveAfterAll } from '../jsutils/resolveAfterAll';
 
 import { getVariableValues, getArgumentValues } from './values';
 import {
@@ -377,8 +377,8 @@ export class Executor {
     const path = undefined;
 
     switch (operation.operation) {
-      // TODO: Change 'query', etc. => to OperationTypeNode.QUERY, etc.
-      // when ready to drop v15 support.
+      // TODO: Change 'query', etc. => to OperationTypeNode.QUERY, etc. when upstream
+      // graphql-js properly exports OperationTypeNode as a value.
       case 'query':
         return this.executeFields(
           exeContext,
@@ -458,7 +458,7 @@ export class Executor {
     fields: Map<string, ReadonlyArray<FieldNode>>,
   ): PromiseOrValue<ObjMap<unknown>> {
     const results = Object.create(null);
-    let containsPromise = false;
+    const promises: Array<Promise<void>> = [];
 
     for (const [responseName, fieldNodes] of fields.entries()) {
       const fieldPath = addPath(path, responseName, parentType.name);
@@ -471,22 +471,26 @@ export class Executor {
       );
 
       if (result !== undefined) {
-        results[responseName] = result;
         if (isPromise(result)) {
-          containsPromise = true;
+          const promise = result.then((resolved) => {
+            results[responseName] = resolved;
+          });
+          promises.push(promise);
+        } else {
+          results[responseName] = result;
         }
       }
     }
 
     // If there are no promises, we can just return the object
-    if (!containsPromise) {
+    if (!promises.length) {
       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.
-    return promiseForObject(results);
+    // Otherwise, results will only eventually be a map from field name to the
+    // result of resolving that field, which is possibly a promise. Return a
+    // promise that will return this map after resolution is complete.
+    return resolveAfterAll(results, promises);
   }
 
   /**
@@ -748,7 +752,8 @@ export class Executor {
     // This is specified as a simple map, however we're optimizing the path
     // where the list contains no Promises by avoiding creating another Promise.
     const itemType = returnType.ofType;
-    let containsPromise = false;
+
+    const promises: Array<Promise<void>> = [];
     const completedResults = Array.from(result, (item, index) => {
       // No need to modify the info object containing the path,
       // since from here on it is not ever accessed by resolver functions.
@@ -777,27 +782,39 @@ export class Executor {
           );
         }
 
-        if (isPromise(completedItem)) {
-          containsPromise = true;
-          // Note: we don't rely on a `catch` method, but we do expect "thenable"
-          // to take a second callback for the error case.
-          return completedItem.then(undefined, (rawError) => {
+        if (!isPromise(completedItem)) {
+          return completedItem;
+        }
+
+        // Note: we don't rely on a `catch` method, but we do expect "thenable"
+        // to take a second callback for the error case.
+        const promise = completedItem
+          .then(undefined, (rawError) => {
             const error = locatedError(
               rawError,
               fieldNodes,
               pathToArray(itemPath),
             );
             return this.handleFieldError(error, itemType, exeContext);
+          })
+          .then((resolved) => {
+            completedResults[index] = resolved;
           });
-        }
-        return completedItem;
+
+        promises.push(promise);
+
+        return undefined;
       } catch (rawError) {
         const error = locatedError(rawError, fieldNodes, pathToArray(itemPath));
         return this.handleFieldError(error, itemType, exeContext);
       }
     });
 
-    return containsPromise ? Promise.all(completedResults) : completedResults;
+    if (!promises.length) {
+      return completedResults;
+    }
+
+    return resolveAfterAll(completedResults, promises);
   }
 
   /**
@@ -1001,7 +1018,7 @@ export class Executor {
    * __schema, __type and __typename. __typename is special because
    * it can always be queried as a field, even in situations where no
    * other fields are allowed, like on a Union. __schema and __type
-   * could get automatically added to the query type, but that uld
+   * could get automatically added to the query type, but that would
    * require mutating type definitions, which would cause issues.
    *
    */

src/jsutils/resolveAfterAll.ts

@@ -0,0 +1,34 @@
+export function resolveAfterAll<
+  T extends Readonly<unknown> | ReadonlyArray<unknown>,
+>(result: T, promises: ReadonlyArray<Promise<void>>): Promise<T> {
+  return new Promise((resolve, reject) => {
+    let rejected = false;
+    let reason: unknown;
+    let numPromises = promises.length;
+
+    const onFulfilled = () => {
+      numPromises--;
+      if (!numPromises) {
+        if (rejected) {
+          reject(reason);
+        }
+        resolve(result);
+      }
+    };
+
+    const onRejected = (_reason: unknown) => {
+      if (!rejected) {
+        rejected = true;
+        reason = _reason;
+      }
+      numPromises--;
+      if (!numPromises) {
+        reject(reason);
+      }
+    };
+
+    for (const promise of promises) {
+      promise.then(onFulfilled, onRejected);
+    }
+  });
+}