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 an issue where useSuspenseQuery would not respond to cache updates when using a cache-first fetch policy with data in the cache when the hook is mounted #10651

Merged
merged 3 commits into from
Mar 15, 2023

Conversation

jerelmiller
Copy link
Member

Fixes #10478

Fixes an issue where mounting useSuspenseQuery while using a cache-first fetch policy (the default) with data in the cache would not respond to cache updates after the initial result.

See this comment with more information on the mechanics behind why this was not working properly.

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

…ith data already in the cache

This fixes an issue where `useSuspenseQuery` would not respond to cache updates
when using a cache-first `fetchPolicy` after the hook was mounted with data
already in the cache.

This is due to the fact that this hook manages the fetching lifecycle
(via `reobserve`) rather than the subscription. We disable fetching when
subscribing to the observable since we kick off the fetch in the first render.
This however has some downstream issues, since `reobserve` is necessary to set
some internal state updates on `ObservableQuery` and `QueryInfo`. In cases where
we can fulfill the result via the cache immediately, we avoid calling
`reobserve` by subscribing (via the `fetchOnFirstSubscribe` option) to avoid the
network request, but this means the interal state updates do not happen.

In this case, `QueryInfo`, is initialized with a `networkStatus` of 1, but
because we don't call `reobserve`, this value never gets updated to 7 even
though `observableQuery.getCurrentResult()` is able to correctly set this value
to 7. This caused issues where `broadcastQueries` was not sending notifications
since `queryInfo` avoids broadcasting to in-flight queries for fetch policies
other than cache-only and cache-and-network.

This attempts to patch the behavior expected from in `reobserve` by
marking the queryInfo as ready if we detect that the result is also
ready.
@changeset-bot
Copy link

changeset-bot bot commented Mar 13, 2023

🦋 Changeset detected

Latest commit: 4098548

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

@jerelmiller
Copy link
Member Author

jerelmiller commented Mar 13, 2023

Ultimately I may decide to refactor useSuspenseQuery a bit in a future PR as I'm seeing a few more of these types of patch updates pop up with the approach I'm using. The problem is that suspense is not built with observables in mind (which are typically lazy) and I need to work around that fact to work with suspense (such as getting a promise during render that we can then throw).

These types of PRs always give me a bit of pause since it feels wrong that I would need to "patch" some of this behavior as a one-off case. I'd love to harden this a bit more so that it doesn't feel like I'm trying to hack around the existing implementation.

@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-10651-20230313210704.

@jerelmiller
Copy link
Member Author

I can confirm this fixes the issue by installing this snapshot release in the Spotify demo and running through the reproduction steps from #10478

@jerelmiller jerelmiller merged commit 8355d0e into release-3.8 Mar 15, 2023
@jerelmiller jerelmiller deleted the fix-remounted-cache-issue branch March 15, 2023 19:14
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

useSuspenseQuery does not respond to cache updates when the component unmounts, then remounts
2 participants