From 11a379ae294134ed92aae0c666ae88c483ed7d07 Mon Sep 17 00:00:00 2001
From: Joe Savona <joesavona@fb.com>
Date: Mon, 27 Sep 2021 13:21:03 -0700
Subject: [PATCH 01/10] Fork dispatchAction for useState/useReducer
---
.../src/ReactFiberHooks.new.js | 159 +++++++++++++++++-
1 file changed, 155 insertions(+), 4 deletions(-)
diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js
index 76af5ddf50746..99c45e4a3da71 100644
--- a/packages/react-reconciler/src/ReactFiberHooks.new.js
+++ b/packages/react-reconciler/src/ReactFiberHooks.new.js
@@ -730,7 +730,7 @@ function mountReducer<S, I, A>(
lastRenderedState: (initialState: any),
};
hook.queue = queue;
- const dispatch: Dispatch<A> = (queue.dispatch = (dispatchAction.bind(
+ const dispatch: Dispatch<A> = (queue.dispatch = (dispatchReducerAction.bind(
null,
currentlyRenderingFiber,
queue,
@@ -1190,7 +1190,7 @@ function useMutableSource<Source, Snapshot>(
lastRenderedReducer: basicStateReducer,
lastRenderedState: snapshot,
};
- newQueue.dispatch = setSnapshot = (dispatchAction.bind(
+ newQueue.dispatch = setSnapshot = (dispatchSetState.bind(
null,
currentlyRenderingFiber,
newQueue,
@@ -1481,7 +1481,7 @@ function mountState<S>(
hook.queue = queue;
const dispatch: Dispatch<
BasicStateAction<S>,
- > = (queue.dispatch = (dispatchAction.bind(
+ > = (queue.dispatch = (dispatchSetState.bind(
null,
currentlyRenderingFiber,
queue,
@@ -2150,7 +2150,158 @@ function refreshCache<T>(fiber: Fiber, seedKey: ?() => T, seedValue: T) {
// TODO: Warn if unmounted?
}
-function dispatchAction<S, A>(
+function dispatchReducerAction<S, A>(
+ fiber: Fiber,
+ queue: UpdateQueue<S, A>,
+ action: A,
+) {
+ if (__DEV__) {
+ if (typeof arguments[3] === 'function') {
+ console.error(
+ "State updates from the useState() and useReducer() Hooks don't support the " +
+ 'second callback argument. To execute a side effect after ' +
+ 'rendering, declare it in the component body with useEffect().',
+ );
+ }
+ }
+
+ const eventTime = requestEventTime();
+ const lane = requestUpdateLane(fiber);
+
+ const update: Update<S, A> = {
+ lane,
+ action,
+ eagerReducer: null,
+ eagerState: null,
+ next: (null: any),
+ };
+
+ const alternate = fiber.alternate;
+ if (
+ fiber === currentlyRenderingFiber ||
+ (alternate !== null && alternate === currentlyRenderingFiber)
+ ) {
+ // This is a render phase update. Stash it in a lazily-created map of
+ // queue -> linked list of updates. After this render pass, we'll restart
+ // and apply the stashed updates on top of the work-in-progress hook.
+ didScheduleRenderPhaseUpdateDuringThisPass = didScheduleRenderPhaseUpdate = true;
+ const pending = queue.pending;
+ if (pending === null) {
+ // This is the first update. Create a circular list.
+ update.next = update;
+ } else {
+ update.next = pending.next;
+ pending.next = update;
+ }
+ queue.pending = update;
+ } else {
+ if (isInterleavedUpdate(fiber, lane)) {
+ const interleaved = queue.interleaved;
+ if (interleaved === null) {
+ // This is the first update. Create a circular list.
+ update.next = update;
+ // At the end of the current render, this queue's interleaved updates will
+ // be transferred to the pending queue.
+ pushInterleavedQueue(queue);
+ } else {
+ update.next = interleaved.next;
+ interleaved.next = update;
+ }
+ queue.interleaved = update;
+ } else {
+ const pending = queue.pending;
+ if (pending === null) {
+ // This is the first update. Create a circular list.
+ update.next = update;
+ } else {
+ update.next = pending.next;
+ pending.next = update;
+ }
+ queue.pending = update;
+ }
+
+ if (
+ fiber.lanes === NoLanes &&
+ (alternate === null || alternate.lanes === NoLanes)
+ ) {
+ // The queue is currently empty, which means we can eagerly compute the
+ // next state before entering the render phase. If the new state is the
+ // same as the current state, we may be able to bail out entirely.
+ const lastRenderedReducer = queue.lastRenderedReducer;
+ if (lastRenderedReducer !== null) {
+ let prevDispatcher;
+ if (__DEV__) {
+ prevDispatcher = ReactCurrentDispatcher.current;
+ ReactCurrentDispatcher.current = InvalidNestedHooksDispatcherOnUpdateInDEV;
+ }
+ try {
+ const currentState: S = (queue.lastRenderedState: any);
+ const eagerState = lastRenderedReducer(currentState, action);
+ // Stash the eagerly computed state, and the reducer used to compute
+ // it, on the update object. If the reducer hasn't changed by the
+ // time we enter the render phase, then the eager state can be used
+ // without calling the reducer again.
+ update.eagerReducer = lastRenderedReducer;
+ update.eagerState = eagerState;
+ if (is(eagerState, currentState)) {
+ // Fast path. We can bail out without scheduling React to re-render.
+ // It's still possible that we'll need to rebase this update later,
+ // if the component re-renders for a different reason and by that
+ // time the reducer has changed.
+ return;
+ }
+ } catch (error) {
+ // Suppress the error. It will throw again in the render phase.
+ } finally {
+ if (__DEV__) {
+ ReactCurrentDispatcher.current = prevDispatcher;
+ }
+ }
+ }
+ }
+ if (__DEV__) {
+ // $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests
+ if ('undefined' !== typeof jest) {
+ warnIfNotCurrentlyActingUpdatesInDev(fiber);
+ }
+ }
+ const root = scheduleUpdateOnFiber(fiber, lane, eventTime);
+
+ if (isTransitionLane(lane) && root !== null) {
+ let queueLanes = queue.lanes;
+
+ // If any entangled lanes are no longer pending on the root, then they
+ // must have finished. We can remove them from the shared queue, which
+ // represents a superset of the actually pending lanes. In some cases we
+ // may entangle more than we need to, but that's OK. In fact it's worse if
+ // we *don't* entangle when we should.
+ queueLanes = intersectLanes(queueLanes, root.pendingLanes);
+
+ // Entangle the new transition lane with the other transition lanes.
+ const newQueueLanes = mergeLanes(queueLanes, lane);
+ queue.lanes = newQueueLanes;
+ // Even if queue.lanes already include lane, we don't know for certain if
+ // the lane finished since the last time we entangled it. So we need to
+ // entangle it again, just to be sure.
+ markRootEntangled(root, newQueueLanes);
+ }
+ }
+
+ if (__DEV__) {
+ if (enableDebugTracing) {
+ if (fiber.mode & DebugTracingMode) {
+ const name = getComponentNameFromFiber(fiber) || 'Unknown';
+ logStateUpdateScheduled(name, lane, action);
+ }
+ }
+ }
+
+ if (enableSchedulingProfiler) {
+ markStateUpdateScheduled(fiber, lane);
+ }
+}
+
+function dispatchSetState<S, A>(
fiber: Fiber,
queue: UpdateQueue<S, A>,
action: A,
From 27048d2841085a9fd8491e52667ee75400ae58e1 Mon Sep 17 00:00:00 2001
From: Joe Savona <joesavona@fb.com>
Date: Mon, 27 Sep 2021 13:38:50 -0700
Subject: [PATCH 02/10] Remove eager bailout from forked dispatchReducerAction,
update tests
---
.../src/ReactFiberHooks.new.js | 39 -------------------
.../ReactHooksWithNoopRenderer-test.js | 4 +-
2 files changed, 2 insertions(+), 41 deletions(-)
diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js
index 99c45e4a3da71..0b09f3f36fa5a 100644
--- a/packages/react-reconciler/src/ReactFiberHooks.new.js
+++ b/packages/react-reconciler/src/ReactFiberHooks.new.js
@@ -2220,45 +2220,6 @@ function dispatchReducerAction<S, A>(
queue.pending = update;
}
- if (
- fiber.lanes === NoLanes &&
- (alternate === null || alternate.lanes === NoLanes)
- ) {
- // The queue is currently empty, which means we can eagerly compute the
- // next state before entering the render phase. If the new state is the
- // same as the current state, we may be able to bail out entirely.
- const lastRenderedReducer = queue.lastRenderedReducer;
- if (lastRenderedReducer !== null) {
- let prevDispatcher;
- if (__DEV__) {
- prevDispatcher = ReactCurrentDispatcher.current;
- ReactCurrentDispatcher.current = InvalidNestedHooksDispatcherOnUpdateInDEV;
- }
- try {
- const currentState: S = (queue.lastRenderedState: any);
- const eagerState = lastRenderedReducer(currentState, action);
- // Stash the eagerly computed state, and the reducer used to compute
- // it, on the update object. If the reducer hasn't changed by the
- // time we enter the render phase, then the eager state can be used
- // without calling the reducer again.
- update.eagerReducer = lastRenderedReducer;
- update.eagerState = eagerState;
- if (is(eagerState, currentState)) {
- // Fast path. We can bail out without scheduling React to re-render.
- // It's still possible that we'll need to rebase this update later,
- // if the component re-renders for a different reason and by that
- // time the reducer has changed.
- return;
- }
- } catch (error) {
- // Suppress the error. It will throw again in the render phase.
- } finally {
- if (__DEV__) {
- ReactCurrentDispatcher.current = prevDispatcher;
- }
- }
- }
- }
if (__DEV__) {
// $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests
if ('undefined' !== typeof jest) {
diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
index b687212f2b32c..9aeb40996ce6c 100644
--- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
+++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
@@ -3867,6 +3867,8 @@ describe('ReactHooksWithNoopRenderer', () => {
});
});
+ // TODO: delete now that we removed the eager bailout optimization? or do we want to keep this
+ // around in some capacity?
it('eager bailout optimization should always compare to latest rendered reducer', () => {
// Edge case based on a bug report
let setCounter;
@@ -3898,7 +3900,6 @@ describe('ReactHooksWithNoopRenderer', () => {
'Render: -1',
'Effect: 1',
'Reducer: 1',
- 'Reducer: 1',
'Render: 1',
]);
expect(ReactNoop).toMatchRenderedOutput('1');
@@ -3911,7 +3912,6 @@ describe('ReactHooksWithNoopRenderer', () => {
'Render: 1',
'Effect: 2',
'Reducer: 2',
- 'Reducer: 2',
'Render: 2',
]);
expect(ReactNoop).toMatchRenderedOutput('2');
From 882bbadefc50d763d3bf6c427e861f3f76c9948f Mon Sep 17 00:00:00 2001
From: Joe Savona <joesavona@fb.com>
Date: Mon, 27 Sep 2021 13:48:05 -0700
Subject: [PATCH 03/10] Update eager reducer/state logic to handle state case
only
---
.../src/ReactFiberHooks.new.js | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/packages/react-reconciler/src/ReactFiberHooks.new.js b/packages/react-reconciler/src/ReactFiberHooks.new.js
index 0b09f3f36fa5a..6d7cb75bbd751 100644
--- a/packages/react-reconciler/src/ReactFiberHooks.new.js
+++ b/packages/react-reconciler/src/ReactFiberHooks.new.js
@@ -125,7 +125,7 @@ const {ReactCurrentDispatcher, ReactCurrentBatchConfig} = ReactSharedInternals;
type Update<S, A> = {|
lane: Lane,
action: A,
- eagerReducer: ((S, A) => S) | null,
+ hasEagerState: boolean,
eagerState: S | null,
next: Update<S, A>,
|};
@@ -801,7 +801,7 @@ function updateReducer<S, I, A>(
const clone: Update<S, A> = {
lane: updateLane,
action: update.action,
- eagerReducer: update.eagerReducer,
+ hasEagerState: update.hasEagerState,
eagerState: update.eagerState,
next: (null: any),
};
@@ -829,7 +829,7 @@ function updateReducer<S, I, A>(
// this will never be skipped by the check above.
lane: NoLane,
action: update.action,
- eagerReducer: update.eagerReducer,
+ hasEagerState: update.hasEagerState,
eagerState: update.eagerState,
next: (null: any),
};
@@ -837,9 +837,9 @@ function updateReducer<S, I, A>(
}
// Process this update.
- if (update.eagerReducer === reducer) {
- // If this update was processed eagerly, and its reducer matches the
- // current reducer, we can use the eagerly computed state.
+ if (update.hasEagerState) {
+ // If this update is a state update (not a reducer) and was processed eagerly,
+ // we can use the eagerly computed state
newState = ((update.eagerState: any): S);
} else {
const action = update.action;
@@ -2171,7 +2171,7 @@ function dispatchReducerAction<S, A>(
const update: Update<S, A> = {
lane,
action,
- eagerReducer: null,
+ hasEagerState: false,
eagerState: null,
next: (null: any),
};
@@ -2283,7 +2283,7 @@ function dispatchSetState<S, A>(
const update: Update<S, A> = {
lane,
action,
- eagerReducer: null,
+ hasEagerState: false,
eagerState: null,
next: (null: any),
};
@@ -2353,7 +2353,7 @@ function dispatchSetState<S, A>(
// it, on the update object. If the reducer hasn't changed by the
// time we enter the render phase, then the eager state can be used
// without calling the reducer again.
- update.eagerReducer = lastRenderedReducer;
+ update.hasEagerState = true;
update.eagerState = eagerState;
if (is(eagerState, currentState)) {
// Fast path. We can bail out without scheduling React to re-render.
From e49a79a327e668dd2b706f7797e674159823fad4 Mon Sep 17 00:00:00 2001
From: Joe Savona <joesavona@fb.com>
Date: Mon, 27 Sep 2021 14:00:38 -0700
Subject: [PATCH 04/10] sync reconciler fork
---
.../src/ReactFiberHooks.old.js | 136 ++++++++++++++++--
1 file changed, 124 insertions(+), 12 deletions(-)
diff --git a/packages/react-reconciler/src/ReactFiberHooks.old.js b/packages/react-reconciler/src/ReactFiberHooks.old.js
index b755852fe618d..75570d1c30969 100644
--- a/packages/react-reconciler/src/ReactFiberHooks.old.js
+++ b/packages/react-reconciler/src/ReactFiberHooks.old.js
@@ -125,7 +125,7 @@ const {ReactCurrentDispatcher, ReactCurrentBatchConfig} = ReactSharedInternals;
type Update<S, A> = {|
lane: Lane,
action: A,
- eagerReducer: ((S, A) => S) | null,
+ hasEagerState: boolean,
eagerState: S | null,
next: Update<S, A>,
|};
@@ -730,7 +730,7 @@ function mountReducer<S, I, A>(
lastRenderedState: (initialState: any),
};
hook.queue = queue;
- const dispatch: Dispatch<A> = (queue.dispatch = (dispatchAction.bind(
+ const dispatch: Dispatch<A> = (queue.dispatch = (dispatchReducerAction.bind(
null,
currentlyRenderingFiber,
queue,
@@ -801,7 +801,7 @@ function updateReducer<S, I, A>(
const clone: Update<S, A> = {
lane: updateLane,
action: update.action,
- eagerReducer: update.eagerReducer,
+ hasEagerState: update.hasEagerState,
eagerState: update.eagerState,
next: (null: any),
};
@@ -829,7 +829,7 @@ function updateReducer<S, I, A>(
// this will never be skipped by the check above.
lane: NoLane,
action: update.action,
- eagerReducer: update.eagerReducer,
+ hasEagerState: update.hasEagerState,
eagerState: update.eagerState,
next: (null: any),
};
@@ -837,9 +837,9 @@ function updateReducer<S, I, A>(
}
// Process this update.
- if (update.eagerReducer === reducer) {
- // If this update was processed eagerly, and its reducer matches the
- // current reducer, we can use the eagerly computed state.
+ if (update.hasEagerState) {
+ // If this update is a state update (not a reducer) and was processed eagerly,
+ // we can use the eagerly computed state
newState = ((update.eagerState: any): S);
} else {
const action = update.action;
@@ -1190,7 +1190,7 @@ function useMutableSource<Source, Snapshot>(
lastRenderedReducer: basicStateReducer,
lastRenderedState: snapshot,
};
- newQueue.dispatch = setSnapshot = (dispatchAction.bind(
+ newQueue.dispatch = setSnapshot = (dispatchSetState.bind(
null,
currentlyRenderingFiber,
newQueue,
@@ -1481,7 +1481,7 @@ function mountState<S>(
hook.queue = queue;
const dispatch: Dispatch<
BasicStateAction<S>,
- > = (queue.dispatch = (dispatchAction.bind(
+ > = (queue.dispatch = (dispatchSetState.bind(
null,
currentlyRenderingFiber,
queue,
@@ -2150,7 +2150,7 @@ function refreshCache<T>(fiber: Fiber, seedKey: ?() => T, seedValue: T) {
// TODO: Warn if unmounted?
}
-function dispatchAction<S, A>(
+function dispatchReducerAction<S, A>(
fiber: Fiber,
queue: UpdateQueue<S, A>,
action: A,
@@ -2171,7 +2171,119 @@ function dispatchAction<S, A>(
const update: Update<S, A> = {
lane,
action,
- eagerReducer: null,
+ hasEagerState: false,
+ eagerState: null,
+ next: (null: any),
+ };
+
+ const alternate = fiber.alternate;
+ if (
+ fiber === currentlyRenderingFiber ||
+ (alternate !== null && alternate === currentlyRenderingFiber)
+ ) {
+ // This is a render phase update. Stash it in a lazily-created map of
+ // queue -> linked list of updates. After this render pass, we'll restart
+ // and apply the stashed updates on top of the work-in-progress hook.
+ didScheduleRenderPhaseUpdateDuringThisPass = didScheduleRenderPhaseUpdate = true;
+ const pending = queue.pending;
+ if (pending === null) {
+ // This is the first update. Create a circular list.
+ update.next = update;
+ } else {
+ update.next = pending.next;
+ pending.next = update;
+ }
+ queue.pending = update;
+ } else {
+ if (isInterleavedUpdate(fiber, lane)) {
+ const interleaved = queue.interleaved;
+ if (interleaved === null) {
+ // This is the first update. Create a circular list.
+ update.next = update;
+ // At the end of the current render, this queue's interleaved updates will
+ // be transferred to the pending queue.
+ pushInterleavedQueue(queue);
+ } else {
+ update.next = interleaved.next;
+ interleaved.next = update;
+ }
+ queue.interleaved = update;
+ } else {
+ const pending = queue.pending;
+ if (pending === null) {
+ // This is the first update. Create a circular list.
+ update.next = update;
+ } else {
+ update.next = pending.next;
+ pending.next = update;
+ }
+ queue.pending = update;
+ }
+
+ if (__DEV__) {
+ // $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests
+ if ('undefined' !== typeof jest) {
+ warnIfNotCurrentlyActingUpdatesInDev(fiber);
+ }
+ }
+ const root = scheduleUpdateOnFiber(fiber, lane, eventTime);
+
+ if (isTransitionLane(lane) && root !== null) {
+ let queueLanes = queue.lanes;
+
+ // If any entangled lanes are no longer pending on the root, then they
+ // must have finished. We can remove them from the shared queue, which
+ // represents a superset of the actually pending lanes. In some cases we
+ // may entangle more than we need to, but that's OK. In fact it's worse if
+ // we *don't* entangle when we should.
+ queueLanes = intersectLanes(queueLanes, root.pendingLanes);
+
+ // Entangle the new transition lane with the other transition lanes.
+ const newQueueLanes = mergeLanes(queueLanes, lane);
+ queue.lanes = newQueueLanes;
+ // Even if queue.lanes already include lane, we don't know for certain if
+ // the lane finished since the last time we entangled it. So we need to
+ // entangle it again, just to be sure.
+ markRootEntangled(root, newQueueLanes);
+ }
+ }
+
+ if (__DEV__) {
+ if (enableDebugTracing) {
+ if (fiber.mode & DebugTracingMode) {
+ const name = getComponentNameFromFiber(fiber) || 'Unknown';
+ logStateUpdateScheduled(name, lane, action);
+ }
+ }
+ }
+
+ if (enableSchedulingProfiler) {
+ markStateUpdateScheduled(fiber, lane);
+ }
+}
+
+function dispatchSetState<S, A>(
+ fiber: Fiber,
+ queue: UpdateQueue<S, A>,
+ action: A,
+) {
+ if (__DEV__) {
+ if (typeof arguments[3] === 'function') {
+ console.error(
+ "State updates from the useState() and useReducer() Hooks don't support the " +
+ 'second callback argument. To execute a side effect after ' +
+ 'rendering, declare it in the component body with useEffect().',
+ );
+ }
+ }
+
+ const eventTime = requestEventTime();
+ const lane = requestUpdateLane(fiber);
+
+ const update: Update<S, A> = {
+ lane,
+ action,
+ hasEagerState: false,
eagerState: null,
next: (null: any),
};
@@ -2241,7 +2353,7 @@ function dispatchAction<S, A>(
// it, on the update object. If the reducer hasn't changed by the
// time we enter the render phase, then the eager state can be used
// without calling the reducer again.
- update.eagerReducer = lastRenderedReducer;
+ update.hasEagerState = true;
update.eagerState = eagerState;
if (is(eagerState, currentState)) {
// Fast path. We can bail out without scheduling React to re-render.
From bf219621ca274800cb295f5c3c9b921c5aada4a1 Mon Sep 17 00:00:00 2001
From: Joe Savona <joesavona@fb.com>
Date: Mon, 27 Sep 2021 14:21:59 -0700
Subject: [PATCH 05/10] rename test
---
.../src/__tests__/ReactHooksWithNoopRenderer-test.js | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
index 9aeb40996ce6c..3027c9ad15007 100644
--- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
+++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
@@ -3867,9 +3867,7 @@ describe('ReactHooksWithNoopRenderer', () => {
});
});
- // TODO: delete now that we removed the eager bailout optimization? or do we want to keep this
- // around in some capacity?
- it('eager bailout optimization should always compare to latest rendered reducer', () => {
+ it('useReducer does not eagerly bail out of state updates', () => {
// Edge case based on a bug report
let setCounter;
function App() {
From c01afe40df7ec776f42f713110f91946e0d8ab81 Mon Sep 17 00:00:00 2001
From: Joe Savona <joesavona@fb.com>
Date: Mon, 27 Sep 2021 14:47:43 -0700
Subject: [PATCH 06/10] test cases from #15198
---
.../ReactHooksWithNoopRenderer-test.js | 99 +++++++++++++++++++
1 file changed, 99 insertions(+)
diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
index 3027c9ad15007..544b3a650ef3e 100644
--- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
+++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
@@ -3915,6 +3915,105 @@ describe('ReactHooksWithNoopRenderer', () => {
expect(ReactNoop).toMatchRenderedOutput('2');
});
+ it('successful eager bailout should not store dispatched item in the queue', () => {
+ let setDisabled;
+ let increment;
+
+ function Counter({disabled}) {
+ const [count, dispatch] = useReducer((state, action) => {
+ if (disabled) {
+ return state;
+ }
+ if (action.type === 'increment') {
+ return state + 1;
+ }
+ return state;
+ }, 0);
+
+ increment = () => dispatch({type: 'increment'});
+
+ Scheduler.unstable_yieldValue('Render count: ' + count);
+ return count;
+ }
+
+ function App() {
+ const [disabled, _setDisabled] = useState(true);
+ setDisabled = _setDisabled;
+ Scheduler.unstable_yieldValue('Render disabled: ' + disabled);
+ return <Counter disabled={disabled} />;
+ }
+
+ ReactNoop.render(<App />);
+ expect(Scheduler).toFlushAndYield([
+ 'Render disabled: true',
+ 'Render count: 0',
+ ]);
+ expect(ReactNoop).toMatchRenderedOutput('0');
+
+ act(() => {
+ increment();
+ increment();
+ increment();
+ });
+ expect(Scheduler).toHaveYielded(['Render count: 0']);
+ expect(ReactNoop).toMatchRenderedOutput('0');
+
+ act(() => {
+ setDisabled(false);
+ });
+ expect(Scheduler).toHaveYielded([
+ 'Render disabled: false',
+ 'Render count: 0',
+ ]);
+ expect(ReactNoop).toMatchRenderedOutput('0');
+ });
+
+ it('eager bailout should get ignored in batch update when parent rerenders with new props', () => {
+ let setDisabled;
+ let increment;
+
+ function Counter({disabled}) {
+ const [count, dispatch] = useReducer((state, action) => {
+ if (disabled) {
+ return state;
+ }
+ if (action.type === 'increment') {
+ return state + 1;
+ }
+ return state;
+ }, 0);
+
+ increment = () => dispatch({type: 'increment'});
+
+ Scheduler.unstable_yieldValue('Render count: ' + count);
+ return count;
+ }
+
+ function App() {
+ const [disabled, _setDisabled] = useState(true);
+ setDisabled = _setDisabled;
+ Scheduler.unstable_yieldValue('Render disabled: ' + disabled);
+ return <Counter disabled={disabled} />;
+ }
+
+ ReactNoop.render(<App />);
+ expect(Scheduler).toFlushAndYield([
+ 'Render disabled: true',
+ 'Render count: 0',
+ ]);
+ expect(ReactNoop).toMatchRenderedOutput('0');
+
+ act(() => {
+ increment();
+ setDisabled(false);
+ });
+ expect(Scheduler).toHaveYielded([
+ 'Render disabled: false',
+ 'Render count: 1',
+ ]);
+ expect(ReactNoop).toMatchRenderedOutput('1');
+ });
+
// Regression test. Covers a case where an internal state variable
// (`didReceiveUpdate`) is not reset properly.
it('state bail out edge case (#16359)', async () => {
From b6cabe8592208ef1df76b76a057b5802dcbc062b Mon Sep 17 00:00:00 2001
From: Joe Savona <joesavona@fb.com>
Date: Mon, 27 Sep 2021 15:00:26 -0700
Subject: [PATCH 07/10] comments on new test cases
---
.../src/__tests__/ReactHooksWithNoopRenderer-test.js | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
index 544b3a650ef3e..26bdfe9fb9da7 100644
--- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
+++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
@@ -3915,7 +3915,7 @@ describe('ReactHooksWithNoopRenderer', () => {
expect(ReactNoop).toMatchRenderedOutput('2');
});
- it('successful eager bailout should not store dispatched item in the queue', () => {
+ it('useReducer does not replay previous no-op actions when props change', () => {
let setDisabled;
let increment;
@@ -3951,6 +3951,7 @@ describe('ReactHooksWithNoopRenderer', () => {
expect(ReactNoop).toMatchRenderedOutput('0');
act(() => {
+ // These increments should have no effect, since disabled=true
increment();
increment();
increment();
@@ -3959,6 +3960,7 @@ describe('ReactHooksWithNoopRenderer', () => {
expect(ReactNoop).toMatchRenderedOutput('0');
act(() => {
+ // Enabling the updater should *not* replay the previous increment() actions
setDisabled(false);
});
expect(Scheduler).toHaveYielded([
@@ -3968,7 +3970,7 @@ describe('ReactHooksWithNoopRenderer', () => {
expect(ReactNoop).toMatchRenderedOutput('0');
});
- it('eager bailout should get ignored in batch update when parent rerenders with new props', () => {
+ it('useReducer should apply potential no-op changes negated by other updates in the batch', () => {
let setDisabled;
let increment;
@@ -4004,6 +4006,9 @@ describe('ReactHooksWithNoopRenderer', () => {
expect(ReactNoop).toMatchRenderedOutput('0');
act(() => {
+ // Although the increment happens first, because these calls are in a batch
+ // the parent updates first, updating the child with disabled=false. The
+ // increment should therefore take effect and render the updated count.
increment();
setDisabled(false);
});
From 895e2a7b23c5f4f4ebcc854879f9e21bc631ea84 Mon Sep 17 00:00:00 2001
From: Joe Savona <joesavona@fb.com>
Date: Mon, 27 Sep 2021 15:03:21 -0700
Subject: [PATCH 08/10] comments on new test cases
---
.../src/__tests__/ReactHooksWithNoopRenderer-test.js | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
index 26bdfe9fb9da7..b8a39c5e907f0 100644
--- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
+++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
@@ -3970,7 +3970,7 @@ describe('ReactHooksWithNoopRenderer', () => {
expect(ReactNoop).toMatchRenderedOutput('0');
});
- it('useReducer should apply potential no-op changes negated by other updates in the batch', () => {
+ it('useReducer should apply potential no-op changes if made relevant by other updates in the batch', () => {
let setDisabled;
let increment;
@@ -4006,9 +4006,10 @@ describe('ReactHooksWithNoopRenderer', () => {
expect(ReactNoop).toMatchRenderedOutput('0');
act(() => {
- // Although the increment happens first, because these calls are in a batch
- // the parent updates first, updating the child with disabled=false. The
- // increment should therefore take effect and render the updated count.
+ // Although the increment happens first (and would seem to do nothing since disabled=true),
+ // because these calls are in a batch the parent updates first. This should cause the child
+ // to re-render with disabled=false and *then* process the increment action, which now
+ // increments the count and causes the component output to change.
increment();
setDisabled(false);
});
From 1e621bf25d9f57a456ac897d9da7cca8b99772e7 Mon Sep 17 00:00:00 2001
From: Joe Savona <joesavona@fb.com>
Date: Mon, 27 Sep 2021 15:25:06 -0700
Subject: [PATCH 09/10] test case from #21419
---
.../ReactHooksWithNoopRenderer-test.js | 54 +++++++++++++++++++
1 file changed, 54 insertions(+)
diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
index b8a39c5e907f0..b82df5dbc2470 100644
--- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
+++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
@@ -3915,6 +3915,60 @@ describe('ReactHooksWithNoopRenderer', () => {
expect(ReactNoop).toMatchRenderedOutput('2');
});
+ it('useReducer does not replay previous no-op actions when other state changes', () => {
+ let increment;
+ let setDisabled;
+
+ function Counter() {
+ const [disabled, _setDisabled] = useState(true);
+ const [count, dispatch] = useReducer((state, action) => {
+ if (disabled) {
+ return state;
+ }
+ if (action.type === 'increment') {
+ return state + 1;
+ }
+ return state;
+ }, 0);
+
+ increment = () => dispatch({type: 'increment'});
+ setDisabled = _setDisabled;
+
+ Scheduler.unstable_yieldValue('Render disabled: ' + disabled);
+ Scheduler.unstable_yieldValue('Render count: ' + count);
+ return count;
+ }
+
+ ReactNoop.render(<Counter />);
+ expect(Scheduler).toFlushAndYield([
+ 'Render disabled: true',
+ 'Render count: 0',
+ ]);
+ expect(ReactNoop).toMatchRenderedOutput('0');
+
+ act(() => {
+ // These increments should have no effect, since disabled=true
+ increment();
+ increment();
+ increment();
+ });
+ expect(Scheduler).toHaveYielded([
+ 'Render disabled: true',
+ 'Render count: 0',
+ ]);
+ expect(ReactNoop).toMatchRenderedOutput('0');
+
+ act(() => {
+ // Enabling the updater should *not* replay the previous increment() actions
+ setDisabled(false);
+ });
+ expect(Scheduler).toHaveYielded([
+ 'Render disabled: false',
+ 'Render count: 0',
+ ]);
+ expect(ReactNoop).toMatchRenderedOutput('0');
+ });
+
it('useReducer does not replay previous no-op actions when props change', () => {
let setDisabled;
let increment;
From 432cf947ed13bb3efaa4728e71e77f1cab9de6b1 Mon Sep 17 00:00:00 2001
From: Joe Savona <joesavona@fb.com>
Date: Mon, 27 Sep 2021 15:41:43 -0700
Subject: [PATCH 10/10] minor tweak to test name to kick CI
---
.../src/__tests__/ReactHooksWithNoopRenderer-test.js | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
index b82df5dbc2470..f56577e8ffea9 100644
--- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
+++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
@@ -4024,7 +4024,7 @@ describe('ReactHooksWithNoopRenderer', () => {
expect(ReactNoop).toMatchRenderedOutput('0');
});
- it('useReducer should apply potential no-op changes if made relevant by other updates in the batch', () => {
+ it('useReducer applies potential no-op changes if made relevant by other updates in the batch', () => {
let setDisabled;
let increment;