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 issue where cache updates would not cause rerenders in useSuspenseQuery while in strict mode #10601

Merged

Conversation

jerelmiller
Copy link
Member

Closes #10428

Fixes an issue where using useSuspenseQuery would not react to cache updates while using React's strict mode.

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 Feb 24, 2023

⚠️ No Changeset found

Latest commit: 44985b8

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@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-10601-20230224210620.

@jerelmiller
Copy link
Member Author

jerelmiller commented Feb 24, 2023

I can confirm this PR fixes the issue using the snapshot release.

To test using the Spotify demo:

  • Enable strict mode by uncommenting the <StrictMode /> wrapper in client/src/index.tsx
  • Start playing a song
  • Like/unlike the song in the playbar using the heart icon

When on the latest alpha version, you'll notice the hear icon is not properly updated to reflect the change made. The heart should toggle on/off when liking/unliking the track but this does not occur. Using the snapshot release, perform the same process again and you'll notice the heart icon is now properly updated.

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.

I agree with the case being made for this workaround—which the comment lays out very clearly 😄—and the strictMode option will make it straightforward to add tests for these scenarios in the future 🎉

As an aside, it looks like nearly all tests still pass with strictMode: true, with only the suspenseCount assertions needing adjusting for obvious reasons. allows the client to be overridden and uses default variables from the client when none provided in options, however, look like they would either a) need further consideration or b) potentially be an indicator of a strict mode issue we should look at more closely - @jerelmiller just flagging these two tests since you surely have more insight here 👍

// for the first time by allowing React to run this effect, tear it down,
// then set it back up again before we resubscribe.
//
// Authors note (Jerel): This feels super hacky and I hate it, but this
Copy link
Contributor

Choose a reason for hiding this comment

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

🥲

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya......................................................

@jerelmiller
Copy link
Member Author

@alessbell good callout. I'm going to try those tests with a strict mode variant and ensure they work as expected.

@jerelmiller
Copy link
Member Author

@alessbell I've added tests for the 2 you called out, each using strict mode: 7277274 and 44985b8. Just to confirm, do these tests satisfy what you're looking to answer, or is there something further I can test?

@alessbell
Copy link
Contributor

@jerelmiller thanks! Yes, that answers it perfectly 🙏

@jerelmiller jerelmiller merged commit 2f789a4 into release-3.8 Mar 8, 2023
@jerelmiller jerelmiller deleted the useSuspenseQuery-cache-updates-in-strict-mode branch March 8, 2023 22:11
jerelmiller added a commit that referenced this pull request Mar 8, 2023
…seSuspenseQuery` while in strict mode (#10601)"

This reverts commit 2f789a4.
@jerelmiller
Copy link
Member Author

arghhhhh. I forgot to create a changeset for this branch. I'll create a separate PR that links to this one.

jerelmiller added a commit that referenced this pull request Mar 8, 2023
@jerelmiller jerelmiller mentioned this pull request Mar 8, 2023
3 tasks
jerelmiller added a commit that referenced this pull request Mar 8, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 8, 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.

2 participants