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.
Changing this useEffect hook to useLayoutEffect fixes the bug for me.
There was a problem hiding this comment.
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.
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.
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.
Don't worry about this. It was really just an early experiment. See #121 for a proper solution and some details.
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.