Skip to content

Commit 97ec757

Browse files
acdliteacdlite
committed
Put the fix behind a flag
1 parent 4ae46ee commit 97ec757

10 files changed

Lines changed: 137 additions & 116 deletions

‎packages/react-reconciler/src/ReactFiberHooks.js‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ import type {SuspenseConfig} from './ReactFiberSuspenseConfig';
2020
import type {ReactPriorityLevel} from './SchedulerWithReactIntegration';
2121

2222
import ReactSharedInternals from 'shared/ReactSharedInternals';
23+
import {rebasedUpdatesNeverUncommit} from 'shared/ReactFeatureFlags';
2324

2425
import {NoWork} from './ReactFiberExpirationTime';
2526
import {readContext} from './ReactFiberNewContext';
@@ -776,16 +777,17 @@ function updateReducer<S, I, A>(
776777
let isRebasing = rebaseTime !== NoWork;
777778

778779
do {
780+
const updateExpirationTime = update.expirationTime;
779781
if (prevUpdate === rebaseEnd) {
780782
isRebasing = false;
781783
}
782-
const updateExpirationTime = update.expirationTime;
783784
if (
784785
// Check if this update should be skipped
785786
updateExpirationTime < renderExpirationTime &&
786787
// If we're currently rebasing, don't skip this update if we already
787788
// committed it.
788-
(!isRebasing || updateExpirationTime < rebaseTime)
789+
(!rebasedUpdatesNeverUncommit ||
790+
(!isRebasing || updateExpirationTime < rebaseTime))
789791
) {
790792
// Priority is insufficient. Skip this update. If this is the first
791793
// skipped update, the previous update/state is the new base

‎packages/react-reconciler/src/ReactUpdateQueue.js‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -97,7 +97,10 @@ import {
9797
import {Callback, ShouldCapture, DidCapture} from 'shared/ReactSideEffectTags';
9898
import {ClassComponent} from 'shared/ReactWorkTags';
9999

100-
import {debugRenderPhaseSideEffectsForStrictMode} from 'shared/ReactFeatureFlags';
100+
import {
101+
debugRenderPhaseSideEffectsForStrictMode,
102+
rebasedUpdatesNeverUncommit,
103+
} from 'shared/ReactFeatureFlags';
101104

102105
import {StrictMode} from './ReactTypeOfMode';
103106
import {
@@ -479,7 +482,8 @@ export function processUpdateQueue<State>(
479482
updateExpirationTime < renderExpirationTime &&
480483
// If we're currently rebasing, don't skip this update if we already
481484
// committed it.
482-
(!isRebasing || updateExpirationTime < rebaseTime)
485+
(!rebasedUpdatesNeverUncommit ||
486+
(!isRebasing || updateExpirationTime < rebaseTime))
483487
) {
484488
// This update does not have sufficient priority. Skip it.
485489
if (newRebaseTime === NoWork) {

‎packages/react-reconciler/src/__tests__/ReactIncrementalUpdates-test.internal.js‎

Lines changed: 118 additions & 112 deletions
Original file line numberDiff line numberDiff line change
@@ -654,127 +654,133 @@ describe('ReactIncrementalUpdates', () => {
654654
});
655655
});
656656

657-
it('when rebasing, does not exclude updates that were already committed, regardless of priority', async () => {
658-
const {useState, useLayoutEffect} = React;
659-
660-
let pushToLog;
661-
function App() {
662-
const [log, setLog] = useState('');
663-
pushToLog = msg => {
664-
setLog(prevLog => prevLog + msg);
665-
};
657+
it.experimental(
658+
'when rebasing, does not exclude updates that were already committed, regardless of priority',
659+
async () => {
660+
const {useState, useLayoutEffect} = React;
661+
662+
let pushToLog;
663+
function App() {
664+
const [log, setLog] = useState('');
665+
pushToLog = msg => {
666+
setLog(prevLog => prevLog + msg);
667+
};
666668

667-
useLayoutEffect(
668-
() => {
669-
Scheduler.unstable_yieldValue('Committed: ' + log);
670-
if (log === 'B') {
669+
useLayoutEffect(
670+
() => {
671+
Scheduler.unstable_yieldValue('Committed: ' + log);
672+
if (log === 'B') {
673+
// Right after B commits, schedule additional updates.
674+
Scheduler.unstable_runWithPriority(
675+
Scheduler.unstable_UserBlockingPriority,
676+
() => {
677+
pushToLog('C');
678+
},
679+
);
680+
setLog(prevLog => prevLog + 'D');
681+
}
682+
},
683+
[log],
684+
);
685+
686+
return log;
687+
}
688+
689+
const root = ReactNoop.createRoot();
690+
await ReactNoop.act(async () => {
691+
root.render(<App />);
692+
});
693+
expect(Scheduler).toHaveYielded(['Committed: ']);
694+
expect(root).toMatchRenderedOutput('');
695+
696+
await ReactNoop.act(async () => {
697+
pushToLog('A');
698+
Scheduler.unstable_runWithPriority(
699+
Scheduler.unstable_UserBlockingPriority,
700+
() => {
701+
pushToLog('B');
702+
},
703+
);
704+
});
705+
expect(Scheduler).toHaveYielded([
706+
// A and B are pending. B is higher priority, so we'll render that first.
707+
'Committed: B',
708+
// Because A comes first in the queue, we're now in rebase mode. B must
709+
// be rebased on top of A. Also, in a layout effect, we received two new
710+
// updates: C and D. C is user-blocking and D is synchronous.
711+
//
712+
// First render the synchronous update. What we're testing here is that
713+
// B *is not dropped* even though it has lower than sync priority. That's
714+
// because we already committed it. However, this render should not
715+
// include C, because that update wasn't already committed.
716+
'Committed: BD',
717+
'Committed: BCD',
718+
'Committed: ABCD',
719+
]);
720+
expect(root).toMatchRenderedOutput('ABCD');
721+
},
722+
);
723+
724+
it.experimental(
725+
'when rebasing, does not exclude updates that were already committed, regardless of priority (classes)',
726+
async () => {
727+
let pushToLog;
728+
class App extends React.Component {
729+
state = {log: ''};
730+
pushToLog = msg => {
731+
this.setState(prevState => ({log: prevState.log + msg}));
732+
};
733+
componentDidUpdate() {
734+
Scheduler.unstable_yieldValue('Committed: ' + this.state.log);
735+
if (this.state.log === 'B') {
671736
// Right after B commits, schedule additional updates.
672737
Scheduler.unstable_runWithPriority(
673738
Scheduler.unstable_UserBlockingPriority,
674739
() => {
675-
pushToLog('C');
740+
this.pushToLog('C');
676741
},
677742
);
678-
setLog(prevLog => prevLog + 'D');
743+
this.pushToLog('D');
679744
}
680-
},
681-
[log],
682-
);
683-
684-
return log;
685-
}
686-
687-
const root = ReactNoop.createRoot();
688-
await ReactNoop.act(async () => {
689-
root.render(<App />);
690-
});
691-
expect(Scheduler).toHaveYielded(['Committed: ']);
692-
expect(root).toMatchRenderedOutput('');
693-
694-
await ReactNoop.act(async () => {
695-
pushToLog('A');
696-
Scheduler.unstable_runWithPriority(
697-
Scheduler.unstable_UserBlockingPriority,
698-
() => {
699-
pushToLog('B');
700-
},
701-
);
702-
});
703-
expect(Scheduler).toHaveYielded([
704-
// A and B are pending. B is higher priority, so we'll render that first.
705-
'Committed: B',
706-
// Because A comes first in the queue, we're now in rebase mode. B must
707-
// be rebased on top of A. Also, in a layout effect, we received two new
708-
// updates: C and D. C is user-blocking and D is synchronous.
709-
//
710-
// First render the synchronous update. What we're testing here is that
711-
// B *is not dropped* even though it has lower than sync priority. That's
712-
// because we already committed it. However, this render should not
713-
// include C, because that update wasn't already committed.
714-
'Committed: BD',
715-
'Committed: BCD',
716-
'Committed: ABCD',
717-
]);
718-
expect(root).toMatchRenderedOutput('ABCD');
719-
});
720-
721-
it('when rebasing, does not exclude updates that were already committed, regardless of priority (classes)', async () => {
722-
let pushToLog;
723-
class App extends React.Component {
724-
state = {log: ''};
725-
pushToLog = msg => {
726-
this.setState(prevState => ({log: prevState.log + msg}));
727-
};
728-
componentDidUpdate() {
729-
Scheduler.unstable_yieldValue('Committed: ' + this.state.log);
730-
if (this.state.log === 'B') {
731-
// Right after B commits, schedule additional updates.
732-
Scheduler.unstable_runWithPriority(
733-
Scheduler.unstable_UserBlockingPriority,
734-
() => {
735-
this.pushToLog('C');
736-
},
737-
);
738-
this.pushToLog('D');
745+
}
746+
render() {
747+
pushToLog = this.pushToLog;
748+
return this.state.log;
739749
}
740750
}
741-
render() {
742-
pushToLog = this.pushToLog;
743-
return this.state.log;
744-
}
745-
}
746751

747-
const root = ReactNoop.createRoot();
748-
await ReactNoop.act(async () => {
749-
root.render(<App />);
750-
});
751-
expect(Scheduler).toHaveYielded([]);
752-
expect(root).toMatchRenderedOutput('');
753-
754-
await ReactNoop.act(async () => {
755-
pushToLog('A');
756-
Scheduler.unstable_runWithPriority(
757-
Scheduler.unstable_UserBlockingPriority,
758-
() => {
759-
pushToLog('B');
760-
},
761-
);
762-
});
763-
expect(Scheduler).toHaveYielded([
764-
// A and B are pending. B is higher priority, so we'll render that first.
765-
'Committed: B',
766-
// Because A comes first in the queue, we're now in rebase mode. B must
767-
// be rebased on top of A. Also, in a layout effect, we received two new
768-
// updates: C and D. C is user-blocking and D is synchronous.
769-
//
770-
// First render the synchronous update. What we're testing here is that
771-
// B *is not dropped* even though it has lower than sync priority. That's
772-
// because we already committed it. However, this render should not
773-
// include C, because that update wasn't already committed.
774-
'Committed: BD',
775-
'Committed: BCD',
776-
'Committed: ABCD',
777-
]);
778-
expect(root).toMatchRenderedOutput('ABCD');
779-
});
752+
const root = ReactNoop.createRoot();
753+
await ReactNoop.act(async () => {
754+
root.render(<App />);
755+
});
756+
expect(Scheduler).toHaveYielded([]);
757+
expect(root).toMatchRenderedOutput('');
758+
759+
await ReactNoop.act(async () => {
760+
pushToLog('A');
761+
Scheduler.unstable_runWithPriority(
762+
Scheduler.unstable_UserBlockingPriority,
763+
() => {
764+
pushToLog('B');
765+
},
766+
);
767+
});
768+
expect(Scheduler).toHaveYielded([
769+
// A and B are pending. B is higher priority, so we'll render that first.
770+
'Committed: B',
771+
// Because A comes first in the queue, we're now in rebase mode. B must
772+
// be rebased on top of A. Also, in a layout effect, we received two new
773+
// updates: C and D. C is user-blocking and D is synchronous.
774+
//
775+
// First render the synchronous update. What we're testing here is that
776+
// B *is not dropped* even though it has lower than sync priority. That's
777+
// because we already committed it. However, this render should not
778+
// include C, because that update wasn't already committed.
779+
'Committed: BD',
780+
'Committed: BCD',
781+
'Committed: ABCD',
782+
]);
783+
expect(root).toMatchRenderedOutput('ABCD');
784+
},
785+
);
780786
});

‎packages/shared/ReactFeatureFlags.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,3 +92,5 @@ export const enableTrustedTypesIntegration = false;
9292

9393
// Flag to turn event.target and event.currentTarget in ReactNative from a reactTag to a component instance
9494
export const enableNativeTargetAsInstance = false;
95+
96+
export const rebasedUpdatesNeverUncommit = __EXPERIMENTAL__;

‎packages/shared/forks/ReactFeatureFlags.native-fb.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ export const warnAboutStringRefs = false;
4343
export const disableLegacyContext = false;
4444
export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
4545
export const enableTrustedTypesIntegration = false;
46+
export const rebasedUpdatesNeverUncommit = __EXPERIMENTAL__;
4647

4748
// Only used in www builds.
4849
export function addUserTimingListener() {

‎packages/shared/forks/ReactFeatureFlags.native-oss.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ export const disableLegacyContext = false;
3838
export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
3939
export const enableTrustedTypesIntegration = false;
4040
export const enableNativeTargetAsInstance = false;
41+
export const rebasedUpdatesNeverUncommit = __EXPERIMENTAL__;
4142

4243
// Only used in www builds.
4344
export function addUserTimingListener() {

‎packages/shared/forks/ReactFeatureFlags.persistent.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ export const disableLegacyContext = false;
3838
export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
3939
export const enableTrustedTypesIntegration = false;
4040
export const enableNativeTargetAsInstance = false;
41+
export const rebasedUpdatesNeverUncommit = __EXPERIMENTAL__;
4142

4243
// Only used in www builds.
4344
export function addUserTimingListener() {

‎packages/shared/forks/ReactFeatureFlags.test-renderer.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ export const disableLegacyContext = false;
3838
export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
3939
export const enableTrustedTypesIntegration = false;
4040
export const enableNativeTargetAsInstance = false;
41+
export const rebasedUpdatesNeverUncommit = __EXPERIMENTAL__;
4142

4243
// Only used in www builds.
4344
export function addUserTimingListener() {

‎packages/shared/forks/ReactFeatureFlags.test-renderer.www.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ export const disableLegacyContext = false;
3636
export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
3737
export const enableTrustedTypesIntegration = false;
3838
export const enableNativeTargetAsInstance = false;
39+
export const rebasedUpdatesNeverUncommit = __EXPERIMENTAL__;
3940

4041
// Only used in www builds.
4142
export function addUserTimingListener() {

‎packages/shared/forks/ReactFeatureFlags.www.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,8 @@ export const flushSuspenseFallbacksInTests = true;
8787

8888
export const enableNativeTargetAsInstance = false;
8989

90+
export const rebasedUpdatesNeverUncommit = __EXPERIMENTAL__;
91+
9092
// Flow magic to verify the exports of this file match the original version.
9193
// eslint-disable-next-line no-unused-vars
9294
type Check<_X, Y: _X, X: Y = _X> = null;

0 commit comments

Comments
 (0)