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

Unmounting a component before a query is resolved via useLazyQuery lets the promise resolve instead of aborting the promise. #10698

Merged
merged 15 commits into from
Mar 31, 2023

Conversation

jerelmiller
Copy link
Member

@jerelmiller jerelmiller commented Mar 31, 2023

Fixes #9755
Fixes #10174
Fixes #10198
Fixes #10520
Fixes #10533

This PR addresses a few outstanding useLazyQuery issues and changes the behavior introduced in #10427 when a component unmounts before a query is fully resolved.

Prior to #10427, components that unmounted before the query was resolved were stuck awaiting the promise indefinitely since promise resolution was dependent on the useQuery render cycle to resolve the promise. #10427 attempted to fix this by aborting the promise any time the component unmounted. This change however was a bit too spicy 🌶️ . Aborting the promise also made it impossible for some use cases that relied upon promise resolution to work correctly.

This PR changes the behavior so that unmounting a component will no longer cause the promise to abort, but rather let the promise resolve naturally with the result from the server. This change had a number of positive side effects that resolved a number of other issues, for example, the ability to run the execution function multiple times with different variables in parallel will now resolve each of the promises with the correct data (#9755).

This PR also makes it possible to dynamically change the query document passed to useLazyQuery and ensure the execute function will use the latest set document (#10198). Some additional tests were added to ensure rerendering with new variables before executing the query would use the proper option set.

Checklist:

  • If this PR contains changes to the library itself (not necessary for e.g. docs updates), please include a changeset (see CONTRIBUTING.md)
  • If this PR is a new feature, please reference an issue where a consensus about the design was reached (not necessary for small changes)
  • Make sure all of the significant new logic is covered by tests

@changeset-bot
Copy link

changeset-bot bot commented Mar 31, 2023

🦋 Changeset detected

Latest commit: 18be640

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@apollo/client Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@netlify
Copy link

netlify bot commented Mar 31, 2023

Deploy Preview for apollo-client-docs ready!

Name Link
🔨 Latest commit
🔍 Latest deploy log https://app.netlify.com/sites/apollo-client-docs/deploys/642726b448ad7e031eda901d
😎 Deploy Preview https://deploy-preview-10698--apollo-client-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@jerelmiller jerelmiller added 🐞 bug ⚛️ React-related 🥚 backwards-compatible for PRs that do not introduce any breaking changes labels Mar 31, 2023
@jerelmiller
Copy link
Member Author

/release:pr

@github-actions
Copy link
Contributor

A new release has been made for this PR. You can install it with npm i @apollo/client@0.0.0-pr-10698-20230331051753.

@jerelmiller jerelmiller changed the title Unmounting a component before a query is resolved via useLazyQuery let the promise resolve instead of aborting the promise. Unmounting a component before a query is resolved via useLazyQuery lets the promise resolve instead of aborting the promise. Mar 31, 2023
@jerelmiller jerelmiller force-pushed the resolve-vs-abort-lazy-query branch from 167d5f4 to 18be640 Compare March 31, 2023 17:51
Copy link
Contributor

@alessbell alessbell left a comment

Choose a reason for hiding this comment

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

Always great to see a single PR manage to address so many issues at once 🎉🎉🎉

Comment on lines +36 to +37
// Use refs to track options and the used query to ensure the `execute`
// function remains referentially stable between renders.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@alessbell
Copy link
Contributor

Also not sure why Netlify is deploying in the first place (no docs changes) but it rebuilt just fine here, the PR check just isn't picking that up...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.