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: disable global state version usage in observer. Fixes #3728 #3763

Merged
merged 3 commits into from
Sep 25, 2023

Conversation

mweststrate
Copy link
Member

This diff disables the usage of global state versioning. The reasoning here is:

  • global state versioning is principle more correct
  • effectively it doesn't make the implementation more compatible with concurrent mode, both implementations fail layer 3 compatibility
  • in neither cases, the snapshot represents what actually will be read during render; as the latest version of all data is read, instead of the state described by the snapshot version.
  • since the local versioning approach doesn't break [Bug] Error: Maximum update depth exceeded #3728 (for which could be argued it is unidiomatic), there seems at this moment little upside to the global version approach.

This PR deliberately doesn't clean up the underlying infra; the tracking of a global state version in the first place. I will probably put up a separate PR for that, but I also want to pursue more implementation solutions that might work better, that build on that infra.

@changeset-bot
Copy link

changeset-bot bot commented Sep 23, 2023

🦋 Changeset detected

Latest commit: 56b8741

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

This PR includes changesets to release 1 package
Name Type
mobx-react-lite 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

@urugator
Copy link
Collaborator

I think you can remove:

// Provides ability to force component update without changing state version
const [, forceUpdate] = React.useState<Symbol>()

and change

               if (!adm.reaction) {
                    // We've lost our reaction and therefore all subscriptions, occurs when:
                    // 1. Timer based finalization registry disposed reaction before component mounted.
                    // 2. React "re-mounts" same component without calling render in between (typically <StrictMode>).
                    // We have to recreate reaction and schedule re-render to recreate subscriptions,
                    // even if state did not change.
                    createReaction(adm)
                    // Update version otherwise onStoreChange() would be ignored
                    adm.stateVersion = Symbol()
                    onStoreChange();
                }

I don't know if it's completely kosher to call onStoreChange during subscription, but IIRC it was working, the only problem was the call was ignored, because the version stayed the same, therefore the workaround with forceUpdate.

@mweststrate mweststrate merged commit 87e5dfb into main Sep 25, 2023
2 checks passed
@github-actions github-actions bot mentioned this pull request Sep 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants