[DOM] move flushSync out of the reconciler
#28500
Merged
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
gnoff
force-pushed
the
flushsync
branch
4 times, most recently
from
82e73ff
to
21e4fa9
Compare
gnoff
force-pushed
the
flushsync
branch
6 times, most recently
from
5b37221
to
8b62c73
Compare
sebmarkbage
reviewed
Apr 4, 2024
| // for NoEventPriority so instead we assert the type and then | ||
| // cast to avoid the opaque type restriction | ||
| // $FlowFixMe[incompatible-type]: NoEventPriority is valid but EventPriority is opaque | ||
| eventPriority: (0: NoEventPriorityType), |
There was a problem hiding this comment.
Event priority is a very Fiber centric concept where as this store is intended to be more generic.
Originally BatchConfigTransition.transition was supposed to be just the options passed in to the public API but now it has become this very Fiber centric thing even importing the type from Fiber.
Ideally it would be more generic so that it can be implemented in another implementation too.
There was a problem hiding this comment.
I changed it to a boolean since we really only have a binary behavior
sebmarkbage
reviewed
Apr 4, 2024
|
|
||
| type BatchConfig = { | ||
| transition: BatchConfigTransition | null, | ||
| eventPriority: EventPriority, | ||
| }; | ||
| /** | ||
| * Keeps track of the current batch's configuration such as how long an update | ||
| * should suspend for if it needs to. | ||
| */ | ||
| const ReactCurrentBatchConfig: BatchConfig = { | ||
| transition: null, |
There was a problem hiding this comment.
This will eventually have to move to AsyncContext where as the eventPriority doesn't.
sebmarkbage
approved these changes
Apr 4, 2024
sebmarkbage
reviewed
Apr 4, 2024
| const previousEventPriority = ReactCurrentBatchConfig.eventPriority; | ||
|
|
||
| try { | ||
| ReactCurrentBatchConfig.transition = null; |
There was a problem hiding this comment.
It's kind of unfortunate that we have to know about all the other options in every implementation. Because wrapping in one means disabling the outer one in either direction.
Although does that mean that startTransition should really reset eventPriority too?
There was a problem hiding this comment.
I think this happens automatically in that a transition supercedes the sync priority so we don't also have to change the priority. You can't really leave a transition except via another flushSync so I think it just works
gnoff
force-pushed
the
flushsync
branch
6 times, most recently
from
7e746d5
to
3aae888
Compare
…This will only be available in builds that no longer support legacy mode because the reconciler flushSync has special logic for legacy mode which is not necessary for concurrent roots. This is one option for how we might recover existing flushSync priorities with an external flushSync implementation. To reduce hidden class checks we need to read the batch config once which requires inlining the requestCurrentTransition implementation. Moves the legacy implementation of flushSync to the fb entrypoint The cost of the separate file is not really warranted. Will use feature flag to scope build specific implementations Moved error to fiber config. The reconciler implementation should be DCE'd in builds that still support legacy mode Exposes an updateContainerSync implementation so we can avoid depending on flushSyncFromReconciler in hot reloading Removes flushSyncFromReconciler from ReactDOMRoot Removes flushSyncFromReconciler from ReactDOMUpdateBatching Removes flushSyncFromReconciler use from ReactFiberReconciler Removes flushSyncFromReconciler from ReactART Rather than use event priority or lane type semantics for the batch config it now uses a boolean. There really is only a binary of sync or not sync so we don't need to express this concept as something overly specific to Fiber. simplifies the during render case to in dev to avoid a bit of extra work. There is no harm in returning true in prod even if there is nothing to react to it.
github-actions bot
pushed a commit
that referenced
this pull request
Apr 8, 2024
This PR moves `flushSync` out of the reconciler. there is still an internal implementation that is used when these semantics are needed for React methods such as `unmount` on roots. This new isomorphic `flushSync` is only used in builds that no longer support legacy mode. Additionally all the internal uses of flushSync in the reconciler have been replaced with more direct methods. There is a new `updateContainerSync` method which updates a container but forces it to the Sync lane and flushes passive effects if necessary. This combined with flushSyncWork can be used to replace flushSync for all instances of internal usage. We still maintain the original flushSync implementation as `flushSyncFromReconciler` because it will be used as the flushSync implementation for FB builds. This is because it has special legacy mode handling that the new isomorphic implementation does not need to consider. It will be removed from production OSS builds by closure though DiffTrain build for [4c12339](4c12339)
rickhanlonii
pushed a commit
that referenced
this pull request
Apr 11, 2024
This PR moves `flushSync` out of the reconciler. there is still an internal implementation that is used when these semantics are needed for React methods such as `unmount` on roots. This new isomorphic `flushSync` is only used in builds that no longer support legacy mode. Additionally all the internal uses of flushSync in the reconciler have been replaced with more direct methods. There is a new `updateContainerSync` method which updates a container but forces it to the Sync lane and flushes passive effects if necessary. This combined with flushSyncWork can be used to replace flushSync for all instances of internal usage. We still maintain the original flushSync implementation as `flushSyncFromReconciler` because it will be used as the flushSync implementation for FB builds. This is because it has special legacy mode handling that the new isomorphic implementation does not need to consider. It will be removed from production OSS builds by closure though
rickhanlonii
pushed a commit
that referenced
this pull request
Apr 11, 2024
This PR moves `flushSync` out of the reconciler. there is still an internal implementation that is used when these semantics are needed for React methods such as `unmount` on roots. This new isomorphic `flushSync` is only used in builds that no longer support legacy mode. Additionally all the internal uses of flushSync in the reconciler have been replaced with more direct methods. There is a new `updateContainerSync` method which updates a container but forces it to the Sync lane and flushes passive effects if necessary. This combined with flushSyncWork can be used to replace flushSync for all instances of internal usage. We still maintain the original flushSync implementation as `flushSyncFromReconciler` because it will be used as the flushSync implementation for FB builds. This is because it has special legacy mode handling that the new isomorphic implementation does not need to consider. It will be removed from production OSS builds by closure though
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
This PR moves `flushSync` out of the reconciler. there is still an internal implementation that is used when these semantics are needed for React methods such as `unmount` on roots. This new isomorphic `flushSync` is only used in builds that no longer support legacy mode. Additionally all the internal uses of flushSync in the reconciler have been replaced with more direct methods. There is a new `updateContainerSync` method which updates a container but forces it to the Sync lane and flushes passive effects if necessary. This combined with flushSyncWork can be used to replace flushSync for all instances of internal usage. We still maintain the original flushSync implementation as `flushSyncFromReconciler` because it will be used as the flushSync implementation for FB builds. This is because it has special legacy mode handling that the new isomorphic implementation does not need to consider. It will be removed from production OSS builds by closure though
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
This PR moves `flushSync` out of the reconciler. there is still an internal implementation that is used when these semantics are needed for React methods such as `unmount` on roots. This new isomorphic `flushSync` is only used in builds that no longer support legacy mode. Additionally all the internal uses of flushSync in the reconciler have been replaced with more direct methods. There is a new `updateContainerSync` method which updates a container but forces it to the Sync lane and flushes passive effects if necessary. This combined with flushSyncWork can be used to replace flushSync for all instances of internal usage. We still maintain the original flushSync implementation as `flushSyncFromReconciler` because it will be used as the flushSync implementation for FB builds. This is because it has special legacy mode handling that the new isomorphic implementation does not need to consider. It will be removed from production OSS builds by closure though DiffTrain build for commit 4c12339.
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Apr 19, 2024
Summary: This sync includes the changes from: - D56103750 - [TODO] A shim for SECRET_INTERNALS This sync includes the following changes: - **[b5e5ce8e0](facebook/react@b5e5ce8e0 )**: Update ReactNativeTypes for root options (part 2) ([#28857](facebook/react#28857)) //<Ricky>// - **[da6ba53b1](facebook/react@da6ba53b1 )**: [UMD] Remove umd builds ([#28735](facebook/react#28735)) //<Josh Story>// - **[0c245df1d](facebook/react@0c245df1d )**: Complete the typo fix ([#28856](facebook/react#28856)) //<Sebastian Silbermann>// - **[f82051d7a](facebook/react@f82051d7a )**: console test utils fix: match entire string, not just first letter ([#28855](facebook/react#28855)) //<Andrew Clark>// - **[4ca20fd36](facebook/react@4ca20fd36 )**: Test top level fragment inside lazy semantics ([#28852](facebook/react#28852)) //<Sebastian Markbåge>// - **[c0cf7c696](facebook/react@c0cf7c696 )**: Promote ASYNC_ITERATOR symbol to React Symbols ([#28851](facebook/react#28851)) //<Sebastian Markbåge>// - **[657428a9e](facebook/react@657428a9e )**: Add ReactNativeTypes for root options ([#28850](facebook/react#28850)) //<Ricky>// - **[7909d8eab](facebook/react@7909d8eab )**: [Flight] Encode ReadableStream and AsyncIterables ([#28847](facebook/react#28847)) //<Sebastian Markbåge>// - **[13eb61d05](facebook/react@13eb61d05 )**: Move enableUseDeferredValueInitialArg to canary ([#28818](facebook/react#28818)) //<Andrew Clark>// - **[8afa144bd](facebook/react@8afa144bd )**: Enable flag disableClientCache ([#28846](facebook/react#28846)) //<Jan Kassens>// - **[734956ace](facebook/react@734956ace )**: Devtools: Add support for useFormStatus ([#28413](facebook/react#28413)) //<Sebastian Silbermann>// - **[17e920c00](facebook/react@17e920c00 )**: [Flight Reply] Encode Typed Arrays and Blobs ([#28819](facebook/react#28819)) //<Sebastian Markbåge>// - **[0347fcd00](facebook/react@0347fcd00 )**: Add on(Caught|Uncaught|Recoverable) opts to RN ([#28836](facebook/react#28836)) //<Ricky>// - **[c113503ad](facebook/react@c113503ad )**: Flush direct streams in Bun ([#28837](facebook/react#28837)) //<Kenta Iwasaki>// - **[9defcd56b](facebook/react@9defcd56b )**: Remove redundant props assign ([#28829](facebook/react#28829)) //<Sebastian Silbermann>// - **[ed4023603](facebook/react@ed4023603 )**: Fix mistaken "react-server" condition ([#28835](facebook/react#28835)) //<Sebastian Markbåge>// - **[c8a035036](facebook/react@c8a035036 )**: [Fizz] hoistables should never flush before the preamble ([#28802](facebook/react#28802)) //<Josh Story>// - **[4f5c812a3](facebook/react@4f5c812a3 )**: DevTools: Rely on sourcemaps to compute hook name of built-in hooks in newer versions ([#28593](facebook/react#28593)) //<Sebastian Silbermann>// - **[435415962](facebook/react@435415962 )**: Backwards compatibility for string refs on WWW ([#28826](facebook/react#28826)) //<Jack Pope>// - **[608edcc90](facebook/react@608edcc90 )**: [tests] add `assertConsole<method>Dev` helpers ([#28732](facebook/react#28732)) //<Ricky>// - **[da69b6af9](facebook/react@da69b6af9 )**: ReactDOM.requestFormReset ([#28809](facebook/react#28809)) //<Andrew Clark>// - **[374b5d26c](facebook/react@374b5d26c )**: Scaffolding for requestFormReset API ([#28808](facebook/react#28808)) //<Andrew Clark>// - **[41950d14a](facebook/react@41950d14a )**: Automatically reset forms after action finishes ([#28804](facebook/react#28804)) //<Andrew Clark>// - **[dc6a7e01e](facebook/react@dc6a7e01e )**: [Float] Don't preload images inside `<noscript>` ([#28815](facebook/react#28815)) //<Josh Story>// - **[3f947b1b4](facebook/react@3f947b1b4 )**: [tests] Assert scheduler log empty in internalAct ([#28737](facebook/react#28737)) //<Ricky>// - **[bf09089f6](facebook/react@bf09089f6 )**: Remove Scheduler.log from ReactSuspenseFuzz-test ([#28812](facebook/react#28812)) //<Ricky>// - **[84cb3b4cb](facebook/react@84cb3b4cb )**: Hardcode disableIEWorkarounds for www ([#28811](facebook/react#28811)) //<Ricky>// - **[2243b40ab](facebook/react@2243b40ab )**: [tests] assertLog before act in useEffectEvent ([#28763](facebook/react#28763)) //<Ricky>// - **[dfc64c6e3](facebook/react@dfc64c6e3 )**: [tests] assertLog before act in ReactUse ([#28762](facebook/react#28762)) //<Ricky>// - **[42eff4bc7](facebook/react@42eff4bc7 )**: [tests] Fix assertions not flushed before act ([#28745](facebook/react#28745)) //<Ricky>// - **[ed3c65caf](facebook/react@ed3c65caf )**: Warn if outdated JSX transform is detected ([#28781](facebook/react#28781)) //<Andrew Clark>// - **[3f9e237a2](facebook/react@3f9e237a2 )**: Fix: Suspend while recovering from hydration error ([#28800](facebook/react#28800)) //<Andrew Clark>// - **[7f5d25e23](facebook/react@7f5d25e23 )**: Fix cloneElement using string ref w no owner ([#28797](facebook/react#28797)) //<Joseph Savona>// - **[bf40b0244](facebook/react@bf40b0244 )**: [Fizz] Stop publishing external-runtime to stable channel ([#28796](facebook/react#28796)) //<Josh Story>// - **[7f93cb41c](facebook/react@7f93cb41c )**: [DOM] Infer react-server entries bundles if not explicitly configured ([#28795](facebook/react#28795)) //<Josh Story>// - **[f61316535](facebook/react@f61316535 )**: Rename SECRET INTERNALS to `__CLIENT_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE` ([#28789](facebook/react#28789)) //<Sebastian Markbåge>// - **[9644d206e](facebook/react@9644d206e )**: Soften useFormState warning ([#28788](facebook/react#28788)) //<Ricky>// - **[c771016e1](facebook/react@c771016e1 )**: Rename The Secret Export of Server Internals ([#28786](facebook/react#28786)) //<Sebastian Markbåge>// - **[d50323eb8](facebook/react@d50323eb8 )**: Flatten ReactSharedInternals ([#28783](facebook/react#28783)) //<Sebastian Markbåge>// - **[f62cf8c62](facebook/react@f62cf8c62 )**: [Float] treat `props.async` in Float consistent with the rest of react-dom ([#26760](facebook/react#26760)) //<Josh Story>// - **[dfd3d5af8](facebook/react@dfd3d5af8 )**: Add support for transition{run,start,cancel} events ([#27345](facebook/react#27345)) //<Hugo Sales>// - **[1f8327f83](facebook/react@1f8327f83 )**: [Fiber] Use real event priority for hydration scheduling ([#28765](facebook/react#28765)) //<Josh Story>// - **[97c90ed88](facebook/react@97c90ed88 )**: [DOM] Shrink ReactDOMCurrentDispatcher method names ([#28770](facebook/react#28770)) //<Josh Story>// - **[9007fdc8f](facebook/react@9007fdc8f )**: [DOM] Shrink ReactDOMSharedInternals source representation ([#28771](facebook/react#28771)) //<Josh Story>// - **[14f50ad15](facebook/react@14f50ad15 )**: [Flight] Allow lazily resolving outlined models ([#28780](facebook/react#28780)) //<Sebastian Markbåge>// - **[4c12339ce](facebook/react@4c12339ce )**: [DOM] move `flushSync` out of the reconciler ([#28500](facebook/react#28500)) //<Josh Story>// - **[8e1462e8c](facebook/react@8e1462e8c )**: [Fiber] Move updatePriority tracking to renderers ([#28751](facebook/react#28751)) //<Josh Story>// - **[0b3b8a6a3](facebook/react@0b3b8a6a3 )**: jsx: Remove unnecessary hasOwnProperty check ([#28775](facebook/react#28775)) //<Andrew Clark>// - **[2acfb7b60](facebook/react@2acfb7b60 )**: [Flight] Support FormData from Server to Client ([#28754](facebook/react#28754)) //<Sebastian Markbåge>// - **[d1547defe](facebook/react@d1547defe )**: Fast JSX: Don't clone props object ([#28768](facebook/react#28768)) //<Andrew Clark>// - **[bfd8da807](facebook/react@bfd8da807 )**: Make class prop resolution faster ([#28766](facebook/react#28766)) //<Andrew Clark>// - **[cbb6f2b54](facebook/react@cbb6f2b54 )**: [Flight] Support Blobs from Server to Client ([#28755](facebook/react#28755)) //<Sebastian Markbåge>// - **[f33a6b69c](facebook/react@f33a6b69c )**: Track Owner for Server Components in DEV ([#28753](facebook/react#28753)) //<Sebastian Markbåge>// - **[e3ebcd54b](facebook/react@e3ebcd54b )**: Move string ref coercion to JSX runtime ([#28473](facebook/react#28473)) //<Andrew Clark>// - **[fd0da3eef](facebook/react@fd0da3eef )**: Remove _owner field from JSX elements in prod if string refs are disabled ([#28739](facebook/react#28739)) //<Sebastian Markbåge>// Changelog: [General][Changed] - React Native sync for revisions 48b4ecc...b5e5ce8 jest_e2e[run_all_tests] bypass-github-export-checks Reviewed By: kassens Differential Revision: D56251607 fbshipit-source-id: e16db2fa101fc7ed1e009158c76388206beabd5f
This was referenced Apr 24, 2024
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.
This PR moves
flushSyncout of the reconciler. there is still an internal implementation that is used when these semantics are needed for React methods such asunmounton roots.This new isomorphic
flushSyncis only used in builds that no longer support legacy mode.Additionally all the internal uses of flushSync in the reconciler have been replaced with more direct methods. There is a new
updateContainerSyncmethod which updates a container but forces it to the Sync lane and flushes passive effects if necessary. This combined with flushSyncWork can be used to replace flushSync for all instances of internal usage.We still maintain the original flushSync implementation as
flushSyncFromReconcilerbecause it will be used as the flushSync implementation for FB builds. This is because it has special legacy mode handling that the new isomorphic implementation does not need to consider. It will be removed from production OSS builds by closure though