Skip to content
This repository has been archived by the owner on Dec 31, 2020. It is now read-only.

fix: render reaction would dispose too early if observable data was changed before first useEffect #328

Merged
merged 2 commits into from
Oct 17, 2020

Conversation

mweststrate
Copy link
Member

Fixed an issue where, if an observable changed between initial render and useEffect, the render reaction would be disposed. This caused any computed values to be cleaned up as well, and become untracked.

It was a bit tricky to reproduce in a unit tests, as they don't run effects async, but see the following screenshots of before and after, and the unit test.

In the screenshot, observerHeader uses amount of todo's left.
Then directly after mount, but before the useEffect happened (create HEADER), we would first change the collection (causing the render reaction to be disposed), and then set up an unrelated reaction. Since the computed value is suspended at that point, it will recomputed.

Screenshot 2020-10-15 at 09 57 33

After the fix, todo's is computed only once, as should be done.

Screenshot 2020-10-15 at 10 18 47

It seem that in the code somebody already thought about this case before, as a field changedBeforeMount was introduced earlier, but it was never set or read.

I've made those fields non optional; a) it would probably have prevented this bug, and b) this is in general faster as as JS engines optimize objects with a fixed shape better.

@changeset-bot
Copy link

changeset-bot bot commented Oct 15, 2020

🦋 Changeset detected

Latest commit: cabaa0a

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

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 93.717% when pulling cabaa0a on fix-observable-change-before-mount into c6c78b3 on master.

@mweststrate mweststrate merged commit 570e8d5 into master Oct 17, 2020
@mweststrate mweststrate deleted the fix-observable-change-before-mount branch October 17, 2020 15:40
@github-actions github-actions bot mentioned this pull request Oct 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants