Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make root.unmount() synchronous #22444

Merged
merged 2 commits into from
Sep 27, 2021
Merged

Make root.unmount() synchronous #22444

merged 2 commits into from
Sep 27, 2021

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Sep 27, 2021

When you unmount a root, the internal state that React stores on the DOM node is immediately cleared. So, we should also synchronously delete the React tree. You should be able to create a new root using the same container.

When you call in `flushSync` from an effect, React fires a warning. I've
moved the implementation of this warning out of the reconciler and into
React DOM.

`flushSync` is a renderer API, not an isomorphic API, because it has
behavior that was designed specifically for the constraints of React
DOM. The equivalent API in a different renderer may not be the same.
For example, React Native has a different threading model than the
browser, so it might not make sense to expose a `flushSync` API to the
JavaScript thread.
@acdlite acdlite requested a review from sebmarkbage September 27, 2021 20:41
@facebook-github-bot facebook-github-bot added the React Core Team Opened by a member of the React Core Team label Sep 27, 2021
@sizebot
Copy link

sizebot commented Sep 27, 2021

Comparing: 2e41568...8cb21aa

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js +0.07% 129.84 kB 129.93 kB +0.04% 41.38 kB 41.40 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.07% 132.66 kB 132.75 kB +0.05% 42.34 kB 42.36 kB
facebook-www/ReactDOM-prod.classic.js = 411.83 kB 411.80 kB = 76.26 kB 76.24 kB
facebook-www/ReactDOM-prod.modern.js +0.01% 400.39 kB 400.43 kB = 74.54 kB 74.54 kB
facebook-www/ReactDOMForked-prod.classic.js = 411.83 kB 411.80 kB = 76.26 kB 76.25 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
oss-experimental/react-noop-renderer/cjs/react-noop-renderer.development.js +0.76% 41.84 kB 42.16 kB +1.62% 9.36 kB 9.51 kB
oss-stable-semver/react-noop-renderer/cjs/react-noop-renderer.development.js +0.76% 41.84 kB 42.16 kB +1.62% 9.36 kB 9.51 kB
oss-stable/react-noop-renderer/cjs/react-noop-renderer.development.js +0.76% 41.84 kB 42.16 kB +1.62% 9.36 kB 9.51 kB
oss-experimental/react-noop-renderer/cjs/react-noop-renderer-persistent.development.js +0.76% 41.97 kB 42.29 kB +1.63% 9.38 kB 9.53 kB
oss-stable-semver/react-noop-renderer/cjs/react-noop-renderer-persistent.development.js +0.76% 41.97 kB 42.29 kB +1.63% 9.38 kB 9.53 kB
oss-stable/react-noop-renderer/cjs/react-noop-renderer-persistent.development.js +0.76% 41.97 kB 42.29 kB +1.63% 9.38 kB 9.53 kB

Generated by 🚫 dangerJS against 8cb21aa

When you unmount a root, the internal state that React stores on the
DOM node is immediately cleared. So, we should also synchronously
delete the React tree. You should be able to create a new root using
the same container.
@acdlite acdlite merged commit d3e0869 into facebook:main Sep 27, 2021
facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Oct 12, 2021
Summary:
This sync includes the following changes:
- **[579c008a7](facebook/react@579c008a7 )**: [Fizz/Flight] pipeToNodeWritable(..., writable).startWriting() -> renderToPipeableStream(...).pipe(writable) ([#22450](facebook/react#22450)) //<Sebastian Markbåge>//
- **[f2c381131](facebook/react@f2c381131 )**: fix: useSyncExternalStoreExtra ([#22500](facebook/react#22500)) //<Daishi Kato>//
- **[0ecbbe142](facebook/react@0ecbbe142 )**: Sync hydrate discrete events in capture phase and dont replay discrete events ([#22448](facebook/react#22448)) //<salazarm>//
- **[a724a3b57](facebook/react@a724a3b57 )**: [RFC] Codemod invariant -> throw new Error ([#22435](facebook/react#22435)) //<Andrew Clark>//
- **[201af81b0](facebook/react@201af81b0 )**: Release pooled cache reference in complete/unwind ([#22464](facebook/react#22464)) //<Joseph Savona>//
- **[033efe731](facebook/react@033efe731 )**: Call get snapshot in useSyncExternalStore server shim ([#22453](facebook/react#22453)) //<salazarm>//
- **[7843b142a](facebook/react@7843b142a )**: [Fizz/Flight] Pass in Destination lazily to startFlowing instead of in createRequest ([#22449](facebook/react#22449)) //<Sebastian Markbåge>//
- **[d9fb383d6](facebook/react@d9fb383d6 )**: Extract queueing logic into shared functions ([#22452](facebook/react#22452)) //<Andrew Clark>//
- **[9175f4d15](facebook/react@9175f4d15 )**: Scheduling Profiler: Show Suspense resource .displayName ([#22451](facebook/react#22451)) //<Brian Vaughn>//
- **[eba248c39](facebook/react@eba248c39 )**: [Fizz/Flight] Remove reentrancy hack ([#22446](facebook/react#22446)) //<Sebastian Markbåge>//
- **[66388150e](facebook/react@66388150e )**: Remove usereducer eager bailout ([#22445](facebook/react#22445)) //<Joseph Savona>//
- **[d3e086932](facebook/react@d3e086932 )**: Make root.unmount() synchronous  ([#22444](facebook/react#22444)) //<Andrew Clark>//
- **[2cc6d79c9](facebook/react@2cc6d79c9 )**: Rename onReadyToStream to onCompleteShell ([#22443](facebook/react#22443)) //<Sebastian Markbåge>//
- **[c88fb49d3](facebook/react@c88fb49d3 )**: Improve DEV errors if string coercion throws (Temporal.*, Symbol, etc.) ([#22064](facebook/react#22064)) //<Justin Grant>//
- **[05726d72c](facebook/react@05726d72c )**: [Fix] Errors should not "unsuspend" a transition ([#22423](facebook/react#22423)) //<Andrew Clark>//
- **[3746eaf98](facebook/react@3746eaf98 )**: Packages/React/src/ReactLazy ---> changing -1 to unintialized ([#22421](facebook/react#22421)) //<BIKI DAS>//
- **[04ccc01d9](facebook/react@04ccc01d9 )**: Hydration errors should force a client render ([#22416](facebook/react#22416)) //<Andrew Clark>//
- **[029fdcebb](facebook/react@029fdcebb )**: root.hydrate -> root.isDehydrated ([#22420](facebook/react#22420)) //<Andrew Clark>//
- **[af87f5a83](facebook/react@af87f5a83 )**: Scheduling Profiler marks should include thrown Errors ([#22417](facebook/react#22417)) //<Brian Vaughn>//
- **[d47339ea3](facebook/react@d47339ea3 )**: [Fizz] Remove assignID mechanism ([#22410](facebook/react#22410)) //<Sebastian Markbåge>//
- **[3a50d9557](facebook/react@3a50d9557 )**: Never attach ping listeners in legacy Suspense ([#22407](facebook/react#22407)) //<Andrew Clark>//
- **[82c8fa90b](facebook/react@82c8fa90b )**: Add back useMutableSource temporarily ([#22396](facebook/react#22396)) //<Andrew Clark>//
- **[5b57bc6e3](facebook/react@5b57bc6e3 )**: [Draft] don't patch console during first render ([#22308](facebook/react#22308)) //<Luna Ruan>//
- **[cf07c3df1](facebook/react@cf07c3df1 )**: Delete all but one `build2` reference ([#22391](facebook/react#22391)) //<Andrew Clark>//
- **[bb0d06935](facebook/react@bb0d06935 )**: [build2 -> build] Local scripts //<Andrew Clark>//
- **[0c81d347b](facebook/react@0c81d347b )**: Write artifacts to `build` instead of `build2` //<Andrew Clark>//
- **[4da03c9fb](facebook/react@4da03c9fb )**: useSyncExternalStore React Native version ([#22367](facebook/react#22367)) //<salazarm>//
- **[48d475c9e](facebook/react@48d475c9e )**: correct typos ([#22294](facebook/react#22294)) //<Bowen>//
- **[cb6c619c0](facebook/react@cb6c619c0 )**: Remove Fiber fields that were used for hydrating useMutableSource ([#22368](facebook/react#22368)) //<Sebastian Silbermann>//
- **[64e70f82e](facebook/react@64e70f82e )**: [Fizz] add avoidThisFallback support ([#22318](facebook/react#22318)) //<salazarm>//
- **[3ee7a004e](facebook/react@3ee7a004e )**: devtools: Display actual ReactDOM API name in root type ([#22363](facebook/react#22363)) //<Sebastian Silbermann>//
- **[79b8fc667](facebook/react@79b8fc667 )**: Implement getServerSnapshot in userspace shim ([#22359](facebook/react#22359)) //<Andrew Clark>//
- **[86b3e2461](facebook/react@86b3e2461 )**: Implement useSyncExternalStore on server ([#22347](facebook/react#22347)) //<Andrew Clark>//
- **[8209de269](facebook/react@8209de269 )**: Delete useMutableSource implementation ([#22292](facebook/react#22292)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions e8feb11...afcb9cd

jest_e2e[run_all_tests]

Reviewed By: yungsters

Differential Revision: D31541359

fbshipit-source-id: c35941bc303fdf55cb061e9996200dc868a6f2af
zhengjitf pushed a commit to zhengjitf/react that referenced this pull request Apr 15, 2022
* Move flushSync warning to React DOM

When you call in `flushSync` from an effect, React fires a warning. I've
moved the implementation of this warning out of the reconciler and into
React DOM.

`flushSync` is a renderer API, not an isomorphic API, because it has
behavior that was designed specifically for the constraints of React
DOM. The equivalent API in a different renderer may not be the same.
For example, React Native has a different threading model than the
browser, so it might not make sense to expose a `flushSync` API to the
JavaScript thread.

* Make root.unmount() synchronous

When you unmount a root, the internal state that React stores on the
DOM node is immediately cleared. So, we should also synchronously
delete the React tree. You should be able to create a new root using
the same container.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants