From 33e59ec941a5218c959e4bba3b6ae7891836fc8c Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Sat, 19 Jan 2019 20:29:27 +0000 Subject: [PATCH 1/3] Double-render functions in strict mode --- .../src/ReactFiberBeginWork.js | 29 +++++++++++++++++++ ...rorBoundaryReconciliation-test.internal.js | 2 +- .../__tests__/ReactTestRendererAsync-test.js | 9 +++++- .../react/src/__tests__/forwardRef-test.js | 28 +++++++++--------- 4 files changed, 52 insertions(+), 16 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index a00b795b16dd4..e1beb80dde8b7 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -250,6 +250,20 @@ function updateForwardRef( ref, renderExpirationTime, ); + if ( + debugRenderPhaseSideEffects || + (debugRenderPhaseSideEffectsForStrictMode && + workInProgress.mode & StrictMode) + ) { + renderWithHooks( + current, + workInProgress, + render, + nextProps, + ref, + renderExpirationTime, + ); + } setCurrentPhase(null); } else { nextChildren = renderWithHooks( @@ -543,6 +557,20 @@ function updateFunctionComponent( context, renderExpirationTime, ); + if ( + debugRenderPhaseSideEffects || + (debugRenderPhaseSideEffectsForStrictMode && + workInProgress.mode & StrictMode) + ) { + renderWithHooks( + current, + workInProgress, + Component, + nextProps, + context, + renderExpirationTime, + ); + } setCurrentPhase(null); } else { nextChildren = renderWithHooks( @@ -1145,6 +1173,7 @@ function mountIndeterminateComponent( context, renderExpirationTime, ); + // TODO: double-render here in strict mode. } else { value = renderWithHooks( null, diff --git a/packages/react-reconciler/src/__tests__/ErrorBoundaryReconciliation-test.internal.js b/packages/react-reconciler/src/__tests__/ErrorBoundaryReconciliation-test.internal.js index 66ddccf894d4d..7962e285bf155 100644 --- a/packages/react-reconciler/src/__tests__/ErrorBoundaryReconciliation-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ErrorBoundaryReconciliation-test.internal.js @@ -70,7 +70,7 @@ describe('ErrorBoundaryReconciliation', () => { if (isConcurrent) { renderer.unstable_flushAll(); } - }).toWarnDev(isConcurrent ? ['invalid', 'invalid'] : ['invalid']); + }).toWarnDev(isConcurrent ? ['invalid', 'invalid', 'invalid', 'invalid'] : ['invalid']); const Fallback = fallbackTagName; expect(renderer).toMatchRenderedOutput(); } diff --git a/packages/react-test-renderer/src/__tests__/ReactTestRendererAsync-test.js b/packages/react-test-renderer/src/__tests__/ReactTestRendererAsync-test.js index cf61a0c3759df..a9d752ce57541 100644 --- a/packages/react-test-renderer/src/__tests__/ReactTestRendererAsync-test.js +++ b/packages/react-test-renderer/src/__tests__/ReactTestRendererAsync-test.js @@ -66,7 +66,14 @@ describe('ReactTestRendererAsync', () => { expect(renderer.toJSON()).toEqual(['A:1', 'B:1', 'C:1']); renderer.update(); - expect(renderer).toFlushAndYield(['A:2', 'B:2', 'C:2']); + expect(renderer).toFlushAndYield(__DEV__ ? [ + 'A:2', + 'A:2', + 'B:2', + 'B:2', + 'C:2', + 'C:2' + ] : ['A:2', 'B:2', 'C:2']); expect(renderer.toJSON()).toEqual(['A:2', 'B:2', 'C:2']); }); diff --git a/packages/react/src/__tests__/forwardRef-test.js b/packages/react/src/__tests__/forwardRef-test.js index 2d20c1aa2acb7..be9a500decc95 100644 --- a/packages/react/src/__tests__/forwardRef-test.js +++ b/packages/react/src/__tests__/forwardRef-test.js @@ -241,11 +241,11 @@ describe('forwardRef', () => { ReactNoop.render(); ReactNoop.flush(); - expect(renderCount).toBe(1); + expect(renderCount).toBe(__DEV__ ? 2 : 1); ReactNoop.render(); ReactNoop.flush(); - expect(renderCount).toBe(2); + expect(renderCount).toBe(__DEV__ ? 4 : 2); }); it('should bailout if forwardRef is wrapped in memo', () => { @@ -264,13 +264,13 @@ describe('forwardRef', () => { ReactNoop.render(); ReactNoop.flush(); - expect(renderCount).toBe(1); + expect(renderCount).toBe(__DEV__ ? 2 : 1); expect(ref.current.type).toBe('div'); ReactNoop.render(); ReactNoop.flush(); - expect(renderCount).toBe(1); + expect(renderCount).toBe(__DEV__ ? 2 : 1); const differentRef = React.createRef(); @@ -278,14 +278,14 @@ describe('forwardRef', () => { , ); ReactNoop.flush(); - expect(renderCount).toBe(2); + expect(renderCount).toBe(__DEV__ ? 4 : 2); expect(ref.current).toBe(null); expect(differentRef.current.type).toBe('div'); ReactNoop.render(); ReactNoop.flush(); - expect(renderCount).toBe(3); + expect(renderCount).toBe(__DEV__ ? 6 : 3); }); it('should custom memo comparisons to compose', () => { @@ -305,19 +305,19 @@ describe('forwardRef', () => { ReactNoop.render(); ReactNoop.flush(); - expect(renderCount).toBe(1); + expect(renderCount).toBe(__DEV__ ? 2 : 1); expect(ref.current.type).toBe('div'); // Changing either a or b rerenders ReactNoop.render(); ReactNoop.flush(); - expect(renderCount).toBe(2); + expect(renderCount).toBe(__DEV__ ? 4 : 2); // Changing c doesn't rerender ReactNoop.render(); ReactNoop.flush(); - expect(renderCount).toBe(2); + expect(renderCount).toBe(__DEV__ ? 4 : 2); const ComposedMemo = React.memo( RefForwardingComponent, @@ -326,29 +326,29 @@ describe('forwardRef', () => { ReactNoop.render(); ReactNoop.flush(); - expect(renderCount).toBe(3); + expect(renderCount).toBe(__DEV__ ? 6 : 3); // Changing just b no longer updates ReactNoop.render(); ReactNoop.flush(); - expect(renderCount).toBe(3); + expect(renderCount).toBe(__DEV__ ? 6 : 3); // Changing just a and c updates ReactNoop.render(); ReactNoop.flush(); - expect(renderCount).toBe(4); + expect(renderCount).toBe(__DEV__ ? 8 : 4); // Changing just c does not update ReactNoop.render(); ReactNoop.flush(); - expect(renderCount).toBe(4); + expect(renderCount).toBe(__DEV__ ? 8 : 4); // Changing ref still rerenders const differentRef = React.createRef(); ReactNoop.render(); ReactNoop.flush(); - expect(renderCount).toBe(5); + expect(renderCount).toBe(__DEV__ ? 10 : 5); expect(ref.current).toBe(null); expect(differentRef.current.type).toBe('div'); From 0271278e808d73bd37ec07d77fba6d958eccd9f8 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Sat, 19 Jan 2019 20:38:17 +0000 Subject: [PATCH 2/3] Double-invoke first function component render too --- .../react-art/src/__tests__/ReactART-test.js | 6 +- .../src/__tests__/ReactDOMRoot-test.js | 2 +- .../src/ReactFiberBeginWork.js | 15 ++- ...rorBoundaryReconciliation-test.internal.js | 6 +- .../__tests__/ReactTestRendererAsync-test.js | 91 ++++++++++++------- 5 files changed, 83 insertions(+), 37 deletions(-) diff --git a/packages/react-art/src/__tests__/ReactART-test.js b/packages/react-art/src/__tests__/ReactART-test.js index a9e0e7486fe06..7e8b2d66ac8c4 100644 --- a/packages/react-art/src/__tests__/ReactART-test.js +++ b/packages/react-art/src/__tests__/ReactART-test.js @@ -385,7 +385,7 @@ describe('ReactART', () => { , ); - ReactNoop.flushThrough(['A']); + ReactNoop.flushThrough(__DEV__ ? ['A', 'A'] : ['A']); ReactDOM.render( @@ -400,7 +400,9 @@ describe('ReactART', () => { expect(ops).toEqual([null, 'ART']); ops = []; - expect(ReactNoop.flush()).toEqual(['B', 'C']); + expect(ReactNoop.flush()).toEqual( + __DEV__ ? ['B', 'B', 'C', 'C'] : ['B', 'C'], + ); expect(ops).toEqual(['Test']); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMRoot-test.js b/packages/react-dom/src/__tests__/ReactDOMRoot-test.js index fd6292385b7f4..3d44d08057214 100644 --- a/packages/react-dom/src/__tests__/ReactDOMRoot-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMRoot-test.js @@ -232,7 +232,7 @@ describe('ReactDOMRoot', () => { // Flush all async work. jest.runAllTimers(); // Root should complete without committing. - expect(ops).toEqual(['Foo']); + expect(ops).toEqual(__DEV__ ? ['Foo', 'Foo'] : ['Foo']); expect(container.textContent).toEqual(''); ops = []; diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index e1beb80dde8b7..c4db8696f3a9a 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -1173,7 +1173,20 @@ function mountIndeterminateComponent( context, renderExpirationTime, ); - // TODO: double-render here in strict mode. + if ( + debugRenderPhaseSideEffects || + (debugRenderPhaseSideEffectsForStrictMode && + workInProgress.mode & StrictMode) + ) { + renderWithHooks( + null, + workInProgress, + Component, + props, + context, + renderExpirationTime, + ); + } } else { value = renderWithHooks( null, diff --git a/packages/react-reconciler/src/__tests__/ErrorBoundaryReconciliation-test.internal.js b/packages/react-reconciler/src/__tests__/ErrorBoundaryReconciliation-test.internal.js index 7962e285bf155..dce0c6326bfa6 100644 --- a/packages/react-reconciler/src/__tests__/ErrorBoundaryReconciliation-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ErrorBoundaryReconciliation-test.internal.js @@ -70,7 +70,11 @@ describe('ErrorBoundaryReconciliation', () => { if (isConcurrent) { renderer.unstable_flushAll(); } - }).toWarnDev(isConcurrent ? ['invalid', 'invalid', 'invalid', 'invalid'] : ['invalid']); + }).toWarnDev( + isConcurrent + ? ['invalid', 'invalid', 'invalid', 'invalid'] + : ['invalid'], + ); const Fallback = fallbackTagName; expect(renderer).toMatchRenderedOutput(); } diff --git a/packages/react-test-renderer/src/__tests__/ReactTestRendererAsync-test.js b/packages/react-test-renderer/src/__tests__/ReactTestRendererAsync-test.js index a9d752ce57541..727328222ae54 100644 --- a/packages/react-test-renderer/src/__tests__/ReactTestRendererAsync-test.js +++ b/packages/react-test-renderer/src/__tests__/ReactTestRendererAsync-test.js @@ -62,18 +62,19 @@ describe('ReactTestRendererAsync', () => { unstable_isConcurrent: true, }); - expect(renderer).toFlushAndYield(['A:1', 'B:1', 'C:1']); + expect(renderer).toFlushAndYield( + __DEV__ + ? ['A:1', 'A:1', 'B:1', 'B:1', 'C:1', 'C:1'] + : ['A:1', 'B:1', 'C:1'], + ); expect(renderer.toJSON()).toEqual(['A:1', 'B:1', 'C:1']); renderer.update(); - expect(renderer).toFlushAndYield(__DEV__ ? [ - 'A:2', - 'A:2', - 'B:2', - 'B:2', - 'C:2', - 'C:2' - ] : ['A:2', 'B:2', 'C:2']); + expect(renderer).toFlushAndYield( + __DEV__ + ? ['A:2', 'A:2', 'B:2', 'B:2', 'C:2', 'C:2'] + : ['A:2', 'B:2', 'C:2'], + ); expect(renderer.toJSON()).toEqual(['A:2', 'B:2', 'C:2']); }); @@ -96,12 +97,14 @@ describe('ReactTestRendererAsync', () => { }); // Flush the first two siblings - expect(renderer).toFlushAndYieldThrough(['A:1', 'B:1']); + expect(renderer).toFlushAndYieldThrough( + __DEV__ ? ['A:1', 'A:1', 'B:1', 'B:1'] : ['A:1', 'B:1'], + ); // Did not commit yet. expect(renderer.toJSON()).toEqual(null); // Flush the remaining work - expect(renderer).toFlushAndYield(['C:1']); + expect(renderer).toFlushAndYield(__DEV__ ? ['C:1', 'C:1'] : ['C:1']); expect(renderer.toJSON()).toEqual(['A:1', 'B:1', 'C:1']); }); @@ -133,7 +136,7 @@ describe('ReactTestRendererAsync', () => { }); // Flush the some of the changes, but don't commit - expect(renderer).toFlushAndYieldThrough(['A:1']); + expect(renderer).toFlushAndYieldThrough(__DEV__ ? ['A:1', 'A:1'] : ['A:1']); expect(renderer.toJSON()).toEqual(null); // Interrupt with higher priority properties @@ -229,30 +232,54 @@ describe('ReactTestRendererAsync', () => { }); expect(renderer).toFlushAndThrow('Oh no!'); - expect(ReactTestRenderer).toHaveYielded([ - 'A', - 'B', - 'C', - 'D', - 'A', - 'B', - 'C', - 'D', - ]); + expect(ReactTestRenderer).toHaveYielded( + __DEV__ + ? [ + 'A', + 'A', + 'B', + 'B', + 'C', + 'C', + 'D', + 'D', + 'A', + 'A', + 'B', + 'B', + 'C', + 'C', + 'D', + 'D', + ] + : ['A', 'B', 'C', 'D', 'A', 'B', 'C', 'D'], + ); renderer.update(); expect(renderer).toFlushAndThrow('Oh no!'); - expect(ReactTestRenderer).toHaveYielded([ - 'A', - 'B', - 'C', - 'D', - 'A', - 'B', - 'C', - 'D', - ]); + expect(ReactTestRenderer).toHaveYielded( + __DEV__ + ? [ + 'A', + 'A', + 'B', + 'B', + 'C', + 'C', + 'D', + 'D', + 'A', + 'A', + 'B', + 'B', + 'C', + 'C', + 'D', + 'D', + ] + : ['A', 'B', 'C', 'D', 'A', 'B', 'C', 'D'], + ); renderer.update(); expect(renderer).toFlushAndThrow('Oh no!'); From f836352d90efd25b3dbc4f9a4d8001af1c887e82 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Sat, 19 Jan 2019 21:16:17 +0000 Subject: [PATCH 3/3] Mark TestRendererAsync test as internal and revert changes to it TestRenderer is built with strict mode doublerender off. We could change that but I'm not sure we want to. So I'll just flip the flag off for this test. --- ...> ReactTestRendererAsync-test.internal.js} | 87 ++++++------------- 1 file changed, 28 insertions(+), 59 deletions(-) rename packages/react-test-renderer/src/__tests__/{ReactTestRendererAsync-test.js => ReactTestRendererAsync-test.internal.js} (81%) diff --git a/packages/react-test-renderer/src/__tests__/ReactTestRendererAsync-test.js b/packages/react-test-renderer/src/__tests__/ReactTestRendererAsync-test.internal.js similarity index 81% rename from packages/react-test-renderer/src/__tests__/ReactTestRendererAsync-test.js rename to packages/react-test-renderer/src/__tests__/ReactTestRendererAsync-test.internal.js index 727328222ae54..9b6b5315e8b40 100644 --- a/packages/react-test-renderer/src/__tests__/ReactTestRendererAsync-test.js +++ b/packages/react-test-renderer/src/__tests__/ReactTestRendererAsync-test.internal.js @@ -11,11 +11,14 @@ 'use strict'; let React; +let ReactFeatureFlags; let ReactTestRenderer; describe('ReactTestRendererAsync', () => { beforeEach(() => { jest.resetModules(); + ReactFeatureFlags = require('shared/ReactFeatureFlags'); + ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false; React = require('react'); ReactTestRenderer = require('react-test-renderer'); }); @@ -62,19 +65,11 @@ describe('ReactTestRendererAsync', () => { unstable_isConcurrent: true, }); - expect(renderer).toFlushAndYield( - __DEV__ - ? ['A:1', 'A:1', 'B:1', 'B:1', 'C:1', 'C:1'] - : ['A:1', 'B:1', 'C:1'], - ); + expect(renderer).toFlushAndYield(['A:1', 'B:1', 'C:1']); expect(renderer.toJSON()).toEqual(['A:1', 'B:1', 'C:1']); renderer.update(); - expect(renderer).toFlushAndYield( - __DEV__ - ? ['A:2', 'A:2', 'B:2', 'B:2', 'C:2', 'C:2'] - : ['A:2', 'B:2', 'C:2'], - ); + expect(renderer).toFlushAndYield(['A:2', 'B:2', 'C:2']); expect(renderer.toJSON()).toEqual(['A:2', 'B:2', 'C:2']); }); @@ -97,14 +92,12 @@ describe('ReactTestRendererAsync', () => { }); // Flush the first two siblings - expect(renderer).toFlushAndYieldThrough( - __DEV__ ? ['A:1', 'A:1', 'B:1', 'B:1'] : ['A:1', 'B:1'], - ); + expect(renderer).toFlushAndYieldThrough(['A:1', 'B:1']); // Did not commit yet. expect(renderer.toJSON()).toEqual(null); // Flush the remaining work - expect(renderer).toFlushAndYield(__DEV__ ? ['C:1', 'C:1'] : ['C:1']); + expect(renderer).toFlushAndYield(['C:1']); expect(renderer.toJSON()).toEqual(['A:1', 'B:1', 'C:1']); }); @@ -136,7 +129,7 @@ describe('ReactTestRendererAsync', () => { }); // Flush the some of the changes, but don't commit - expect(renderer).toFlushAndYieldThrough(__DEV__ ? ['A:1', 'A:1'] : ['A:1']); + expect(renderer).toFlushAndYieldThrough(['A:1']); expect(renderer.toJSON()).toEqual(null); // Interrupt with higher priority properties @@ -232,54 +225,30 @@ describe('ReactTestRendererAsync', () => { }); expect(renderer).toFlushAndThrow('Oh no!'); - expect(ReactTestRenderer).toHaveYielded( - __DEV__ - ? [ - 'A', - 'A', - 'B', - 'B', - 'C', - 'C', - 'D', - 'D', - 'A', - 'A', - 'B', - 'B', - 'C', - 'C', - 'D', - 'D', - ] - : ['A', 'B', 'C', 'D', 'A', 'B', 'C', 'D'], - ); + expect(ReactTestRenderer).toHaveYielded([ + 'A', + 'B', + 'C', + 'D', + 'A', + 'B', + 'C', + 'D', + ]); renderer.update(); expect(renderer).toFlushAndThrow('Oh no!'); - expect(ReactTestRenderer).toHaveYielded( - __DEV__ - ? [ - 'A', - 'A', - 'B', - 'B', - 'C', - 'C', - 'D', - 'D', - 'A', - 'A', - 'B', - 'B', - 'C', - 'C', - 'D', - 'D', - ] - : ['A', 'B', 'C', 'D', 'A', 'B', 'C', 'D'], - ); + expect(ReactTestRenderer).toHaveYielded([ + 'A', + 'B', + 'C', + 'D', + 'A', + 'B', + 'C', + 'D', + ]); renderer.update(); expect(renderer).toFlushAndThrow('Oh no!');