Skip to content

Commit

Permalink
[fix] whenResultIsFinished works on Promise of Array of Promises (#7843)
Browse files Browse the repository at this point in the history
Paired with @bbeesley.

Updates whenResultIsFinished to call callback also on Promise of Array
of Promises.
Fixes (partially)
#5372 and
#4667.

The test case we're fixing is `'passes result of Promise of Array of
Promises to the callback'`, the other tests test existing behaviour.

The existing bug prevented us from dynamically setting cache hints in
the reference resolver depending on the result of an asynchronous
operation. We discovered the bug within an integration test. If you
@glasser have an idea how to add an integration test and would like to
add it, that would be amazing!

---------

Co-authored-by: David Glasser <glasser@apollographql.com>
  • Loading branch information
bscherlein and glasser authored Mar 5, 2024
1 parent 5704146 commit 72f568e
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 2 deletions.
5 changes: 5 additions & 0 deletions .changeset/long-crabs-reply.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@apollo/server": patch
---

Improves timing of the `willResolveField` end hook on fields which return Promises resolving to Arrays. This makes the use of the `setCacheHint` method more reliable.
37 changes: 37 additions & 0 deletions packages/server/src/__tests__/utils/schemaInstrumentation.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { describe, expect, it, jest } from '@jest/globals';
import { whenResultIsFinished } from '../../utils/schemaInstrumentation';

describe('whenResultIsFinished', () => {
it('passes result of Promise to the callback', async () => {
const expected = 1;
const result = Promise.resolve(expected);
const callback = jest.fn();
whenResultIsFinished(result, callback);
await new Promise((r) => setImmediate(r));
expect(callback).toBeCalledWith(null, expected);
});
it('passes result of Array of Promises to the callback', async () => {
const expected = 1;
const result = [Promise.resolve(expected)];
const callback = jest.fn();
whenResultIsFinished(result, callback);
await new Promise((r) => setImmediate(r));
expect(callback).toBeCalledWith(null, [expected]);
});
it('passes result which is not asynchronous directly to the callback', async () => {
const expected = 1;
const result = expected;
const callback = jest.fn();
whenResultIsFinished(result, callback);
await new Promise((r) => setImmediate(r));
expect(callback).toBeCalledWith(null, expected);
});
it('passes result of Promise of Array of Promises to the callback', async () => {
const expected = 1;
const result = Promise.resolve([Promise.resolve(expected)]);
const callback = jest.fn();
whenResultIsFinished(result, callback);
await new Promise((r) => setImmediate(r));
expect(callback).toBeCalledWith(null, [expected]);
});
});
5 changes: 3 additions & 2 deletions packages/server/src/utils/schemaInstrumentation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,14 +106,15 @@ function isPromise(x: any): boolean {

// Given result (which may be a Promise or an array some of whose elements are
// promises) Promises, set up 'callback' to be invoked when result is fully
// resolved.
// resolved. (Unfortunately, this does not perfectly handle every possible
// return value shape, such as arrays of arrays of Promises.)
export function whenResultIsFinished(
result: any,
callback: (err: Error | null, result?: any) => void,
) {
if (isPromise(result)) {
result.then(
(r: any) => callback(null, r),
(r: any) => whenResultIsFinished(r, callback),
(err: Error) => callback(err),
);
} else if (Array.isArray(result)) {
Expand Down

0 comments on commit 72f568e

Please sign in to comment.