Remove usereducer eager bailout #22445

josephsavona · 2021-09-27T20:59:18Z

Remove eager bailout mechanism for useReducer

Both useState and userReducer have an eager bailout mechanism that attempts to avoid a re-render by checking if the state/reducer update is a no-op. Per @acdlite there are some flaws with the eager bailout mechanism for useReducer specifically, so this PR removes the eager bailout for useReducer and keeps it for useState.

Changes:

Forked dispatchAction() into dispatchReducerAction() and dispatchSetState(), updated call sites to use one of the new names.
Removed the eager bailout mechanism from dispatchReducerAction().
Remove eagerReducer from the Update type, replacing it with hasEagerState to tell if there's an eager state update.

How did you test this change?

Followed the contribution guidelines: yarn test/lint/flow

Closes #15088
Closes #21419
Closes #21416
Closes #17953

acdlite · 2021-09-27T21:01:55Z

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?


Yeah could be useful as a way to document the current behavior, or in case we ever add it back. You can retitle to "useReducer does not eagerly bail out of state updates."

acdlite

Looks great! Good to merge assuming CI passes

gaearon · 2021-09-27T21:23:13Z

Does this make tests from #15198 pass?

gaearon · 2021-09-27T21:24:04Z

Another possible regression test that may be worth including is in #21419

josephsavona · 2021-09-27T21:48:07Z

Does this make tests from #15198 pass?

@gaearon Yup! I just added them.

gaearon · 2021-09-27T21:57:20Z

Did you verify they failed on main? Just to make sure this is actually the fix for that problem

josephsavona · 2021-09-27T22:07:35Z

Did you verify they failed on main? Just to make sure this is actually the fix for that problem

Oops no - i went back and checked and the first new test case fails on main.

josephsavona · 2021-09-27T22:09:24Z

packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js

+    expect(ReactNoop).toMatchRenderedOutput('0');
+  });
+
+  it('useReducer should apply potential no-op changes if made relevant by other updates in the batch', () => {


This passed on main, but it seems like a good test of priority of eager bailout vs updates from a parent.

josephsavona · 2021-09-27T22:09:59Z

packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js

+    });
+    expect(Scheduler).toHaveYielded([
+      'Render disabled: false',
+      'Render count: 0',


this failed on main, with 'Render count: 3'

josephsavona · 2021-09-27T22:27:48Z

I confirmed that the test from #21419 repros on main and is fixed by this change

josephsavona · 2021-09-27T22:42:35Z

@acdlite, @gaearon any idea why CI is failing with "Error: Could not find commit for: 1e621bf"?

acdlite · 2021-09-27T23:18:59Z

The CodeSandbox CI job has been a bit flaky lately, I'll look into that. It's not merge blocking, though.

acdlite · 2021-09-27T23:25:14Z

Thanks for working on this, @josephsavona!

As a follow up to facebook#22445, this extracts the queueing logic that is shared between `dispatchSetState` and `dispatchReducerAction` into separate functions. It likely doesn't save any bytes since these will get inlined, anyway, but it does make the flow a bit easier to follow.

As a follow up to #22445, this extracts the queueing logic that is shared between `dispatchSetState` and `dispatchReducerAction` into separate functions. It likely doesn't save any bytes since these will get inlined, anyway, but it does make the flow a bit easier to follow.

Summary: This sync includes the following changes: - **[579c008a7](facebook/react@579c008a7 )**: [Fizz/Flight] pipeToNodeWritable(..., writable).startWriting() -> renderToPipeableStream(...).pipe(writable) ([#22450](facebook/react#22450)) //<Sebastian Markbåge>// - **[f2c381131](facebook/react@f2c381131 )**: fix: useSyncExternalStoreExtra ([#22500](facebook/react#22500)) //<Daishi Kato>// - **[0ecbbe142](facebook/react@0ecbbe142 )**: Sync hydrate discrete events in capture phase and dont replay discrete events ([#22448](facebook/react#22448)) //<salazarm>// - **[a724a3b57](facebook/react@a724a3b57 )**: [RFC] Codemod invariant -> throw new Error ([#22435](facebook/react#22435)) //<Andrew Clark>// - **[201af81b0](facebook/react@201af81b0 )**: Release pooled cache reference in complete/unwind ([#22464](facebook/react#22464)) //<Joseph Savona>// - **[033efe731](facebook/react@033efe731 )**: Call get snapshot in useSyncExternalStore server shim ([#22453](facebook/react#22453)) //<salazarm>// - **[7843b142a](facebook/react@7843b142a )**: [Fizz/Flight] Pass in Destination lazily to startFlowing instead of in createRequest ([#22449](facebook/react#22449)) //<Sebastian Markbåge>// - **[d9fb383d6](facebook/react@d9fb383d6 )**: Extract queueing logic into shared functions ([#22452](facebook/react#22452)) //<Andrew Clark>// - **[9175f4d15](facebook/react@9175f4d15 )**: Scheduling Profiler: Show Suspense resource .displayName ([#22451](facebook/react#22451)) //<Brian Vaughn>// - **[eba248c39](facebook/react@eba248c39 )**: [Fizz/Flight] Remove reentrancy hack ([#22446](facebook/react#22446)) //<Sebastian Markbåge>// - **[66388150e](facebook/react@66388150e )**: Remove usereducer eager bailout ([#22445](facebook/react#22445)) //<Joseph Savona>// - **[d3e086932](facebook/react@d3e086932 )**: Make root.unmount() synchronous ([#22444](facebook/react#22444)) //<Andrew Clark>// - **[2cc6d79c9](facebook/react@2cc6d79c9 )**: Rename onReadyToStream to onCompleteShell ([#22443](facebook/react#22443)) //<Sebastian Markbåge>// - **[c88fb49d3](facebook/react@c88fb49d3 )**: Improve DEV errors if string coercion throws (Temporal.*, Symbol, etc.) ([#22064](facebook/react#22064)) //<Justin Grant>// - **[05726d72c](facebook/react@05726d72c )**: [Fix] Errors should not "unsuspend" a transition ([#22423](facebook/react#22423)) //<Andrew Clark>// - **[3746eaf98](facebook/react@3746eaf98 )**: Packages/React/src/ReactLazy ---> changing -1 to unintialized ([#22421](facebook/react#22421)) //<BIKI DAS>// - **[04ccc01d9](facebook/react@04ccc01d9 )**: Hydration errors should force a client render ([#22416](facebook/react#22416)) //<Andrew Clark>// - **[029fdcebb](facebook/react@029fdcebb )**: root.hydrate -> root.isDehydrated ([#22420](facebook/react#22420)) //<Andrew Clark>// - **[af87f5a83](facebook/react@af87f5a83 )**: Scheduling Profiler marks should include thrown Errors ([#22417](facebook/react#22417)) //<Brian Vaughn>// - **[d47339ea3](facebook/react@d47339ea3 )**: [Fizz] Remove assignID mechanism ([#22410](facebook/react#22410)) //<Sebastian Markbåge>// - **[3a50d9557](facebook/react@3a50d9557 )**: Never attach ping listeners in legacy Suspense ([#22407](facebook/react#22407)) //<Andrew Clark>// - **[82c8fa90b](facebook/react@82c8fa90b )**: Add back useMutableSource temporarily ([#22396](facebook/react#22396)) //<Andrew Clark>// - **[5b57bc6e3](facebook/react@5b57bc6e3 )**: [Draft] don't patch console during first render ([#22308](facebook/react#22308)) //<Luna Ruan>// - **[cf07c3df1](facebook/react@cf07c3df1 )**: Delete all but one `build2` reference ([#22391](facebook/react#22391)) //<Andrew Clark>// - **[bb0d06935](facebook/react@bb0d06935 )**: [build2 -> build] Local scripts //<Andrew Clark>// - **[0c81d347b](facebook/react@0c81d347b )**: Write artifacts to `build` instead of `build2` //<Andrew Clark>// - **[4da03c9fb](facebook/react@4da03c9fb )**: useSyncExternalStore React Native version ([#22367](facebook/react#22367)) //<salazarm>// - **[48d475c9e](facebook/react@48d475c9e )**: correct typos ([#22294](facebook/react#22294)) //<Bowen>// - **[cb6c619c0](facebook/react@cb6c619c0 )**: Remove Fiber fields that were used for hydrating useMutableSource ([#22368](facebook/react#22368)) //<Sebastian Silbermann>// - **[64e70f82e](facebook/react@64e70f82e )**: [Fizz] add avoidThisFallback support ([#22318](facebook/react#22318)) //<salazarm>// - **[3ee7a004e](facebook/react@3ee7a004e )**: devtools: Display actual ReactDOM API name in root type ([#22363](facebook/react#22363)) //<Sebastian Silbermann>// - **[79b8fc667](facebook/react@79b8fc667 )**: Implement getServerSnapshot in userspace shim ([#22359](facebook/react#22359)) //<Andrew Clark>// - **[86b3e2461](facebook/react@86b3e2461 )**: Implement useSyncExternalStore on server ([#22347](facebook/react#22347)) //<Andrew Clark>// - **[8209de269](facebook/react@8209de269 )**: Delete useMutableSource implementation ([#22292](facebook/react#22292)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions e8feb11...afcb9cd jest_e2e[run_all_tests] Reviewed By: yungsters Differential Revision: D31541359 fbshipit-source-id: c35941bc303fdf55cb061e9996200dc868a6f2af

* Fork dispatchAction for useState/useReducer * Remove eager bailout from forked dispatchReducerAction, update tests * Update eager reducer/state logic to handle state case only * sync reconciler fork * rename test * test cases from facebook#15198 * comments on new test cases * comments on new test cases * test case from facebook#21419 * minor tweak to test name to kick CI

As a follow up to facebook#22445, this extracts the queueing logic that is shared between `dispatchSetState` and `dispatchReducerAction` into separate functions. It likely doesn't save any bytes since these will get inlined, anyway, but it does make the flow a bit easier to follow.

…bailout change React 18 removed some logic from `useReducer()` where a dispatch would eagerly call reducers to recompute the state so now reducers will be called after render. This poses a problem for `useTable()` because during render it mutates the `instance` by spreading in `props` and does not expect this to happen before reducer execution. We were passing a `pageCount: undefined` prop to `useTable()` when the reducer executes `instance.pageCount` is been overwritten, causing a bug in our app. In the Codesandbox reproduction you can see that hitting the "next" button will not work. I understand that our abstraction around `useTable()` probably shouldn't be passing this prop when `undefined` but the behavior change was unexpected and I wonder if it breaks any other assumptions that this library makes. The fix I made here is to just replace `useReducer` with `useState`, which still has the eager bailout check. React PR in question: facebook/react#22445 Codesandbox repro: https://codesandbox.io/s/cool-edison-45tkqx?file=/src/App.js:1331-1358

React 18 removed some logic from `useReducer()` where a dispatch would eagerly call reducers to recompute the state so now reducers will be called after render. This poses a problem for `useTable()` because during render it mutates the `instance` by spreading in `props` and does not expect this to happen before reducer execution. We were passing a `pageCount: undefined` prop to `useTable()` when the reducer executes `instance.pageCount` is been overwritten, causing a bug in our app. In the Codesandbox reproduction you can see that hitting the "next" button will not work. I understand that our abstraction around `useTable()` probably shouldn't be passing this prop when `undefined` but the behavior change was unexpected and I wonder if it breaks any other assumptions that this library makes. The fix I made here is to just replace `useReducer` with `useState`, which still has the eager bailout check. React PR in question: facebook/react#22445 Codesandbox repro: https://codesandbox.io/s/cool-edison-45tkqx?file=/src/App.js:1331-1358

React 18 removed some logic from `useReducer()` where a dispatch would eagerly call reducers to recompute the state so now reducers will be called after render. This poses a problem for `useTable()` because during render it mutates the `instance` by spreading in `props` and does not expect this to happen before reducer execution. We were passing a `pageCount: undefined` prop to `useTable()` and by the time the reducer executes `instance.pageCount` is been overwritten, causing a bug in our app. In the Codesandbox reproduction you can see that hitting the "next" button will not work. I understand that our abstraction around `useTable()` probably shouldn't be passing this prop when `undefined` but the behavior change was unexpected and I wonder if it breaks any other assumptions that this library makes. The fix I made here is to just replace `useReducer` with `useState`, which still has the eager bailout check. React PR in question: facebook/react#22445 Codesandbox repro: https://codesandbox.io/s/cool-edison-45tkqx?file=/src/App.js:1331-1358

…4207) React 18 removed some logic from `useReducer()` where a dispatch would eagerly call reducers to recompute the state so now reducers will be called after render. This poses a problem for `useTable()` because during render it mutates the `instance` by spreading in `props` and does not expect this to happen before reducer execution. We were passing a `pageCount: undefined` prop to `useTable()` and by the time the reducer executes `instance.pageCount` is been overwritten, causing a bug in our app. In the Codesandbox reproduction you can see that hitting the "next" button will not work. I understand that our abstraction around `useTable()` probably shouldn't be passing this prop when `undefined` but the behavior change was unexpected and I wonder if it breaks any other assumptions that this library makes. The fix I made here is to just replace `useReducer` with `useState`, which still has the eager bailout check. React PR in question: facebook/react#22445 Codesandbox repro: https://codesandbox.io/s/cool-edison-45tkqx?file=/src/App.js:1331-1358

josephsavona added 3 commits September 27, 2021 13:21

Fork dispatchAction for useState/useReducer

11a379a

Remove eager bailout from forked dispatchReducerAction, update tests

27048d2

Update eager reducer/state logic to handle state case only

882bbad

josephsavona requested a review from acdlite September 27, 2021 20:59

sync reconciler fork

e49a79a

facebook-github-bot added the CLA Signed label Sep 27, 2021

acdlite reviewed Sep 27, 2021

View reviewed changes

acdlite approved these changes Sep 27, 2021

View reviewed changes

rename test

bf21962

test cases from facebook#15198

c01afe4

josephsavona added 2 commits September 27, 2021 15:00

comments on new test cases

b6cabe8

comments on new test cases

895e2a7

josephsavona commented Sep 27, 2021

View reviewed changes

test case from facebook#21419

1e621bf

minor tweak to test name to kick CI

432cf94

acdlite merged commit 6638815 into facebook:main Sep 27, 2021

acdlite mentioned this pull request Sep 28, 2021

Extract queueing logic into shared functions #22452

Merged

markerikson mentioned this pull request Oct 22, 2021

[Early Preview] New React Documentation reactjs/react.dev#3965

Merged

gaearon mentioned this pull request Mar 29, 2022

Add React 18 changelog #24195

Merged

markerikson mentioned this pull request May 28, 2022

Bug: useReducer and same state in React 18 #24596

Closed

henryqdineen mentioned this pull request Jul 22, 2022

fix(useTable): work around React 18 useReducer eager bailout change TanStack/table#4207

Merged

hhkk28 mentioned this pull request Feb 23, 2023

Bug: useReducer rendering behaviour in React 18 #26131

Closed

rickhanlonii mentioned this pull request Feb 10, 2024

Bug: Parent rerenders unnecessarily on Child-to-Parent setter call, while child rerenders only when changes occur. #28287

Closed

layershifter mentioned this pull request May 2, 2024

fix: useContextSelector with React 18 microsoft/fluentui#30951

Merged

NMinhNguyen mentioned this pull request May 15, 2024

Bug: memory leak in useReducer #21692

Closed

knaveenkumar3576 mentioned this pull request Jun 24, 2024

Faster Paint with delayed flush of Effects clauderic/dnd-kit#1435

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove usereducer eager bailout #22445

Remove usereducer eager bailout #22445

josephsavona commented Sep 27, 2021 •

edited

Loading

acdlite Sep 27, 2021

acdlite left a comment

gaearon commented Sep 27, 2021

gaearon commented Sep 27, 2021

josephsavona commented Sep 27, 2021

gaearon commented Sep 27, 2021

josephsavona commented Sep 27, 2021

josephsavona Sep 27, 2021

josephsavona Sep 27, 2021

josephsavona commented Sep 27, 2021

josephsavona commented Sep 27, 2021

acdlite commented Sep 27, 2021 •

edited

Loading

acdlite commented Sep 27, 2021

Remove usereducer eager bailout #22445

Remove usereducer eager bailout #22445

Conversation

josephsavona commented Sep 27, 2021 • edited Loading

Remove eager bailout mechanism for useReducer

How did you test this change?

acdlite Sep 27, 2021

Choose a reason for hiding this comment

acdlite left a comment

Choose a reason for hiding this comment

gaearon commented Sep 27, 2021

gaearon commented Sep 27, 2021

josephsavona commented Sep 27, 2021

gaearon commented Sep 27, 2021

josephsavona commented Sep 27, 2021

josephsavona Sep 27, 2021

Choose a reason for hiding this comment

josephsavona Sep 27, 2021

Choose a reason for hiding this comment

josephsavona commented Sep 27, 2021

josephsavona commented Sep 27, 2021

acdlite commented Sep 27, 2021 • edited Loading

acdlite commented Sep 27, 2021

josephsavona commented Sep 27, 2021 •

edited

Loading

acdlite commented Sep 27, 2021 •

edited

Loading