From d2a05161b3c2ace8b05359cfaa48a0b68fe011f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Burzy=C5=84ski?= Date: Sat, 23 Mar 2019 10:12:14 +0100 Subject: [PATCH 1/3] Add failing test for bailouted actions being preserved in the queue --- ...eactHooksWithNoopRenderer-test.internal.js | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index a1e12ca463750..84553b4e9e70e 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -2071,4 +2071,57 @@ describe('ReactHooksWithNoopRenderer', () => { expect(Scheduler).toFlushAndYield(['Step: 5, Shadow: 5']); expect(ReactNoop).toMatchRenderedOutput('5'); }); + + 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.yieldValue('Render count: ' + count); + return count; + } + + function App() { + const [disabled, _setDisabled] = useState(true); + setDisabled = _setDisabled; + Scheduler.yieldValue('Render disabled: ' + disabled); + return ; + } + + ReactNoop.render(); + expect(Scheduler).toFlushAndYield([ + 'Render disabled: true', + 'Render count: 0', + ]); + expect(ReactNoop).toMatchRenderedOutput('0'); + + act(() => { + increment(); + increment(); + increment(); + }); + expect(Scheduler).toFlushAndYield([]); + expect(ReactNoop).toMatchRenderedOutput('0'); + + act(() => { + setDisabled(false); + }); + expect(Scheduler).toFlushAndYield([ + 'Render disabled: false', + 'Render count: 0', + ]); + expect(ReactNoop).toMatchRenderedOutput('0'); + }); }); From 7a778065cb3628470ef5ad7d09f963d661113cbb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Burzy=C5=84ski?= Date: Sat, 23 Mar 2019 11:39:53 +0100 Subject: [PATCH 2/3] Do not keep bailouted actions which might lead to reducer computing wrong value on update later --- .../react-reconciler/src/ReactFiberHooks.js | 31 ++++++++++--------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 57d8a7c4edb71..755f89e8d1bd6 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -1123,21 +1123,6 @@ function dispatchAction( next: null, }; - // Append the update to the end of the list. - const last = queue.last; - if (last === null) { - // This is the first update. Create a circular list. - update.next = update; - } else { - const first = last.next; - if (first !== null) { - // Still circular. - update.next = first; - } - last.next = update; - } - queue.last = update; - if ( fiber.expirationTime === NoWork && (alternate === null || alternate.expirationTime === NoWork) @@ -1182,6 +1167,22 @@ function dispatchAction( warnIfNotCurrentlyBatchingInDev(fiber); } } + + // Append the update to the end of the list. + const last = queue.last; + if (last === null) { + // This is the first update. Create a circular list. + update.next = update; + } else { + const first = last.next; + if (first !== null) { + // Still circular. + update.next = first; + } + last.next = update; + } + queue.last = update; + scheduleWork(fiber, expirationTime); } } From 1a2ce0291080f0005715bb4c0c14f056febcab37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mateusz=20Burzy=C5=84ski?= Date: Sun, 24 Mar 2019 11:19:46 +0100 Subject: [PATCH 3/3] Add failing test for eager bailout being ignored in batch update when parent rerenders with new props --- ...eactHooksWithNoopRenderer-test.internal.js | 46 +++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index 84553b4e9e70e..c6c83191d02f7 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -2124,4 +2124,50 @@ describe('ReactHooksWithNoopRenderer', () => { ]); 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.yieldValue('Render count: ' + count); + return count; + } + + function App() { + const [disabled, _setDisabled] = useState(true); + setDisabled = _setDisabled; + Scheduler.yieldValue('Render disabled: ' + disabled); + return ; + } + + ReactNoop.render(); + expect(Scheduler).toFlushAndYield([ + 'Render disabled: true', + 'Render count: 0', + ]); + expect(ReactNoop).toMatchRenderedOutput('0'); + + act(() => { + increment(); + setDisabled(false); + }); + expect(Scheduler).toFlushAndYield([ + 'Render disabled: false', + 'Render count: 1', + ]); + expect(ReactNoop).toMatchRenderedOutput('1'); + }); });