Fix incorrect unmounted state update warning #18617
Conversation
facebook-github-bot
added
CLA Signed
React Core Team
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 4537c13:
|
Details of bundled changes.Comparing: 58c895e...4537c13 react-dom
react-art
react-test-renderer
ReactDOM: size: 0.0%, gzip: 0.0% Size changes (stable) |
Details of bundled changes.Comparing: 58c895e...4537c13 react-test-renderer
react-dom
react-art
Size changes (experimental) |
bvaughn
force-pushed
the
fix-unmounted-state-update-warning
branch
from
aa44b1e
to
84fb700
Compare
|
Cool 👍 What is it? |
|
Instead of pushing the fiber to You can also set that bit on the |
We detach fibers (which nulls the field) when we commit a deletion, so any state updates scheduled between that point and when we eventually flush passive effect destroys won't have a way to check if there is a pending passive unmount effect scheduled for its alternate unless we also explicitly track the alternates.
bvaughn
force-pushed
the
fix-unmounted-state-update-warning
branch
from
84fb700
to
a72eab8
Compare
dquote> dquote> Replace the separate current and alternate arrays with a new effect tag, PassiveUnmountPending, that we use to check for the case where a state update is scheduled for an unmoutned component that has a pending passive effect cleanup scheduled.
|
That sounds like a nicer approach, @acdlite. Back to you. |
bvaughn
force-pushed
the
fix-unmounted-state-update-warning
branch
from
a72eab8
to
5223a07
Compare
| @@ -2310,6 +2311,15 @@ export function enqueuePendingPassiveHookEffectUnmount( | |||
| ): void { | |||
| if (runAllPassiveEffectDestroysBeforeCreates) { | |||
| pendingPassiveHookEffectsUnmount.push(effect, fiber); | |||
| if (__DEV__) { | |||
| if (deferPassiveEffectCleanupDuringUnmount) { | |||
| fiber.effectTag |= PassiveUnmountPending; | |||
There was a problem hiding this comment.
Is it weird to only set this bit in DEV mode? It's only needed there.
There was a problem hiding this comment.
Add Dev suffix to the flag?
| if (__DEV__) { | ||
| if (deferPassiveEffectCleanupDuringUnmount) { | ||
| fiber.effectTag |= PassiveUnmountPendingDev; | ||
| const alternate = fiber.alternate; |
There was a problem hiding this comment.
Nit: Can you pass current as an argument through schedulePassiveEffects? We try to limit the number of places we access the .alternate pointer, since one day we're hoping to remove i t.
There was a problem hiding this comment.
I could do that for schedulePassiveEffects() because commitLifeCycles() already has both fibers.
But we also call enqueuePendingPassiveHookEffectUnmount() from commitUnmount() which is called from commitNestedUnmounts() which traces all the through to commitMutationEffects(), so I'm still going to have to access alternate there (and add a bunch of additional params to the functions in between).
Is this still the right change?
There was a problem hiding this comment.
It looks like what we're calling "current" is inconsistent between commitWork and commitDeletion calls.
For e.g. commitWork we pass nextEffect.alternate as current:
react/packages/react-reconciler/src/ReactFiberWorkLoop.new.js
Lines 2192 to 2193 in cfefc81
But for commitDeletion we pass nextEffect as current:
There was a problem hiding this comment.
We also call commitNestedUnmounts and commitUnmount from unmountHostComponents while traversing the tree, so we'd need to add a bunch of additional alternate access here to pass through the added param.
Since this was a "nit" and given the above considerations, I think I'm not going to make any additional change to the alternate stuff for now. Seems like it would be a net loss. We can always follow up with another PR if someone disagrees strongly.
There was a problem hiding this comment.
Right I guess “current” isn’t accurate, it’s just the “other fiber”
Yeah that makes sense. If we got rid of alternates, each fiber would have a shared instance object and we could put the flag on that.
|
Turns out this is actually going to be what tips us over and would have to require us to add a whole other field since we'd run out of double effect tags like we talked about earlier. We probably need to revisit to see if we can do something less invasive. (I'll also do a pass and see if we can remove a couple of other ones.) |
|
I'm not sure I fully understand the "whole other field" comment. I think I'm missing some context. Is there more written down somewhere or is it mostly just convos between you and Andrew? Since this is DEV only, seems like we could use a less optimal, DEV-only strategy for this? |
There was a recent report within Facebook of an invalid "can't perform a React state update on an unmounted component" warning that was eventually reduced to the fact that the Fiber being warned about was not tracked in the
pendingPassiveHookEffectsUnmountarray, but it's alternate was.Unfortunately, because we detach fibers (which nulls the
.alternatefield) when we commit a deletion, any state updates scheduled between that point and the eventual passive effects flush won't have a way to check if there is a pending passive unmount effect scheduled for the alternate unless we also explicitly track the alternates.This fix feels heavy handed and gross to me. Is there a better solution that I'm overlooking?Rather than tracking both sets of Fibers, this PR uses a new DEV-only effect that indicates if the Fiber has a pending passive unmount effect scheduled.Relates to PR #18096