-
-
Notifications
You must be signed in to change notification settings - Fork 90
Revert "Add test for proper reaction disposal in StrictMode" #145
Conversation
This reverts commit bf3a6c9.
(It would be good to have some failing tests for #143) |
}) | ||
} | ||
|
||
useDebugValue(reaction, printDebugValue) | ||
|
||
useEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing this useEffect
hook to useLayoutEffect
fixes the bug for me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We certainly don't want to useLayoutEffect
, that's bad for karma and stuff :) Besides, you said this PR alone fixed it, so I am not sure why would modify it even further. Please do provide an example, this is really hard to understand what kind of issue it actually is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ha, yeah, makes sense. Out of curiosity, what’s the reason that committed.current
is set asynchronously in the useEffect
hook rather than synchronously in the body of the useObserver
hook?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving committed.current = true;
up and out of the useEffect
hook also fixes the bug I was encountering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't worry about this. It was really just an early experiment. See #121 for a proper solution and some details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rad, thanks 🎉
Reverts #119
This was surpassed by #121 and should not have been released. It's actually possible it is a culprit of #143 even though current tests are passing.
@RoystonS I think you said you had some problems with this change, right? So it's probably better to get rid of it altogether.