From 1bda3bb0f6101bfa70833dfccccbc0b7a85fc52c Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 11 Feb 2019 16:57:45 +0000 Subject: [PATCH 1/3] Failing test for false positive warning --- .../src/__tests__/ReactHooks-test.internal.js | 31 +++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 0ed4014dfb29e..740cb94fe9834 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -1366,4 +1366,35 @@ describe('ReactHooks', () => { ), ).toThrow('Hello'); }); + + // Regression test for https://github.com/facebook/react/issues/14790 + it('does not fire a false positive warning when suspending', async () => { + const {Suspense, useState, useRef} = React; + + let wasSuspended = false; + function trySuspend() { + if (!wasSuspended) { + throw new Promise(resolve => { + wasSuspended = true; + resolve(); + }); + } + } + + function Child() { + React.useState(); + trySuspend(); + return 'hello'; + } + + const Wrapper = React.memo(Child); + const root = ReactTestRenderer.create( + + + , + ); + expect(root).toMatchRenderedOutput('loading'); + await Promise.resolve(); + expect(root).toMatchRenderedOutput('hello'); + }); }); From 3a53a84dff5c72d103ff6e0bc68440a34904ee85 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Mon, 11 Feb 2019 17:29:31 +0000 Subject: [PATCH 2/3] Add tests for forwardRef too --- .../src/__tests__/ReactHooks-test.internal.js | 64 ++++++++++++++++++- 1 file changed, 63 insertions(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 740cb94fe9834..51bf0ae3fd692 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -1368,7 +1368,7 @@ describe('ReactHooks', () => { }); // Regression test for https://github.com/facebook/react/issues/14790 - it('does not fire a false positive warning when suspending', async () => { + it('does not fire a false positive warning when suspending memo', async () => { const {Suspense, useState, useRef} = React; let wasSuspended = false; @@ -1397,4 +1397,66 @@ describe('ReactHooks', () => { await Promise.resolve(); expect(root).toMatchRenderedOutput('hello'); }); + + // Regression test for https://github.com/facebook/react/issues/14790 + it('does not fire a false positive warning when suspending forwardRef', async () => { + const {Suspense, useState, useRef} = React; + + let wasSuspended = false; + function trySuspend() { + if (!wasSuspended) { + throw new Promise(resolve => { + wasSuspended = true; + resolve(); + }); + } + } + + function render(props, ref) { + React.useState(); + trySuspend(); + return 'hello'; + } + + const Wrapper = React.forwardRef(render); + const root = ReactTestRenderer.create( + + + , + ); + expect(root).toMatchRenderedOutput('loading'); + await Promise.resolve(); + expect(root).toMatchRenderedOutput('hello'); + }); + + // Regression test for https://github.com/facebook/react/issues/14790 + it('does not fire a false positive warning when suspending memo(forwardRef)', async () => { + const {Suspense, useState, useRef} = React; + + let wasSuspended = false; + function trySuspend() { + if (!wasSuspended) { + throw new Promise(resolve => { + wasSuspended = true; + resolve(); + }); + } + } + + function render(props, ref) { + React.useState(); + trySuspend(); + return 'hello'; + } + + const Wrapper = React.memo(React.forwardRef(render)); + const root = ReactTestRenderer.create( + + + , + ); + expect(root).toMatchRenderedOutput('loading'); + await Promise.resolve(); + expect(root).toMatchRenderedOutput('hello'); + }); }); From 23bf0a72c55b2667542f44ac538cd21030d68d41 Mon Sep 17 00:00:00 2001 From: Dan Abramov Date: Tue, 12 Feb 2019 20:24:15 +0000 Subject: [PATCH 3/3] Remove the warning and add TODOs --- packages/react-reconciler/src/ReactFiberBeginWork.js | 8 ++++++++ packages/react-reconciler/src/ReactFiberHooks.js | 11 ----------- .../src/__tests__/ReactHooks-test.internal.js | 12 ++++++------ 3 files changed, 14 insertions(+), 17 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.js b/packages/react-reconciler/src/ReactFiberBeginWork.js index 945051d3d1b2a..611554594fc8d 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.js @@ -220,6 +220,10 @@ function updateForwardRef( nextProps: any, renderExpirationTime: ExpirationTime, ) { + // TODO: current can be non-null here even if the component + // hasn't yet mounted. This happens after the first render suspends. + // We'll need to figure out if this is fine or can cause issues. + if (__DEV__) { if (workInProgress.type !== workInProgress.elementType) { // Lazy component props can't be validated in createElement @@ -415,6 +419,10 @@ function updateSimpleMemoComponent( updateExpirationTime, renderExpirationTime: ExpirationTime, ): null | Fiber { + // TODO: current can be non-null here even if the component + // hasn't yet mounted. This happens when the inner render suspends. + // We'll need to figure out if this is fine or can cause issues. + if (__DEV__) { if (workInProgress.type !== workInProgress.elementType) { // Lazy component props can't be validated in createElement diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 6eceed84f7e41..0b5f0172e7ea1 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -449,17 +449,6 @@ function mountWorkInProgressHook(): Hook { if (__DEV__) { (hook: any)._debugType = (currentHookNameInDev: any); - if ( - currentlyRenderingFiber !== null && - currentlyRenderingFiber.alternate !== null - ) { - warning( - false, - '%s: Rendered more hooks than during the previous render. This is ' + - 'not currently supported and may lead to unexpected behavior.', - getComponentName(currentlyRenderingFiber.type), - ); - } } if (workInProgressHook === null) { // This is the first hook in the list diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 51bf0ae3fd692..f7ab26011b64e 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -1369,7 +1369,7 @@ describe('ReactHooks', () => { // Regression test for https://github.com/facebook/react/issues/14790 it('does not fire a false positive warning when suspending memo', async () => { - const {Suspense, useState, useRef} = React; + const {Suspense, useState} = React; let wasSuspended = false; function trySuspend() { @@ -1382,7 +1382,7 @@ describe('ReactHooks', () => { } function Child() { - React.useState(); + useState(); trySuspend(); return 'hello'; } @@ -1400,7 +1400,7 @@ describe('ReactHooks', () => { // Regression test for https://github.com/facebook/react/issues/14790 it('does not fire a false positive warning when suspending forwardRef', async () => { - const {Suspense, useState, useRef} = React; + const {Suspense, useState} = React; let wasSuspended = false; function trySuspend() { @@ -1413,7 +1413,7 @@ describe('ReactHooks', () => { } function render(props, ref) { - React.useState(); + useState(); trySuspend(); return 'hello'; } @@ -1431,7 +1431,7 @@ describe('ReactHooks', () => { // Regression test for https://github.com/facebook/react/issues/14790 it('does not fire a false positive warning when suspending memo(forwardRef)', async () => { - const {Suspense, useState, useRef} = React; + const {Suspense, useState} = React; let wasSuspended = false; function trySuspend() { @@ -1444,7 +1444,7 @@ describe('ReactHooks', () => { } function render(props, ref) { - React.useState(); + useState(); trySuspend(); return 'hello'; }