Move update scheduling to microtask #26512
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
facebook-github-bot
added
CLA Signed
React Core Team
acdlite
force-pushed
the
move-scheduling-to-microtask
branch
from
aad8bfc to
9faeced
Compare
|
Comparing: 8310854...5c1cf56 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
|
acdlite
force-pushed
the
move-scheduling-to-microtask
branch
from
9faeced to
55aa9f1
Compare
acdlite
commented
Mar 29, 2023
|
|
||
| let isFlushingWork: boolean = false; | ||
|
|
||
| export function ensureRootIsScheduled(root: FiberRoot): void { |
There was a problem hiding this comment.
This looks like a bunch of new code but it's less new than it looks.
This module replaces the ReactFiberSyncTaskQueue module that existed previously. Instead of being a generic callback queue, we now maintain a list of all the FiberRoots that have work schedule on them. This was effectively what we were already doing because the only type of callback that was ever scheduled was performSyncWorkOnRoot.
Then, since we have a list of roots with pending work, I also use that same data structure to stash the root until the microtask where we can do our scheduling logic. So I moved ensureRootIsScheduled (where most of that logic lives) into this file to try to keep it all colocated.
I originally was just going to dump this all into the work loop module but there's enough going on here that I think it merits its own module. The way I think of it is that the work loop contains all the logic for a particular render + commit phase, and this module is about scheduling those render cycles with the host environment.
There was a problem hiding this comment.
The best way to review this PR is probably to read this module from top to bottom, keeping in mind that most of it isn't actually new but is ported from the previous implementation.
tyao1
reviewed
Mar 30, 2023
| } | ||
| root = root.next; | ||
| } | ||
| } while (didPerformSomeWork); |
There was a problem hiding this comment.
Do we need this outer loop because inner loop can schedule new work?
There was a problem hiding this comment.
Yeah completing one sync task might spawn additional sync tasks so we keep checking until there's nothing left
|
|
||
| // Add the root to the schedule | ||
| if (root === lastScheduledRoot || root.next !== null) { | ||
| // Fast path. This root is already scheduled. |
There was a problem hiding this comment.
No because even if a root is scheduled its priority might have changed so we still need to process it in the microtask
Comment on lines
+321
to
+329
| if ( | ||
| newCallbackPriority === existingCallbackPriority && |
There was a problem hiding this comment.
This moved after the includesSyncLane check compared to the old version. Does this condition no longer apply when includesSyncLane?
There was a problem hiding this comment.
It didn't apply previously either because sync tasks don't get canceled, so I moved it into this more specific branch
There was a problem hiding this comment.
Doesn't this mean that when we have a task scheduled, and then sync work comes in, we wont cancel the first task anymore?
There was a problem hiding this comment.
Yeah that's true, though canceling the callback is really just a perf optimization in that case because the extra callback will immediately exit. Though I suppose that's reason enough to move this back to the common path.
There was a problem hiding this comment.
Ended up adding a separate cancelCallback in that path since it doesn't need the priority check
| @@ -963,7 +963,7 @@ describe('ProfilingCache', () => { | |||
| 2 => 0, | |||
| }, | |||
| "passiveEffectDuration": null, | |||
| "priorityLevel": "Immediate", | |||
| "priorityLevel": "Normal", | |||
There was a problem hiding this comment.
Why did the value change here?
There was a problem hiding this comment.
The Suspense retry runs in a microtask now instead of an Immediate Scheduler task. This is an implementation detail so it doesn't matter.
Yeah probably should because it will likely break the www Relay tests, and maybe some other older tests in www that don't use |
It doesn't have much benefit on its own, except for when you have a single event handler that calls setState many times repeatedly, like in a Flux implementation. Which is more common for older React apps, like Ads Manager. The main benefit is it makes future projects like sync unification easier because we can put more logic into the microtask phase. |
acdlite
force-pushed
the
move-scheduling-to-microtask
branch
3 times, most recently
from
f8cd4bb to
73a68fd
Compare
acdlite
force-pushed
the
move-scheduling-to-microtask
branch
2 times, most recently
from
a09250d to
0bd3dc3
Compare
acdlite
force-pushed
the
move-scheduling-to-microtask
branch
2 times, most recently
from
6289cee to
76c70b7
Compare
rickhanlonii
approved these changes
Mar 30, 2023
| }); | ||
| } | ||
|
|
||
| // TODO: Can we land supportsMicrotasks? Which environments don't support it? |
There was a problem hiding this comment.
I don't think so, React Native does not support microtasks. Hermes is working on it, but it doesn't exist in JSC. If we moved this to the host config, then RN could probably do a user-land fallback for JSC if microtasks were not available. I think I originally had it layered so it was all in the host config so +1.
Previous discussion here: #20658
There was a problem hiding this comment.
I think we could argue that Immediate tasks should run before microtasks, which is essentially the only way we use the immediate priority now, so you could argue this belongs in the Scheduler :)
There was a problem hiding this comment.
Ok I'll just leave this as-is for now then
| // Alternatively, can we move this check to the host config? | ||
| if (supportsMicrotasks) { | ||
| scheduleMicrotask(() => { | ||
| // In Safari, appending an iframe forces microtasks to run. |
There was a problem hiding this comment.
Is this a problem if we're already in a microtask?
There was a problem hiding this comment.
That's why line 456 schedules a Scheduler task instead of a microtask, to prevent a potential loop
| if ((executionContext & (RenderContext | CommitContext)) === NoContext) { | ||
| resetRenderTimer(); | ||
| // TODO: For historical reasons this flushes all sync work across all | ||
| // roots. It shouldn't really matter either way, but we could change this | ||
| // to only flush the given root. |
There was a problem hiding this comment.
I may misunderstand when this happens, but would this matter for RN where multiple roots is common? e.g. the pre-rendering case? Is there a test for this I can look at?
| additionalLogsAfterAttemptingToYield: ['C', 'D'], | ||
| }); | ||
| await waitFor(['B']); | ||
| await waitForAll(['C', 'D']); |
Comment on lines
+321
to
+329
| if ( | ||
| newCallbackPriority === existingCallbackPriority && |
There was a problem hiding this comment.
Doesn't this mean that when we have a task scheduled, and then sync work comes in, we wont cancel the first task anymore?
acdlite
force-pushed
the
move-scheduling-to-microtask
branch
from
4de5afd to
a967c67
Compare
This moves a special check related to `act` in the legacy renderer. I moved it out of the `ensureRootIsScheduled` path and into `scheduleUpdateOnFiber`. There's some related logic that's already there and it makes sense to keep them together. Pure refactor, no behavior change.
acdlite
force-pushed
the
move-scheduling-to-microtask
branch
from
ec26a44 to
be41794
Compare
When React receives new input (via `setState`, a Suspense promise resolution, and so on), it needs to ensure there's a rendering task associated with the update. Most of this happens `ensureRootIsScheduled`. If a single event contains multiple updates, we end up running the scheduling code once per update. But this is wasteful because we really only need to run it once, at the end of the event (or in the case of flushSync, at the end of the scope function's execution). So this PR moves the scheduling logic to happen in a microtask instead. In some cases, we will force it run earlier than that, like for `flushSync`, but since updates are batched by default, it will almost always happen in the microtask. Even for discrete updates. In production, this should have no observable behavior difference. In a testing environment that uses `act`, this should also not have a behavior difference because React will push these tasks to an internal `act` queue. However, tests that do not use `act` and do not simulate an actual production environment (like an e2e test) may be affected. For example, before this change, if a test were to call `setState` outside of `act` and then immediately call `jest.runAllTimers()`, the update would be synchronously applied. After this change, that will no longer work because the rendering task (a timer, in this case) isn't scheduled until after the microtask queue has run. I don't expect this to be an issue in practice because most people do not write their tests this way. They either use `act`, or they write e2e-style tests. The biggest exception has been... our own internal test suite. Until recently, many of our tests were written in a way that accidentally relied on the updates being scheduled synchronously. Over the past few weeks, @tyao1 and I have gradually converted the test suite to use a new set of testing helpers that are resilient to this implementation detail. (There are also some old Relay tests that were written in the style of React's internal test suite. Those will need to be fixed, too.) The larger motivation behind this change, aside from a minor performance improvement, is we intend to use this new microtask to perform additional logic that doesn't yet exist. Like inferring the priority of a custom event.
acdlite
force-pushed
the
move-scheduling-to-microtask
branch
from
be41794 to
5c1cf56
Compare
github-actions bot
pushed a commit
that referenced
this pull request
Mar 31, 2023
When React receives new input (via `setState`, a Suspense promise resolution, and so on), it needs to ensure there's a rendering task associated with the update. Most of this happens `ensureRootIsScheduled`. If a single event contains multiple updates, we end up running the scheduling code once per update. But this is wasteful because we really only need to run it once, at the end of the event (or in the case of flushSync, at the end of the scope function's execution). So this PR moves the scheduling logic to happen in a microtask instead. In some cases, we will force it run earlier than that, like for `flushSync`, but since updates are batched by default, it will almost always happen in the microtask. Even for discrete updates. In production, this should have no observable behavior difference. In a testing environment that uses `act`, this should also not have a behavior difference because React will push these tasks to an internal `act` queue. However, tests that do not use `act` and do not simulate an actual production environment (like an e2e test) may be affected. For example, before this change, if a test were to call `setState` outside of `act` and then immediately call `jest.runAllTimers()`, the update would be synchronously applied. After this change, that will no longer work because the rendering task (a timer, in this case) isn't scheduled until after the microtask queue has run. I don't expect this to be an issue in practice because most people do not write their tests this way. They either use `act`, or they write e2e-style tests. The biggest exception has been... our own internal test suite. Until recently, many of our tests were written in a way that accidentally relied on the updates being scheduled synchronously. Over the past few weeks, @tyao1 and I have gradually converted the test suite to use a new set of testing helpers that are resilient to this implementation detail. (There are also some old Relay tests that were written in the style of React's internal test suite. Those will need to be fixed, too.) The larger motivation behind this change, aside from a minor performance improvement, is we intend to use this new microtask to perform additional logic that doesn't yet exist. Like inferring the priority of a custom event. DiffTrain build for [09c8d25](09c8d25)
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Apr 4, 2023
`act` uses the `didScheduleLegacyUpdate` field to simulate the behavior of batching in React <17 and below. It's a quirk leftover from a previous implementation, not intentionally designed. This sets `didScheduleLegacyUpdate` every time a legacy root receives an update as opposed to only when the `executionContext` is empty. There's no real reason to do it this way over some other way except that it's how it used to work before facebook#26512 and we should try our best to maintain the existing behavior, quirks and all, since existing tests may have come to accidentally rely on it. This should fix some (though not all) of the internal Meta tests that started failing after facebook#26512 landed.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Apr 4, 2023
`act` uses the `didScheduleLegacyUpdate` field to simulate the behavior of batching in React <17 and below. It's a quirk leftover from a previous implementation, not intentionally designed. This sets `didScheduleLegacyUpdate` every time a legacy root receives an update as opposed to only when the `executionContext` is empty. There's no real reason to do it this way over some other way except that it's how it used to work before facebook#26512 and we should try our best to maintain the existing behavior, quirks and all, since existing tests may have come to accidentally rely on it. This should fix some (though not all) of the internal Meta tests that started failing after facebook#26512 landed.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Apr 10, 2023
`act` uses the `didScheduleLegacyUpdate` field to simulate the behavior of batching in React <17 and below. It's a quirk leftover from a previous implementation, not intentionally designed. This sets `didScheduleLegacyUpdate` every time a legacy root receives an update as opposed to only when the `executionContext` is empty. There's no real reason to do it this way over some other way except that it's how it used to work before facebook#26512 and we should try our best to maintain the existing behavior, quirks and all, since existing tests may have come to accidentally rely on it. This should fix some (though not all) of the internal Meta tests that started failing after facebook#26512 landed.
acdlite
added a commit
to acdlite/react
that referenced
this pull request
Apr 10, 2023
`act` uses the `didScheduleLegacyUpdate` field to simulate the behavior of batching in React <17 and below. It's a quirk leftover from a previous implementation, not intentionally designed. This sets `didScheduleLegacyUpdate` every time a legacy root receives an update as opposed to only when the `executionContext` is empty. There's no real reason to do it this way over some other way except that it's how it used to work before facebook#26512 and we should try our best to maintain the existing behavior, quirks and all, since existing tests may have come to accidentally rely on it. This should fix some (though not all) of the internal Meta tests that started failing after facebook#26512 landed.
acdlite
added a commit
that referenced
this pull request
Apr 10, 2023
`act` uses the `didScheduleLegacyUpdate` field to simulate the behavior of batching in React <17 and below. It's a quirk leftover from a previous implementation, not intentionally designed. This sets `didScheduleLegacyUpdate` every time a legacy root receives an update as opposed to only when the `executionContext` is empty. There's no real reason to do it this way over some other way except that it's how it used to work before #26512 and we should try our best to maintain the existing behavior, quirks and all, since existing tests may have come to accidentally rely on it. This should fix some (though not all) of the internal Meta tests that started failing after #26512 landed. Will add a regression test before merging.
github-actions bot
pushed a commit
that referenced
this pull request
Apr 10, 2023
`act` uses the `didScheduleLegacyUpdate` field to simulate the behavior of batching in React <17 and below. It's a quirk leftover from a previous implementation, not intentionally designed. This sets `didScheduleLegacyUpdate` every time a legacy root receives an update as opposed to only when the `executionContext` is empty. There's no real reason to do it this way over some other way except that it's how it used to work before #26512 and we should try our best to maintain the existing behavior, quirks and all, since existing tests may have come to accidentally rely on it. This should fix some (though not all) of the internal Meta tests that started failing after #26512 landed. Will add a regression test before merging. DiffTrain build for [fec97ec](fec97ec)
facebook-github-bot
pushed a commit
to facebook/react-native
that referenced
this pull request
Apr 13, 2023
Summary: This sync includes the following changes: - **[58742c21b](facebook/react@58742c21b )**: Delete unused `eventTimes` Fiber field ([#26599](facebook/react#26599)) //<Andrew Clark>// - **[0b931f90e](facebook/react@0b931f90e )**: Remove JND delay for non-transition updates ([#26597](facebook/react#26597)) //<Andrew Clark>// - **[ac43bf687](facebook/react@ac43bf687 )**: Move validation of text nesting into ReactDOMComponent ([#26594](facebook/react#26594)) //<Sebastian Markbåge>// - **[ca41adb8c](facebook/react@ca41adb8c )**: Diff properties in the commit phase instead of generating an update payload ([#26583](facebook/react#26583)) //<Sebastian Markbåge>// - **[dd0619b2e](facebook/react@dd0619b2e )**: rename $$$hostConfig to $$$config ([#26593](facebook/react#26593)) //<Josh Story>// - **[b55d31955](facebook/react@b55d31955 )**: Rename HostConfig files to FiberConfig to clarify they are configs fo… ([#26592](facebook/react#26592)) //<Josh Story>// - **[ffb8eaca5](facebook/react@ffb8eaca5 )**: Rename ReactServerFormatConfig to ReactFizzConfig ([#26591](facebook/react#26591)) //<Josh Story>// - **[f4f873f62](facebook/react@f4f873f62 )**: Implements wiring for Flight to have it's own "HostConfig" ([#26590](facebook/react#26590)) //<Josh Story>// - **[44db16afc](facebook/react@44db16afc )**: Normalize ReactFlightServerConfig and related files ([#26589](facebook/react#26589)) //<Josh Story>// - **[fec97ecbc](facebook/react@fec97ecbc )**: act: Move didScheduleLegacyUpdate to ensureRootIsScheduled ([#26552](facebook/react#26552)) //<Andrew Clark>// - **[9a9da7721](facebook/react@9a9da7721 )**: Don't update textarea defaultValue and input checked unnecessarily ([#26580](facebook/react#26580)) //<Sebastian Markbåge>// - **[e5146cb52](facebook/react@e5146cb52 )**: Refactor some controlled component stuff ([#26573](facebook/react#26573)) //<Sebastian Markbåge>// - **[657698e48](facebook/react@657698e48 )**: [Tests] `waitForThrow` should diff strings ([#26568](facebook/react#26568)) //<Josh Story>// - **[85bb7b685](facebook/react@85bb7b685 )**: Fix: Move `destroy` field to shared instance object ([#26561](facebook/react#26561)) //<Andrew Clark>// - **[9cfba0f6e](facebook/react@9cfba0f6e )**: Clean up discrete event replaying ([#26558](facebook/react#26558)) //<Sebastian Markbåge>// - **[790ebc962](facebook/react@790ebc962 )**: Remove no-fallthrough lint suppressions ([#26553](facebook/react#26553)) //<Sophie Alpert>// - **[c15579631](facebook/react@c15579631 )**: Put common aliases in Map/Set instead of switch over strings ([#26551](facebook/react#26551)) //<Sebastian Markbåge>// - **[d5fd60f7e](facebook/react@d5fd60f7e )**: Remove findInstanceBlockingEvent unused parameters ([#26534](facebook/react#26534)) //<Mohammad Ghorbani>// - **[eeabb7312](facebook/react@eeabb7312 )**: Refactor DOM Bindings Completely Off of DOMProperty Meta Programming ([#26546](facebook/react#26546)) //<Sebastian Markbåge>// - **[da94e8b24](facebook/react@da94e8b24 )**: Revert "Cleanup enableSyncDefaultUpdate flag ([#26236](facebook/react#26236))" ([#26528](facebook/react#26528)) //<Jan Kassens>// - **[0700dd50b](facebook/react@0700dd50b )**: Implement public instances for text nodes in Fabric ([#26516](facebook/react#26516)) //<Rubén Norte>// - **[4a1cc2ddd](facebook/react@4a1cc2ddd )**: Fix logic around attribute seralization ([#26526](facebook/react#26526)) //<Josh Story>// - **[7329ea81c](facebook/react@7329ea81c )**: Fix suspense replaying forward refs ([#26535](facebook/react#26535)) //<Hans Otto Wirtz>// - **[0ae348018](facebook/react@0ae348018 )**: [Float] Suspend unstyled content for up to 1 minute ([#26532](facebook/react#26532)) //<Andrew Clark>// - **[888874673](facebook/react@888874673 )**: Allow transitions to interrupt Suspensey commits ([#26531](facebook/react#26531)) //<Andrew Clark>// - **[09c8d2563](facebook/react@09c8d2563 )**: Move update scheduling to microtask ([#26512](facebook/react#26512)) //<Andrew Clark>// - **[8310854ce](facebook/react@8310854ce )**: Clean up enableCapturePhaseSelectiveHydrationWithoutDiscreteEventReplay ([#26521](facebook/react#26521)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions ca01f35...58742c2 jest_e2e[run_all_tests] bypass-github-export-checks Reviewed By: sammy-SC Differential Revision: D44872333 fbshipit-source-id: 0695e86645955aac7a20afdaf3ed02ad33592f5c
hoxyq
added a commit
that referenced
this pull request
Apr 17, 2023
Full list of changes (not everything included in changelog): * refactor[devtools]: copy to clipboard only on frontend side ([hoxyq](https://github.com/hoxyq) in [#26604](#26604)) * Provide icon to edge devtools. ([harrygz889](https://github.com/harrygz889) in [#26543](#26543)) * [BE] move shared types & constants to consolidated locations ([mondaychen](https://github.com/mondaychen) in [#26572](#26572)) * remove backend dependency from the global hook ([mondaychen](https://github.com/mondaychen) in [#26563](#26563)) * Replace deprecated `new-window` with `webContents.setWindowOpenHandler()` ([Willie-Boy](https://github.com/Willie-Boy) in [#26559](#26559)) * DevTools: Inline references to fiber flags ([acdlite](https://github.com/acdlite) in [#26542](#26542)) * refactor[devtools]: forbid editing class instances in props ([hoxyq](https://github.com/hoxyq) in [#26522](#26522)) * Move update scheduling to microtask ([acdlite](https://github.com/acdlite) in [#26512](#26512)) * Remove unnecessary CIRCLE_CI_API_TOKEN checks ([mondaychen](https://github.com/mondaychen) in [#26499](#26499)) * browser extension: improve script injection logic ([mondaychen](https://github.com/mondaychen) in [#26492](#26492)) * [flow] make Flow suppressions explicit on the error ([kassens](https://github.com/kassens) in [#26487](#26487))
kassens
pushed a commit
to kassens/react
that referenced
this pull request
Apr 17, 2023
Full list of changes (not everything included in changelog): * refactor[devtools]: copy to clipboard only on frontend side ([hoxyq](https://github.com/hoxyq) in [facebook#26604](facebook#26604)) * Provide icon to edge devtools. ([harrygz889](https://github.com/harrygz889) in [facebook#26543](facebook#26543)) * [BE] move shared types & constants to consolidated locations ([mondaychen](https://github.com/mondaychen) in [facebook#26572](facebook#26572)) * remove backend dependency from the global hook ([mondaychen](https://github.com/mondaychen) in [facebook#26563](facebook#26563)) * Replace deprecated `new-window` with `webContents.setWindowOpenHandler()` ([Willie-Boy](https://github.com/Willie-Boy) in [facebook#26559](facebook#26559)) * DevTools: Inline references to fiber flags ([acdlite](https://github.com/acdlite) in [facebook#26542](facebook#26542)) * refactor[devtools]: forbid editing class instances in props ([hoxyq](https://github.com/hoxyq) in [facebook#26522](facebook#26522)) * Move update scheduling to microtask ([acdlite](https://github.com/acdlite) in [facebook#26512](facebook#26512)) * Remove unnecessary CIRCLE_CI_API_TOKEN checks ([mondaychen](https://github.com/mondaychen) in [facebook#26499](facebook#26499)) * browser extension: improve script injection logic ([mondaychen](https://github.com/mondaychen) in [facebook#26492](facebook#26492)) * [flow] make Flow suppressions explicit on the error ([kassens](https://github.com/kassens) in [facebook#26487](facebook#26487))
kassens
added a commit
to kassens/react
that referenced
this pull request
Apr 17, 2023
This reverts commit 09c8d25.
jeongshin
pushed a commit
to jeongshin/react-native
that referenced
this pull request
May 7, 2023
Summary: This sync includes the following changes: - **[58742c21b](facebook/react@58742c21b )**: Delete unused `eventTimes` Fiber field ([facebook#26599](facebook/react#26599)) //<Andrew Clark>// - **[0b931f90e](facebook/react@0b931f90e )**: Remove JND delay for non-transition updates ([facebook#26597](facebook/react#26597)) //<Andrew Clark>// - **[ac43bf687](facebook/react@ac43bf687 )**: Move validation of text nesting into ReactDOMComponent ([facebook#26594](facebook/react#26594)) //<Sebastian Markbåge>// - **[ca41adb8c](facebook/react@ca41adb8c )**: Diff properties in the commit phase instead of generating an update payload ([facebook#26583](facebook/react#26583)) //<Sebastian Markbåge>// - **[dd0619b2e](facebook/react@dd0619b2e )**: rename $$$hostConfig to $$$config ([facebook#26593](facebook/react#26593)) //<Josh Story>// - **[b55d31955](facebook/react@b55d31955 )**: Rename HostConfig files to FiberConfig to clarify they are configs fo… ([facebook#26592](facebook/react#26592)) //<Josh Story>// - **[ffb8eaca5](facebook/react@ffb8eaca5 )**: Rename ReactServerFormatConfig to ReactFizzConfig ([facebook#26591](facebook/react#26591)) //<Josh Story>// - **[f4f873f62](facebook/react@f4f873f62 )**: Implements wiring for Flight to have it's own "HostConfig" ([facebook#26590](facebook/react#26590)) //<Josh Story>// - **[44db16afc](facebook/react@44db16afc )**: Normalize ReactFlightServerConfig and related files ([facebook#26589](facebook/react#26589)) //<Josh Story>// - **[fec97ecbc](facebook/react@fec97ecbc )**: act: Move didScheduleLegacyUpdate to ensureRootIsScheduled ([facebook#26552](facebook/react#26552)) //<Andrew Clark>// - **[9a9da7721](facebook/react@9a9da7721 )**: Don't update textarea defaultValue and input checked unnecessarily ([facebook#26580](facebook/react#26580)) //<Sebastian Markbåge>// - **[e5146cb52](facebook/react@e5146cb52 )**: Refactor some controlled component stuff ([facebook#26573](facebook/react#26573)) //<Sebastian Markbåge>// - **[657698e48](facebook/react@657698e48 )**: [Tests] `waitForThrow` should diff strings ([facebook#26568](facebook/react#26568)) //<Josh Story>// - **[85bb7b685](facebook/react@85bb7b685 )**: Fix: Move `destroy` field to shared instance object ([facebook#26561](facebook/react#26561)) //<Andrew Clark>// - **[9cfba0f6e](facebook/react@9cfba0f6e )**: Clean up discrete event replaying ([facebook#26558](facebook/react#26558)) //<Sebastian Markbåge>// - **[790ebc962](facebook/react@790ebc962 )**: Remove no-fallthrough lint suppressions ([facebook#26553](facebook/react#26553)) //<Sophie Alpert>// - **[c15579631](facebook/react@c15579631 )**: Put common aliases in Map/Set instead of switch over strings ([facebook#26551](facebook/react#26551)) //<Sebastian Markbåge>// - **[d5fd60f7e](facebook/react@d5fd60f7e )**: Remove findInstanceBlockingEvent unused parameters ([facebook#26534](facebook/react#26534)) //<Mohammad Ghorbani>// - **[eeabb7312](facebook/react@eeabb7312 )**: Refactor DOM Bindings Completely Off of DOMProperty Meta Programming ([facebook#26546](facebook/react#26546)) //<Sebastian Markbåge>// - **[da94e8b24](facebook/react@da94e8b24 )**: Revert "Cleanup enableSyncDefaultUpdate flag ([facebook#26236](facebook/react#26236))" ([facebook#26528](facebook/react#26528)) //<Jan Kassens>// - **[0700dd50b](facebook/react@0700dd50b )**: Implement public instances for text nodes in Fabric ([facebook#26516](facebook/react#26516)) //<Rubén Norte>// - **[4a1cc2ddd](facebook/react@4a1cc2ddd )**: Fix logic around attribute seralization ([facebook#26526](facebook/react#26526)) //<Josh Story>// - **[7329ea81c](facebook/react@7329ea81c )**: Fix suspense replaying forward refs ([facebook#26535](facebook/react#26535)) //<Hans Otto Wirtz>// - **[0ae348018](facebook/react@0ae348018 )**: [Float] Suspend unstyled content for up to 1 minute ([facebook#26532](facebook/react#26532)) //<Andrew Clark>// - **[888874673](facebook/react@888874673 )**: Allow transitions to interrupt Suspensey commits ([facebook#26531](facebook/react#26531)) //<Andrew Clark>// - **[09c8d2563](facebook/react@09c8d2563 )**: Move update scheduling to microtask ([facebook#26512](facebook/react#26512)) //<Andrew Clark>// - **[8310854ce](facebook/react@8310854ce )**: Clean up enableCapturePhaseSelectiveHydrationWithoutDiscreteEventReplay ([facebook#26521](facebook/react#26521)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions ca01f35...58742c2 jest_e2e[run_all_tests] bypass-github-export-checks Reviewed By: sammy-SC Differential Revision: D44872333 fbshipit-source-id: 0695e86645955aac7a20afdaf3ed02ad33592f5c
OlimpiaZurek
pushed a commit
to OlimpiaZurek/react-native
that referenced
this pull request
May 22, 2023
Summary: This sync includes the following changes: - **[58742c21b](facebook/react@58742c21b )**: Delete unused `eventTimes` Fiber field ([facebook#26599](facebook/react#26599)) //<Andrew Clark>// - **[0b931f90e](facebook/react@0b931f90e )**: Remove JND delay for non-transition updates ([facebook#26597](facebook/react#26597)) //<Andrew Clark>// - **[ac43bf687](facebook/react@ac43bf687 )**: Move validation of text nesting into ReactDOMComponent ([facebook#26594](facebook/react#26594)) //<Sebastian Markbåge>// - **[ca41adb8c](facebook/react@ca41adb8c )**: Diff properties in the commit phase instead of generating an update payload ([facebook#26583](facebook/react#26583)) //<Sebastian Markbåge>// - **[dd0619b2e](facebook/react@dd0619b2e )**: rename $$$hostConfig to $$$config ([facebook#26593](facebook/react#26593)) //<Josh Story>// - **[b55d31955](facebook/react@b55d31955 )**: Rename HostConfig files to FiberConfig to clarify they are configs fo… ([facebook#26592](facebook/react#26592)) //<Josh Story>// - **[ffb8eaca5](facebook/react@ffb8eaca5 )**: Rename ReactServerFormatConfig to ReactFizzConfig ([facebook#26591](facebook/react#26591)) //<Josh Story>// - **[f4f873f62](facebook/react@f4f873f62 )**: Implements wiring for Flight to have it's own "HostConfig" ([facebook#26590](facebook/react#26590)) //<Josh Story>// - **[44db16afc](facebook/react@44db16afc )**: Normalize ReactFlightServerConfig and related files ([facebook#26589](facebook/react#26589)) //<Josh Story>// - **[fec97ecbc](facebook/react@fec97ecbc )**: act: Move didScheduleLegacyUpdate to ensureRootIsScheduled ([facebook#26552](facebook/react#26552)) //<Andrew Clark>// - **[9a9da7721](facebook/react@9a9da7721 )**: Don't update textarea defaultValue and input checked unnecessarily ([facebook#26580](facebook/react#26580)) //<Sebastian Markbåge>// - **[e5146cb52](facebook/react@e5146cb52 )**: Refactor some controlled component stuff ([facebook#26573](facebook/react#26573)) //<Sebastian Markbåge>// - **[657698e48](facebook/react@657698e48 )**: [Tests] `waitForThrow` should diff strings ([facebook#26568](facebook/react#26568)) //<Josh Story>// - **[85bb7b685](facebook/react@85bb7b685 )**: Fix: Move `destroy` field to shared instance object ([facebook#26561](facebook/react#26561)) //<Andrew Clark>// - **[9cfba0f6e](facebook/react@9cfba0f6e )**: Clean up discrete event replaying ([facebook#26558](facebook/react#26558)) //<Sebastian Markbåge>// - **[790ebc962](facebook/react@790ebc962 )**: Remove no-fallthrough lint suppressions ([facebook#26553](facebook/react#26553)) //<Sophie Alpert>// - **[c15579631](facebook/react@c15579631 )**: Put common aliases in Map/Set instead of switch over strings ([facebook#26551](facebook/react#26551)) //<Sebastian Markbåge>// - **[d5fd60f7e](facebook/react@d5fd60f7e )**: Remove findInstanceBlockingEvent unused parameters ([facebook#26534](facebook/react#26534)) //<Mohammad Ghorbani>// - **[eeabb7312](facebook/react@eeabb7312 )**: Refactor DOM Bindings Completely Off of DOMProperty Meta Programming ([facebook#26546](facebook/react#26546)) //<Sebastian Markbåge>// - **[da94e8b24](facebook/react@da94e8b24 )**: Revert "Cleanup enableSyncDefaultUpdate flag ([facebook#26236](facebook/react#26236))" ([facebook#26528](facebook/react#26528)) //<Jan Kassens>// - **[0700dd50b](facebook/react@0700dd50b )**: Implement public instances for text nodes in Fabric ([facebook#26516](facebook/react#26516)) //<Rubén Norte>// - **[4a1cc2ddd](facebook/react@4a1cc2ddd )**: Fix logic around attribute seralization ([facebook#26526](facebook/react#26526)) //<Josh Story>// - **[7329ea81c](facebook/react@7329ea81c )**: Fix suspense replaying forward refs ([facebook#26535](facebook/react#26535)) //<Hans Otto Wirtz>// - **[0ae348018](facebook/react@0ae348018 )**: [Float] Suspend unstyled content for up to 1 minute ([facebook#26532](facebook/react#26532)) //<Andrew Clark>// - **[888874673](facebook/react@888874673 )**: Allow transitions to interrupt Suspensey commits ([facebook#26531](facebook/react#26531)) //<Andrew Clark>// - **[09c8d2563](facebook/react@09c8d2563 )**: Move update scheduling to microtask ([facebook#26512](facebook/react#26512)) //<Andrew Clark>// - **[8310854ce](facebook/react@8310854ce )**: Clean up enableCapturePhaseSelectiveHydrationWithoutDiscreteEventReplay ([facebook#26521](facebook/react#26521)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions ca01f35...58742c2 jest_e2e[run_all_tests] bypass-github-export-checks Reviewed By: sammy-SC Differential Revision: D44872333 fbshipit-source-id: 0695e86645955aac7a20afdaf3ed02ad33592f5c
hoxyq
added a commit
that referenced
this pull request
Jun 26, 2023
…est (#27008) - Correctly gate the test to `[18.0.0, 18.2.0]` versions of react, as it was initially defined before #26742 - I have recently fixed the gating logic in #26955 and #26997, should be stable now - I have added a non-gated version of this test, which should run only for the current version of react, the one we build from sources - The test version for react `v[18.0.0, 18.2.0]` should expect `"priorityLevel": "Immediate"`, the other `"priorityLevel": "Normal"`, as it was changed in #26512
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
When React receives new input (via `setState`, a Suspense promise resolution, and so on), it needs to ensure there's a rendering task associated with the update. Most of this happens `ensureRootIsScheduled`. If a single event contains multiple updates, we end up running the scheduling code once per update. But this is wasteful because we really only need to run it once, at the end of the event (or in the case of flushSync, at the end of the scope function's execution). So this PR moves the scheduling logic to happen in a microtask instead. In some cases, we will force it run earlier than that, like for `flushSync`, but since updates are batched by default, it will almost always happen in the microtask. Even for discrete updates. In production, this should have no observable behavior difference. In a testing environment that uses `act`, this should also not have a behavior difference because React will push these tasks to an internal `act` queue. However, tests that do not use `act` and do not simulate an actual production environment (like an e2e test) may be affected. For example, before this change, if a test were to call `setState` outside of `act` and then immediately call `jest.runAllTimers()`, the update would be synchronously applied. After this change, that will no longer work because the rendering task (a timer, in this case) isn't scheduled until after the microtask queue has run. I don't expect this to be an issue in practice because most people do not write their tests this way. They either use `act`, or they write e2e-style tests. The biggest exception has been... our own internal test suite. Until recently, many of our tests were written in a way that accidentally relied on the updates being scheduled synchronously. Over the past few weeks, @tyao1 and I have gradually converted the test suite to use a new set of testing helpers that are resilient to this implementation detail. (There are also some old Relay tests that were written in the style of React's internal test suite. Those will need to be fixed, too.) The larger motivation behind this change, aside from a minor performance improvement, is we intend to use this new microtask to perform additional logic that doesn't yet exist. Like inferring the priority of a custom event.
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
…26552) `act` uses the `didScheduleLegacyUpdate` field to simulate the behavior of batching in React <17 and below. It's a quirk leftover from a previous implementation, not intentionally designed. This sets `didScheduleLegacyUpdate` every time a legacy root receives an update as opposed to only when the `executionContext` is empty. There's no real reason to do it this way over some other way except that it's how it used to work before facebook#26512 and we should try our best to maintain the existing behavior, quirks and all, since existing tests may have come to accidentally rely on it. This should fix some (though not all) of the internal Meta tests that started failing after facebook#26512 landed. Will add a regression test before merging.
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
Full list of changes (not everything included in changelog): * refactor[devtools]: copy to clipboard only on frontend side ([hoxyq](https://github.com/hoxyq) in [facebook#26604](facebook#26604)) * Provide icon to edge devtools. ([harrygz889](https://github.com/harrygz889) in [facebook#26543](facebook#26543)) * [BE] move shared types & constants to consolidated locations ([mondaychen](https://github.com/mondaychen) in [facebook#26572](facebook#26572)) * remove backend dependency from the global hook ([mondaychen](https://github.com/mondaychen) in [facebook#26563](facebook#26563)) * Replace deprecated `new-window` with `webContents.setWindowOpenHandler()` ([Willie-Boy](https://github.com/Willie-Boy) in [facebook#26559](facebook#26559)) * DevTools: Inline references to fiber flags ([acdlite](https://github.com/acdlite) in [facebook#26542](facebook#26542)) * refactor[devtools]: forbid editing class instances in props ([hoxyq](https://github.com/hoxyq) in [facebook#26522](facebook#26522)) * Move update scheduling to microtask ([acdlite](https://github.com/acdlite) in [facebook#26512](facebook#26512)) * Remove unnecessary CIRCLE_CI_API_TOKEN checks ([mondaychen](https://github.com/mondaychen) in [facebook#26499](facebook#26499)) * browser extension: improve script injection logic ([mondaychen](https://github.com/mondaychen) in [facebook#26492](facebook#26492)) * [flow] make Flow suppressions explicit on the error ([kassens](https://github.com/kassens) in [facebook#26487](facebook#26487))
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
…est (facebook#27008) - Correctly gate the test to `[18.0.0, 18.2.0]` versions of react, as it was initially defined before facebook#26742 - I have recently fixed the gating logic in facebook#26955 and facebook#26997, should be stable now - I have added a non-gated version of this test, which should run only for the current version of react, the one we build from sources - The test version for react `v[18.0.0, 18.2.0]` should expect `"priorityLevel": "Immediate"`, the other `"priorityLevel": "Normal"`, as it was changed in facebook#26512
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
When React receives new input (via `setState`, a Suspense promise resolution, and so on), it needs to ensure there's a rendering task associated with the update. Most of this happens `ensureRootIsScheduled`. If a single event contains multiple updates, we end up running the scheduling code once per update. But this is wasteful because we really only need to run it once, at the end of the event (or in the case of flushSync, at the end of the scope function's execution). So this PR moves the scheduling logic to happen in a microtask instead. In some cases, we will force it run earlier than that, like for `flushSync`, but since updates are batched by default, it will almost always happen in the microtask. Even for discrete updates. In production, this should have no observable behavior difference. In a testing environment that uses `act`, this should also not have a behavior difference because React will push these tasks to an internal `act` queue. However, tests that do not use `act` and do not simulate an actual production environment (like an e2e test) may be affected. For example, before this change, if a test were to call `setState` outside of `act` and then immediately call `jest.runAllTimers()`, the update would be synchronously applied. After this change, that will no longer work because the rendering task (a timer, in this case) isn't scheduled until after the microtask queue has run. I don't expect this to be an issue in practice because most people do not write their tests this way. They either use `act`, or they write e2e-style tests. The biggest exception has been... our own internal test suite. Until recently, many of our tests were written in a way that accidentally relied on the updates being scheduled synchronously. Over the past few weeks, @tyao1 and I have gradually converted the test suite to use a new set of testing helpers that are resilient to this implementation detail. (There are also some old Relay tests that were written in the style of React's internal test suite. Those will need to be fixed, too.) The larger motivation behind this change, aside from a minor performance improvement, is we intend to use this new microtask to perform additional logic that doesn't yet exist. Like inferring the priority of a custom event. DiffTrain build for commit 09c8d25.
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
`act` uses the `didScheduleLegacyUpdate` field to simulate the behavior of batching in React <17 and below. It's a quirk leftover from a previous implementation, not intentionally designed. This sets `didScheduleLegacyUpdate` every time a legacy root receives an update as opposed to only when the `executionContext` is empty. There's no real reason to do it this way over some other way except that it's how it used to work before #26512 and we should try our best to maintain the existing behavior, quirks and all, since existing tests may have come to accidentally rely on it. This should fix some (though not all) of the internal Meta tests that started failing after #26512 landed. Will add a regression test before merging. DiffTrain build for commit fec97ec.
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 React receives new input (via
setState, a Suspense promise resolution, and so on), it needs to ensure there's a rendering task associated with the update. Most of this happensensureRootIsScheduled.If a single event contains multiple updates, we end up running the scheduling code once per update. But this is wasteful because we really only need to run it once, at the end of the event (or in the case of flushSync, at the end of the scope function's execution).
So this PR moves the scheduling logic to happen in a microtask instead. In some cases, we will force it run earlier than that, like for
flushSync, but since updates are batched by default, it will almost always happen in the microtask. Even for discrete updates.In production, this should have no observable behavior difference. In a testing environment that uses
act, this should also not have a behavior difference because React will push these tasks to an internalactqueue.However, tests that do not use
actand do not simulate an actual production environment (like an e2e test) may be affected. For example, before this change, if a test were to callsetStateoutside ofactand then immediately calljest.runAllTimers(), the update would be synchronously applied. After this change, that will no longer work because the rendering task (a timer, in this case) isn't scheduled until after the microtask queue has run.I don't expect this to be an issue in practice because most people do not write their tests this way. They either use
act, or they write e2e-style tests.The biggest exception has been... our own internal test suite. Until recently, many of our tests were written in a way that accidentally relied on the updates being scheduled synchronously. Over the past few weeks, @tyao1 and I have gradually converted the test suite to use a new set of testing helpers that are resilient to this implementation detail.
(There are also some old Relay tests that were written in the style of React's internal test suite. Those will need to be fixed, too.)
The larger motivation behind this change, aside from a minor performance improvement, is we intend to use this new microtask to perform additional logic that doesn't yet exist. Like inferring the priority of a custom event.