Bugfix: In legacy mode, call suspended tree's unmount effects when it is deleted #24400
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
acdlite
changed the title
acdlite
changed the title
|
Comparing: 2bf5eba...ca98b30 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
When a suspended tree switches to a fallback, we unmount the effects. If the suspended tree is then deleted, there's a guard to prevent us from unmounting the effects again. However, in legacy mode, we don't unmount effects when a tree suspends. So if the suspended tree is then deleted, we do need to unmount the effects. We're missing a check for legacy/concurrent mode.
acdlite
force-pushed
the
fix-missing-unmount-legacy
branch
from
4d55e60
to
ca98b30
Compare
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Apr 26, 2022
Summary: This sync includes the following changes: - **[bd4784c8f](facebook/react@bd4784c8f )**: Revert #24236 (Don't recreate the same fallback on the client if hydrating suspends) ([#24434](facebook/react#24434)) //<dan>// - **[6d3b6d0f4](facebook/react@6d3b6d0f4 )**: forwardRef et al shouldn't affect if props reused ([#24421](facebook/react#24421)) //<Andrew Clark>// - **[bd0813766](facebook/react@bd0813766 )**: Fix: useDeferredValue should reuse previous value ([#24413](facebook/react#24413)) //<Andrew Clark>// - **[9ae80d6a2](facebook/react@9ae80d6a2 )**: Suppress hydration warnings when a preceding sibling suspends ([#24404](facebook/react#24404)) //<Josh Story>// - **[0dc4e6663](facebook/react@0dc4e6663 )**: Land enableClientRenderFallbackOnHydrationMismatch ([#24410](facebook/react#24410)) //<Andrew Clark>// - **[354772952](facebook/react@354772952 )**: Land enableSelectiveHydration flag ([#24406](facebook/react#24406)) //<Andrew Clark>// - **[392808a1f](facebook/react@392808a1f )**: Land enableClientRenderFallbackOnTextMismatch flag ([#24405](facebook/react#24405)) //<Andrew Clark>// - **[1e748b452](facebook/react@1e748b452 )**: Land enableLazyElements flag ([#24407](facebook/react#24407)) //<Andrew Clark>// - **[4175f0593](facebook/react@4175f0593 )**: Temporarily feature flag numeric fallback for symbols ([#24401](facebook/react#24401)) //<Ricky>// - **[a6d53f346](facebook/react@a6d53f346 )**: Revert "Clean up Selective Hydration / Event Replay flag ([#24156](facebook/react#24156))" ([#24402](facebook/react#24402)) //<Ricky>// - **[ab9cdd34f](facebook/react@ab9cdd34f )**: Bugfix: In legacy mode, call suspended tree's unmount effects when it is deleted ([#24400](facebook/react#24400)) //<Andrew Clark>// - **[168da8d55](facebook/react@168da8d55 )**: Fix typo that happened during rebasing //<Andrew Clark>// - **[8bc527a4c](facebook/react@8bc527a4c )**: Bugfix: Fix race condition between interleaved and non-interleaved updates ([#24353](facebook/react#24353)) //<Andrew Clark>// - **[f7cf077cc](facebook/react@f7cf077cc )**: [Transition Tracing] Add Offscreen Queue ([#24341](facebook/react#24341)) //<Luna Ruan>// - **[4fc394bbe](facebook/react@4fc394bbe )**: Fix suspense fallback throttling ([#24253](facebook/react#24253)) //<sunderls>// - **[80170a068](facebook/react@80170a068 )**: Match bundle.name and match upper case entry points ([#24346](facebook/react#24346)) //<Sebastian Markbåge>// - **[fea6f8da6](facebook/react@fea6f8da6 )**: [Transition Tracing] Add transition to OffscreenState and pendingSuspenseBoundaries to RootState ([#24340](facebook/react#24340)) //<Luna Ruan>// - **[8e2f9b086](facebook/react@8e2f9b086 )**: move passive flag ([#24339](facebook/react#24339)) //<Luna Ruan>// - **[55a21ef7e](facebook/react@55a21ef7e )**: fix pushTransition for transition tracing ([#24338](facebook/react#24338)) //<Luna Ruan>// - **[069d23bb7](facebook/react@069d23bb7 )**: [eslint-plugin-exhaustive-deps] Fix exhaustive deps check for unstable vars ([#24343](facebook/react#24343)) //<Afzal Sayed>// - **[4997515b9](facebook/react@4997515b9 )**: Point useSubscription to useSyncExternalStore shim ([#24289](facebook/react#24289)) //<dan>// - **[01e2bff1d](facebook/react@01e2bff1d )**: Remove unnecessary check ([#24332](facebook/react#24332)) //<zhoulixiang>// - **[d9a0f9e20](facebook/react@d9a0f9e20 )**: Delete create-subscription folder ([#24288](facebook/react#24288)) //<dan>// - **[f993ffc51](facebook/react@f993ffc51 )**: Fix infinite update loop that happens when an unmemoized value is passed to useDeferredValue ([#24247](facebook/react#24247)) //<Andrew Clark>// - **[fa5800226](facebook/react@fa5800226 )**: [Fizz] Pipeable Stream Perf ([#24291](facebook/react#24291)) //<Josh Story>// - **[0568c0f8c](facebook/react@0568c0f8c )**: Replace zero with NoLanes for consistency in FiberLane ([#24327](facebook/react#24327)) //<Leo>// - **[e0160d50c](facebook/react@e0160d50c )**: add transition tracing transitions stack ([#24321](facebook/react#24321)) //<Luna Ruan>// - **[b0f13e5d3](facebook/react@b0f13e5d3 )**: add pendingPassiveTransitions ([#24320](facebook/react#24320)) //<Luna Ruan>// Changelog: [General][Changed] - React Native sync for revisions 60e63b9...bd4784c jest_e2e[run_all_tests] Reviewed By: kacieb Differential Revision: D35899012 fbshipit-source-id: 86a885e336fca9f0efa80cd2b8ca040f2cb53853
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Jun 2, 2022
Interleaves updates (updates that are scheduled while another render is already is progress) go into a special queue that isn't applied until the end of the current render. They are transferred to the "real" queue at the beginning of the next render. Currently we check during `setState` whether an update should go directly onto the real queue or onto the special interleaved queue. The logic is subtle and it can lead to bugs if you mess it up, as in facebook#24400. Instead, this changes it to always go onto the interleaved queue. The behavior is the same but the logic is simpler. As a further step, we can also wait to update the `childLanes` until the end of the current render. I'll do this in the next step.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Jun 3, 2022
Interleaves updates (updates that are scheduled while another render is already is progress) go into a special queue that isn't applied until the end of the current render. They are transferred to the "real" queue at the beginning of the next render. Currently we check during `setState` whether an update should go directly onto the real queue or onto the special interleaved queue. The logic is subtle and it can lead to bugs if you mess it up, as in facebook#24400. Instead, this changes it to always go onto the interleaved queue. The behavior is the same but the logic is simpler. As a further step, we can also wait to update the `childLanes` until the end of the current render. I'll do this in the next step.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Jun 3, 2022
Interleaves updates (updates that are scheduled while another render is already is progress) go into a special queue that isn't applied until the end of the current render. They are transferred to the "real" queue at the beginning of the next render. Currently we check during `setState` whether an update should go directly onto the real queue or onto the special interleaved queue. The logic is subtle and it can lead to bugs if you mess it up, as in facebook#24400. Instead, this changes it to always go onto the interleaved queue. The behavior is the same but the logic is simpler. As a further step, we can also wait to update the `childLanes` until the end of the current render. I'll do this in the next step.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Jun 3, 2022
Interleaves updates (updates that are scheduled while another render is already is progress) go into a special queue that isn't applied until the end of the current render. They are transferred to the "real" queue at the beginning of the next render. Currently we check during `setState` whether an update should go directly onto the real queue or onto the special interleaved queue. The logic is subtle and it can lead to bugs if you mess it up, as in facebook#24400. Instead, this changes it to always go onto the interleaved queue. The behavior is the same but the logic is simpler. As a further step, we can also wait to update the `childLanes` until the end of the current render. I'll do this in the next step.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Jun 6, 2022
Interleaves updates (updates that are scheduled while another render is already is progress) go into a special queue that isn't applied until the end of the current render. They are transferred to the "real" queue at the beginning of the next render. Currently we check during `setState` whether an update should go directly onto the real queue or onto the special interleaved queue. The logic is subtle and it can lead to bugs if you mess it up, as in facebook#24400. Instead, this changes it to always go onto the interleaved queue. The behavior is the same but the logic is simpler. As a further step, we can also wait to update the `childLanes` until the end of the current render. I'll do this in the next step.
acdlite
added a commit
that referenced
this pull request
Jun 6, 2022
* Always push updates to interleaved queue first Interleaves updates (updates that are scheduled while another render is already is progress) go into a special queue that isn't applied until the end of the current render. They are transferred to the "real" queue at the beginning of the next render. Currently we check during `setState` whether an update should go directly onto the real queue or onto the special interleaved queue. The logic is subtle and it can lead to bugs if you mess it up, as in #24400. Instead, this changes it to always go onto the interleaved queue. The behavior is the same but the logic is simpler. As a further step, we can also wait to update the `childLanes` until the end of the current render. I'll do this in the next step. * Move setState return path traversal to own call A lot of the logic around scheduling an update needs access to the fiber root. To obtain this reference, we must walk up the fiber return path. We also do this to update `childLanes` on all the parent nodes, so we can use the same traversal for both purposes. The traversal currently happens inside `scheduleUpdateOnFiber`, but sometimes we need to access it beyond that function, too. So I've hoisted the traversal out of `scheduleUpdateOnFiber` into its own function call that happens at the beginning of the `setState` algorithm. * Rename ReactInterleavedUpdates -> ReactFiberConcurrentUpdates The scope of this module is expanding so I've renamed accordingly. No behavioral changes. * Enqueue and update childLanes in same function During a setState, the childLanes are updated immediately, even if a render is already in progress. This can lead to subtle concurrency bugs, so the plan is to wait until the in-progress render has finished before updating the childLanes, to prevent subtle concurrency bugs. As a step toward that change, when scheduling an update, we should not update the childLanes directly, but instead defer to the ReactConcurrentUpdates module to do it at the appropriate time. This makes markUpdateLaneFromFiberToRoot a private function that is only called from the ReactConcurrentUpdates module. * [FORKED] Don't update childLanes until after current render (This is the riskiest commit in the stack. Only affects the "new" reconciler fork.) Updates that occur in a concurrent event while a render is already in progress can't be processed during that render. This is tricky to get right. Previously we solved this by adding concurrent updates to a special `interleaved` queue, then transferring the `interleaved` queue to the `pending` queue after the render phase had completed. However, we would still mutate the `childLanes` along the parent path immediately, which can lead to its own subtle data races. Instead, we can queue the entire operation until after the render phase has completed. This replaces the need for an `interleaved` field on every fiber/hook queue. The main motivation for this change, aside from simplifying the logic a bit, is so we can read information about the current fiber while we're walking up its return path, like whether it's inside a hidden tree. (I haven't done anything like that in this commit, though.) * Add 17691ac to forked revisions
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.
When a suspended tree switches to a fallback, we unmount the effects. If the suspended tree is then deleted, there's a guard to prevent us from unmounting the effects again (added in ec52a56)
However, in legacy mode, we don't unmount effects when a tree suspends. So if the suspended tree is then deleted, we do need to unmount the effects.
This only affects apps using
ReactDOM.render, which is deprecated (not supported) in React 18+. (And React Native, which hasn't finished adopting the React 18 behavior yet.)Until
ReactDOM.renderis completely removed, though, we need to make sure to add concurrent mode checks whenever we add Offscreen-related logic to a shared codepath.