Don't warn about unmounted updates if pending passive unmount #18096
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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. |
bvaughn
requested review from
sebmarkbage,
threepointone and
acdlite
and removed request for
sebmarkbage and
threepointone
threepointone
approved these changes
Feb 21, 2020
| return; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
odd, syntax highlighting seems to be broken for this file
There was a problem hiding this comment.
Huh. Yeah for me me too.
Details of bundled changes.Comparing: 65bbda7...fbd5194 react-dom
react-art
Size changes (stable) |
Details of bundled changes.Comparing: 65bbda7...fbd5194 react-dom
react-art
Size changes (experimental) |
acdlite
requested changes
Feb 21, 2020
acdlite
reviewed
Feb 21, 2020
| ) { | ||
| // If there are pending passive effects unmounts for this Fiber, | ||
| // we can assume that they would have prevented this update. | ||
| if (pendingPassiveHookEffectsUnmount.includes(fiber)) { |
There was a problem hiding this comment.
Nit: includes is a new-ish API, so use indexof or something instead so this doesn't break in older browsers.
acdlite
reviewed
Feb 21, 2020
| }); | ||
| }); | ||
|
|
||
| it('still warns about state updates for unmounted components with no pending passive unmounts', () => { |
There was a problem hiding this comment.
Can you add a test for when there's a pending unmount on a different component?
acdlite
approved these changes
Feb 21, 2020
|
I'm personally OK with merging this but I do think we should continue to think about the other types of side effects that might occur in an unmount function but aren't a state update |
|
This change is narrowly scoped to only suppress warnings for things that happen between a committed unmount and flushing of passive effects. It won't block any warnings for anything else- including a |
I recently landed a change to the timing of passive effect cleanup functions during unmount (see facebook#17925). This change defers flushing of passive effects for unmounted components until later (whenever we next flush pending passive effects). Since this change increases the likelihood of a (not actionable) state update warning for unmounted components, I've suppressed that warning for Fibers that have scheduled passive effect unmounts pending.
bvaughn
force-pushed
the
suppress-unmounted-state-warning-pending-passive-effects
branch
from
f4be010
to
fbd5194
Compare
bvaughn
deleted the
suppress-unmounted-state-warning-pending-passive-effects
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I recently landed a change to the timing of passive effect cleanup functions during unmount (see #17925). This change defers flushing of passive effects for unmounted components until later (whenever we next flush pending passive effects).
Since this change increases the likelihood of a (not actionable) state update warning for unmounted components, I've suppressed that warning for Fibers that have scheduled passive effect unmounts pending.