Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix] whenResultIsFinished works on Promise of Array of Promises #7843

Merged
merged 4 commits into from
Mar 5, 2024

Conversation

bscherlein
Copy link
Contributor

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!

@apollo-cla
Copy link

@bscherlein: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/

Copy link

netlify bot commented Mar 5, 2024

👷 Deploy request for apollo-server-docs pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 8fad87b

Copy link

codesandbox-ci bot commented Mar 5, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

const callback = jest.fn();
whenResultIsFinished(result, callback);
await new Promise((r) => setImmediate(r));
expect(callback).toBeCalledWith(null, [expected]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super positive this is guaranteed (the callback will be called in parallel with the await finishing?) but if the test isn't flaky that's OK for me.

@@ -113,7 +113,7 @@ export function whenResultIsFinished(
) {
if (isPromise(result)) {
result.then(
(r: any) => callback(null, r),
(r: any) => whenResultIsFinished(r, callback),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This specifically improves the "Promise of Array of ..." case.

.changeset/long-crabs-reply.md Outdated Show resolved Hide resolved
@glasser
Copy link
Member

glasser commented Mar 5, 2024

Thanks!

@glasser glasser enabled auto-merge (squash) March 5, 2024 20:07
@glasser glasser merged commit 72f568e into apollographql:main Mar 5, 2024
16 checks passed
@github-actions github-actions bot mentioned this pull request Mar 5, 2024
glasser pushed a commit that referenced this pull request Mar 5, 2024
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @apollo/server-integration-testsuite@4.10.1

### Patch Changes

- Updated dependencies
\[[`72f568e`](72f568e)]:
    -   @apollo/server@4.10.1

## @apollo/server@4.10.1

### Patch Changes

- [#7843](#7843)
[`72f568e`](72f568e)
Thanks [@bscherlein](https://github.com/bscherlein)! - 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.

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants