-
Notifications
You must be signed in to change notification settings - Fork 46.4k
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
Fix: Don't flush discrete updates at end of batchedUpdates
, only legacy sync updates
#21229
Fix: Don't flush discrete updates at end of batchedUpdates
, only legacy sync updates
#21229
Conversation
|
||
describe('ReactFiberHostContext', () => { | ||
beforeEach(() => { | ||
jest.resetModules(); | ||
React = require('react'); | ||
ReactFiberReconciler = require('react-reconciler'); | ||
ConcurrentRoot = require('react-reconciler/src/ReactRootTags'); | ||
ConcurrentRoot = require('react-reconciler/src/ReactRootTags') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in this file aren't related. The tests were already wrong because this import was assigning the whole default exports object to ConcurrentRoot
, instead of just that tag. So the test below was passing an invalid value as a root tag, which caused to to behave as if it were in legacy mode.
This PR added a check for root.tag === LegacyRoot
, which exposed the mistake in this file.
6fd5e18
to
8973f71
Compare
Comparing: 89847bf...c8bd181 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: (No significant changes) |
8973f71
to
03b4375
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this fix this test (I confirmed locally that it doesn't)?
I think that test is fine. See my comment here: https://github.com/facebook/react/pull/21072/files#r611099834 |
03b4375
to
f7f0258
Compare
The outermost `batchedUpdates` call flushes pending sync updates at the end. This was intended for legacy sync mode, but it also happens to flush discrete updates in concurrent mode. Instead, we should only flush sync updates at the end of `batchedUpdates` for legacy roots. Discrete sync updates can wait to flush in the microtask. `discreteUpdates` has the same issue, which is how I originally noticed this, but I'll change that one in a separate commit since it requires updating a few (no longer relevant) internal tests.
4710b05
to
c8bd181
Compare
Summary: This sync includes the following changes: - **[9a2591681](facebook/react@9a2591681 )**: Fix export //<Sebastian Markbage>// - **[4a8deb083](facebook/react@4a8deb083 )**: Switch the isPrimaryRender flag based on the stream config ([#21357](facebook/react#21357)) //<Sebastian Markbåge>// - **[bd4f056a3](facebook/react@bd4f056a3 )**: [Fizz] Implement lazy components and nodes ([#21355](facebook/react#21355)) //<Sebastian Markbåge>// - **[fc33f12bd](facebook/react@fc33f12bd )**: Remove unstable scheduler/tracing API ([#20037](facebook/react#20037)) //<Brian Vaughn>// - **[721238394](facebook/react@721238394 )**: Enable strict effects mode for React Native Facebook builds ([#21354](facebook/react#21354)) //<Brian Vaughn>// - **[48740429b](facebook/react@48740429b )**: Expiration: Do nothing except disable time slicing ([#21345](facebook/react#21345)) //<Andrew Clark>// - **[0f5ebf366](facebook/react@0f5ebf366 )**: Delete unreferenced type ([#21343](facebook/react#21343)) //<Andrew Clark>// - **[9cd52b27f](facebook/react@9cd52b27f )**: Restore context after an error happens ([#21341](facebook/react#21341)) //<Sebastian Markbåge>// - **[ad091759a](facebook/react@ad091759a )**: Revert "Emit reactroot attribute on the first element we discover ([#21154](facebook/react#21154))" ([#21340](facebook/react#21340)) //<Sebastian Markbåge>// - **[709f94841](facebook/react@709f94841 )**: [Fizz] Add FB specific streaming API and build ([#21337](facebook/react#21337)) //<Sebastian Markbåge>// - **[e8cdce40d](facebook/react@e8cdce40d )**: Don't flush sync at end of discreteUpdates ([#21327](facebook/react#21327)) //<Andrew Clark>// - **[a15586001](facebook/react@a15586001 )**: Fix: Don't flush discrete at end of batchedUpdates ([#21229](facebook/react#21229)) //<Andrew Clark>// - **[89847bf6e](facebook/react@89847bf6e )**: Continuous updates should interrupt transitions ([#21323](facebook/react#21323)) //<Andrew Clark>// - **[ef37d55b6](facebook/react@ef37d55b6 )**: Use performConcurrentWorkOnRoot for "sync default" ([#21322](facebook/react#21322)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions a632f7d...2a7bb41 jest_e2e[run_all_tests] Reviewed By: JoshuaGross Differential Revision: D28063006 fbshipit-source-id: 7e3535f80961706863b6c2188ee44b5796b2f000
The outermost `batchedUpdates` call flushes pending sync updates at the end. This was intended for legacy sync mode, but it also happens to flush discrete updates in concurrent mode. Instead, we should only flush sync updates at the end of `batchedUpdates` for legacy roots. Discrete sync updates can wait to flush in the microtask. `discreteUpdates` has the same issue, which is how I originally noticed this, but I'll change that one in a separate commit since it requires updating a few (no longer relevant) internal tests.
The outermost
batchedUpdates
call flushes pending sync updates at the end. This was intended for legacy sync mode, but it also happens to flush discrete updates in concurrent mode.Instead, we should only flush sync updates at the end of
batchedUpdates
for legacy roots. Discrete sync updates can wait to flush in the microtask.discreteUpdates
has the same issue, which is how I originally noticed this, but I'll change that one in a separate commit since it requires updating a few (no longer relevant) internal tests.