From 6ab57a8d1644bb01d7cd15328000a0b065c8aab1 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Tue, 7 Sep 2021 11:41:52 +0200 Subject: [PATCH 1/3] Test bad useEffect return value with noop-renderer --- .../src/__tests__/ReactHooks-test.internal.js | 36 ----------- .../ReactHooksWithNoopRenderer-test.js | 59 +++++++++++++++++++ 2 files changed, 59 insertions(+), 36 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 9e39b35fc1ba2..f82efce9ef94c 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -727,42 +727,6 @@ describe('ReactHooks', () => { ReactTestRenderer.create(); }); - it('assumes useEffect clean-up function is either a function or undefined', () => { - const {useLayoutEffect} = React; - - function App(props) { - useLayoutEffect(() => { - return props.return; - }); - return null; - } - - const root1 = ReactTestRenderer.create(null); - expect(() => root1.update()).toErrorDev([ - 'Warning: An effect function must not return anything besides a ' + - 'function, which is used for clean-up. You returned: 17', - ]); - - const root2 = ReactTestRenderer.create(null); - expect(() => root2.update()).toErrorDev([ - 'Warning: An effect function must not return anything besides a ' + - 'function, which is used for clean-up. You returned null. If your ' + - 'effect does not require clean up, return undefined (or nothing).', - ]); - - const root3 = ReactTestRenderer.create(null); - expect(() => root3.update()).toErrorDev([ - 'Warning: An effect function must not return anything besides a ' + - 'function, which is used for clean-up.\n\n' + - 'It looks like you wrote useEffect(async () => ...) or returned a Promise.', - ]); - - // Error on unmount because React assumes the value is a function - expect(() => { - root3.update(null); - }).toThrow('is not a function'); - }); - it('does not forget render phase useState updates inside an effect', () => { const {useState, useEffect} = React; diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index 64207dcaa4707..ab6574d469e2f 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -2632,6 +2632,65 @@ describe('ReactHooksWithNoopRenderer', () => { }); expect(Scheduler).toHaveYielded(['layout destroy', 'passive destroy']); }); + + it('assumes passive effect destroy function is either a function or undefined', () => { + function App(props) { + useEffect(() => { + return props.return; + }); + return null; + } + + expect(() => + act(() => { + ReactNoop.render(); + }), + ).toErrorDev([ + 'Warning: An effect function must not return anything besides a ' + + 'function, which is used for clean-up. You returned: 17', + ]); + + // Error on unmount because React assumes the value is a function + expect(() => + act(() => { + ReactNoop.render(null); + }), + ).toThrow('is not a function'); + + expect(() => + act(() => { + ReactNoop.render(); + }), + ).toErrorDev([ + 'Warning: An effect function must not return anything besides a ' + + 'function, which is used for clean-up. You returned null. If your ' + + 'effect does not require clean up, return undefined (or nothing).', + ]); + + // Error on unmount because React assumes the value is a function + expect(() => + act(() => { + ReactNoop.render(null); + }), + ).toThrow('is not a function'); + + expect(() => + act(() => { + ReactNoop.render(); + }), + ).toErrorDev([ + 'Warning: An effect function must not return anything besides a ' + + 'function, which is used for clean-up.\n\n' + + 'It looks like you wrote useEffect(async () => ...) or returned a Promise.', + ]); + + // Error on unmount because React assumes the value is a function + expect(() => + act(() => { + ReactNoop.render(null); + }), + ).toThrow('is not a function'); + }); }); describe('useLayoutEffect', () => { From 03a9fc8a38ba6529bdc96fec1bf6725ae707e0c8 Mon Sep 17 00:00:00 2001 From: eps1lon Date: Tue, 7 Sep 2021 12:06:39 +0200 Subject: [PATCH 2/3] Use previous "root"-approach Tests should now be invariant under variants --- .../ReactHooksWithNoopRenderer-test.js | 25 ++++++------------- 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index ab6574d469e2f..08faec61d05ce 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -2641,25 +2641,20 @@ describe('ReactHooksWithNoopRenderer', () => { return null; } + const root1 = ReactNoop.createRoot(); expect(() => act(() => { - ReactNoop.render(); + root1.render(); }), ).toErrorDev([ 'Warning: An effect function must not return anything besides a ' + 'function, which is used for clean-up. You returned: 17', ]); - // Error on unmount because React assumes the value is a function - expect(() => - act(() => { - ReactNoop.render(null); - }), - ).toThrow('is not a function'); - + const root2 = ReactNoop.createRoot(); expect(() => act(() => { - ReactNoop.render(); + root2.render(); }), ).toErrorDev([ 'Warning: An effect function must not return anything besides a ' + @@ -2667,16 +2662,10 @@ describe('ReactHooksWithNoopRenderer', () => { 'effect does not require clean up, return undefined (or nothing).', ]); - // Error on unmount because React assumes the value is a function - expect(() => - act(() => { - ReactNoop.render(null); - }), - ).toThrow('is not a function'); - + const root3 = ReactNoop.createRoot(); expect(() => act(() => { - ReactNoop.render(); + root3.render(); }), ).toErrorDev([ 'Warning: An effect function must not return anything besides a ' + @@ -2687,7 +2676,7 @@ describe('ReactHooksWithNoopRenderer', () => { // Error on unmount because React assumes the value is a function expect(() => act(() => { - ReactNoop.render(null); + root3.unmount(); }), ).toThrow('is not a function'); }); From d8ad8e8166f95f91d079906f7e814ab2c460219d Mon Sep 17 00:00:00 2001 From: eps1lon Date: Wed, 8 Sep 2021 10:20:20 +0200 Subject: [PATCH 3/3] Add same test for layout effects --- .../ReactHooksWithNoopRenderer-test.js | 48 +++++++++++++++++++ 1 file changed, 48 insertions(+) diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index 08faec61d05ce..342c8abf9a254 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -2858,6 +2858,54 @@ describe('ReactHooksWithNoopRenderer', () => { ]); expect(ReactNoop.getChildren()).toEqual([span('OuterFallback')]); }); + + it('assumes layout effect destroy function is either a function or undefined', () => { + function App(props) { + useLayoutEffect(() => { + return props.return; + }); + return null; + } + + const root1 = ReactNoop.createRoot(); + expect(() => + act(() => { + root1.render(); + }), + ).toErrorDev([ + 'Warning: An effect function must not return anything besides a ' + + 'function, which is used for clean-up. You returned: 17', + ]); + + const root2 = ReactNoop.createRoot(); + expect(() => + act(() => { + root2.render(); + }), + ).toErrorDev([ + 'Warning: An effect function must not return anything besides a ' + + 'function, which is used for clean-up. You returned null. If your ' + + 'effect does not require clean up, return undefined (or nothing).', + ]); + + const root3 = ReactNoop.createRoot(); + expect(() => + act(() => { + root3.render(); + }), + ).toErrorDev([ + 'Warning: An effect function must not return anything besides a ' + + 'function, which is used for clean-up.\n\n' + + 'It looks like you wrote useEffect(async () => ...) or returned a Promise.', + ]); + + // Error on unmount because React assumes the value is a function + expect(() => + act(() => { + root3.unmount(); + }), + ).toThrow('is not a function'); + }); }); describe('useCallback', () => {