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: atomRegistry deletion overlaps in the cleanup stage #2295

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cjvnjde
Copy link

@cjvnjde cjvnjde commented Dec 8, 2023

I found an issue particularly noticeable in strict mode: initializing two effects followed by a cleanup operation erroneously removes cached data for both effects.
This issue becomes particularly apparent in environments combining strict mode with Next.js. To illustrate, consider the following sequence of events (real sequence on my project):

set key:  testState       1
delete key:  testState    0
set key:  testState       1
set key:  testState       2
delete key:  testState    0 // Issue: should be 1 because only one deletion occurred

The root cause of this issue is the reuse of the same key for multiple 'set' operations, leading to incorrect 'delete' behavior. A single cleanup call affects all instances with the same key rather than the specific instance intended.

To resolve this, we need to use a distinct key for each 'set' operation, and the corresponding 'delete' operation targets this specific key. This approach ensures accurate tracking and removal of individual effects.

Additionally, this fix also fixes this issue. The reason for that problem was the deletion of the "second key" that had been deleted with the previous cleaning.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Dec 8, 2023
@cjvnjde cjvnjde changed the title Fix Duplicate Key Issue in Cleanup Process fix: atomRegistry deletion overlaps in the cleanup stage Dec 8, 2023
@FezVrasta
Copy link
Contributor

I'm experiencing #1994 just using syncEffect while I'm trying to upgrade from React 17 to 18, on Chrome.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Snapshot has already been released.
3 participants