From a98cd03441da8c67e74114de653df1c2d1b12bcb Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Wed, 3 Jul 2019 01:29:45 +0100 Subject: [PATCH] [fail] Only warn on unacted effects for strict / non sync modes (#16041) * only warn on unacted effects for strict / non sync modes (basically, when `fiber.mode !== 0b0000`) Warnings on unacted effects may be too noisy, especially for legacy apps. This PR fires the warning only when in a non sync mode (concurrent/batched), or when in strict mode. This should make updating easier. I also added batched mode tests to the act() suite. * explicitly check for modes before warning, explicit tests for all modes --- .../src/__tests__/ReactDOMHooks-test.js | 43 +++++------- .../src/__tests__/ReactTestUtilsAct-test.js | 67 +++++++++++++++++++ .../src/ReactFiberWorkLoop.js | 5 ++ .../src/__tests__/ReactHooks-test.internal.js | 1 - 4 files changed, 89 insertions(+), 27 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMHooks-test.js b/packages/react-dom/src/__tests__/ReactDOMHooks-test.js index 896c8c0acac0..aff3003fc0ce 100644 --- a/packages/react-dom/src/__tests__/ReactDOMHooks-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMHooks-test.js @@ -53,32 +53,23 @@ describe('ReactDOMHooks', () => { return 3 * n; } - // we explicitly catch the missing act() warnings - // to simulate this tricky repro - expect(() => { - ReactDOM.render(, container); - expect(container.textContent).toBe('1'); - expect(container2.textContent).toBe(''); - expect(container3.textContent).toBe(''); - Scheduler.unstable_flushAll(); - expect(container.textContent).toBe('1'); - expect(container2.textContent).toBe('2'); - expect(container3.textContent).toBe('3'); - - ReactDOM.render(, container); - expect(container.textContent).toBe('2'); - expect(container2.textContent).toBe('2'); // Not flushed yet - expect(container3.textContent).toBe('3'); // Not flushed yet - Scheduler.unstable_flushAll(); - expect(container.textContent).toBe('2'); - expect(container2.textContent).toBe('4'); - expect(container3.textContent).toBe('6'); - }).toWarnDev([ - 'An update to Example1 ran an effect', - 'An update to Example2 ran an effect', - 'An update to Example1 ran an effect', - 'An update to Example2 ran an effect', - ]); + ReactDOM.render(, container); + expect(container.textContent).toBe('1'); + expect(container2.textContent).toBe(''); + expect(container3.textContent).toBe(''); + Scheduler.unstable_flushAll(); + expect(container.textContent).toBe('1'); + expect(container2.textContent).toBe('2'); + expect(container3.textContent).toBe('3'); + + ReactDOM.render(, container); + expect(container.textContent).toBe('2'); + expect(container2.textContent).toBe('2'); // Not flushed yet + expect(container3.textContent).toBe('3'); // Not flushed yet + Scheduler.unstable_flushAll(); + expect(container.textContent).toBe('2'); + expect(container2.textContent).toBe('4'); + expect(container3.textContent).toBe('6'); }); it('should not bail out when an update is scheduled from within an event handler', () => { diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js index fb1027a30628..827b3aa2f185 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js @@ -48,6 +48,73 @@ describe('ReactTestUtils.act()', () => { ReactDOM.unmountComponentAtNode(dom); } runActTests('legacy sync mode', renderSync, unmountSync); + + // and then in batched mode + let batchedRoot; + function renderBatched(el, dom) { + batchedRoot = ReactDOM.unstable_createSyncRoot(dom); + batchedRoot.render(el); + } + function unmountBatched(dom) { + if (batchedRoot !== null) { + batchedRoot.unmount(); + batchedRoot = null; + } + } + runActTests('batched mode', renderBatched, unmountBatched); + + describe('unacted effects', () => { + function App() { + React.useEffect(() => {}, []); + return null; + } + + it('does not warn in legacy sync mode', () => { + expect(() => { + ReactDOM.render(, document.createElement('div')); + }).toWarnDev([]); + }); + + it('warns in strict mode', () => { + expect(() => { + ReactDOM.render( + + + , + document.createElement('div'), + ); + }).toWarnDev([ + 'An update to App ran an effect, but was not wrapped in act(...)', + 'An update to App ran an effect, but was not wrapped in act(...)', + ]); + }); + + it('warns in batched mode', () => { + expect(() => { + const root = ReactDOM.unstable_createSyncRoot( + document.createElement('div'), + ); + root.render(); + Scheduler.unstable_flushAll(); + }).toWarnDev([ + 'An update to App ran an effect, but was not wrapped in act(...)', + 'An update to App ran an effect, but was not wrapped in act(...)', + ]); + }); + + it('warns in concurrent mode', () => { + expect(() => { + const root = ReactDOM.unstable_createRoot( + document.createElement('div'), + ); + root.render(); + Scheduler.unstable_flushAll(); + }).toWarnDev([ + 'An update to App ran an effect, but was not wrapped in act(...)', + 'An update to App ran an effect, but was not wrapped in act(...)', + ]); + }); + }); }); function runActTests(label, render, unmount) { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 1c5d14651fc2..bdd43849c304 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -61,6 +61,7 @@ import { import {createWorkInProgress, assignFiberPropertiesInDEV} from './ReactFiber'; import { NoMode, + StrictMode, ProfileMode, BatchedMode, ConcurrentMode, @@ -2453,6 +2454,10 @@ export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void { if (__DEV__) { if ( warnsIfNotActing === true && + (fiber.mode & StrictMode || + fiber.mode & ProfileMode || + fiber.mode & BatchedMode || + fiber.mode & ConcurrentMode) && IsSomeRendererActing.current === false && IsThisRendererActing.current === false ) { diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index e0022d0a70c2..eff2904158a7 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -1806,7 +1806,6 @@ describe('ReactHooks', () => { globalListener(); globalListener(); }).toWarnDev([ - 'An update to C ran an effect', 'An update to C inside a test was not wrapped in act', 'An update to C inside a test was not wrapped in act', // Note: should *not* warn about updates on unmounted component.