From 4b466ced50a3729f8f7f5bfcaaf5ea1e5c6782c9 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Wed, 3 Jul 2019 00:15:04 +0100 Subject: [PATCH 1/2] 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. --- .../src/__tests__/ReactDOMHooks-test.js | 43 ++++++++----------- .../src/__tests__/ReactTestUtilsAct-test.js | 34 +++++++++++++++ .../src/ReactFiberWorkLoop.js | 1 + .../src/__tests__/ReactHooks-test.internal.js | 1 - 4 files changed, 52 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..ac37e5ba07ac 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js @@ -48,6 +48,20 @@ 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); }); function runActTests(label, render, unmount) { @@ -68,6 +82,26 @@ function runActTests(label, render, unmount) { document.body.removeChild(container); }); describe('sync', () => { + it('warns if an effect is queued outside an act scope, except in legacy sync+non-strict mode', () => { + function App() { + React.useEffect(() => {}, []); + return null; + } + expect(() => { + render(, container); + // flush all queued work + Scheduler.unstable_flushAll(); + }).toWarnDev( + label !== 'legacy sync mode' + ? [ + // warns twice because we're in strict+dev mode + '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('can use act to flush effects', () => { function App() { React.useEffect(() => { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index 1c5d14651fc2..f8a389e63577 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -2453,6 +2453,7 @@ export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void { if (__DEV__) { if ( warnsIfNotActing === true && + fiber.mode && 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. From 394ce777142c2a722b8beb5edcb54e821d6a7139 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Wed, 3 Jul 2019 01:03:35 +0100 Subject: [PATCH 2/2] explicitly check for modes before warning, explicit tests for all modes --- .../src/__tests__/ReactTestUtilsAct-test.js | 73 ++++++++++++++----- .../src/ReactFiberWorkLoop.js | 6 +- 2 files changed, 58 insertions(+), 21 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js index ac37e5ba07ac..827b3aa2f185 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js @@ -62,6 +62,59 @@ describe('ReactTestUtils.act()', () => { } } 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) { @@ -82,26 +135,6 @@ function runActTests(label, render, unmount) { document.body.removeChild(container); }); describe('sync', () => { - it('warns if an effect is queued outside an act scope, except in legacy sync+non-strict mode', () => { - function App() { - React.useEffect(() => {}, []); - return null; - } - expect(() => { - render(, container); - // flush all queued work - Scheduler.unstable_flushAll(); - }).toWarnDev( - label !== 'legacy sync mode' - ? [ - // warns twice because we're in strict+dev mode - '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('can use act to flush effects', () => { function App() { React.useEffect(() => { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index f8a389e63577..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,7 +2454,10 @@ export function warnIfNotCurrentlyActingEffectsInDEV(fiber: Fiber): void { if (__DEV__) { if ( warnsIfNotActing === true && - fiber.mode && + (fiber.mode & StrictMode || + fiber.mode & ProfileMode || + fiber.mode & BatchedMode || + fiber.mode & ConcurrentMode) && IsSomeRendererActing.current === false && IsThisRendererActing.current === false ) {