From e1f7952145f129018e6ad6906aa8604a8f5c70c6 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Fri, 1 Feb 2019 14:54:43 +0000 Subject: [PATCH 01/31] expose unstable_interact for batching actions in tests --- .../react-dom/src/__tests__/ReactDOM-test.js | 53 +++++++++++++++++++ packages/react-dom/src/client/ReactDOM.js | 6 +++ packages/react-dom/src/fire/ReactFire.js | 6 +++ .../src/ReactFiberReconciler.js | 1 + 4 files changed, 66 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactDOM-test.js b/packages/react-dom/src/__tests__/ReactDOM-test.js index e618d3e6ffc63..0dba4c1bfc6ca 100644 --- a/packages/react-dom/src/__tests__/ReactDOM-test.js +++ b/packages/react-dom/src/__tests__/ReactDOM-test.js @@ -509,4 +509,57 @@ describe('ReactDOM', () => { ' in App (at **)', ]); }); + + it('can use interact to batch effects', () => { + function App(props) { + React.useEffect(props.callback); + return null; + } + const container = document.createElement('div'); + document.body.appendChild(container); + + try { + let called = false; + ReactDOM.unstable_interact(() => { + ReactDOM.render( + { + called = true; + }} + />, + container, + ); + }); + + expect(called).toBe(true); + } finally { + document.body.removeChild(container); + } + }); + it('can use interact to batch effects on updates too', () => { + function App() { + let [ctr, setCtr] = React.useState(0); + return ( + + ); + } + const container = document.createElement('div'); + document.body.appendChild(container); + let button; + try { + ReactDOM.unstable_interact(() => { + ReactDOM.render(, container); + }); + button = document.getElementById('button'); + expect(button.innerHTML).toBe('0'); + ReactDOM.unstable_interact(() => { + button.dispatchEvent(new MouseEvent('click', {bubbles: true})); + }); + expect(button.innerHTML).toBe('1'); + } finally { + document.body.removeChild(container); + } + }); }); diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index 80251c2ac2fe7..bce9a299ef690 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -35,6 +35,7 @@ import { getPublicRootInstance, findHostInstance, findHostInstanceWithWarning, + flushPassiveEffects, } from 'react-reconciler/inline.dom'; import {createPortal as createPortalImpl} from 'shared/ReactPortal'; import {canUseDOM} from 'shared/ExecutionEnvironment'; @@ -802,6 +803,11 @@ const ReactDOM: Object = { unstable_interactiveUpdates: interactiveUpdates, + unstable_interact(callback: () => void) { + batchedUpdates(callback); + flushPassiveEffects(); + }, + flushSync: flushSync, unstable_createRoot: createRoot, diff --git a/packages/react-dom/src/fire/ReactFire.js b/packages/react-dom/src/fire/ReactFire.js index 58a74eba45e92..550cd1b9d00b5 100644 --- a/packages/react-dom/src/fire/ReactFire.js +++ b/packages/react-dom/src/fire/ReactFire.js @@ -40,6 +40,7 @@ import { getPublicRootInstance, findHostInstance, findHostInstanceWithWarning, + flushPassiveEffects, } from 'react-reconciler/inline.fire'; import {createPortal as createPortalImpl} from 'shared/ReactPortal'; import {canUseDOM} from 'shared/ExecutionEnvironment'; @@ -807,6 +808,11 @@ const ReactDOM: Object = { unstable_interactiveUpdates: interactiveUpdates, + unstable_interact(callback: () => void) { + batchedUpdates(callback); + flushPassiveEffects(); + }, + flushSync: flushSync, unstable_createRoot: createRoot, diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index acb91ea3bb905..e2704f1b62118 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.js @@ -310,6 +310,7 @@ export { flushInteractiveUpdates, flushControlled, flushSync, + flushPassiveEffects, }; export function getPublicRootInstance( From 5407e887d80ffd83711fe0779cb833664ab91af0 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Fri, 1 Feb 2019 16:02:13 +0000 Subject: [PATCH 02/31] move to TestUtils --- .../react-dom/src/__tests__/ReactDOM-test.js | 53 ------------------ .../src/__tests__/ReactTestUtils-test.js | 54 +++++++++++++++++++ packages/react-dom/src/client/ReactDOM.js | 13 +++-- packages/react-dom/src/fire/ReactFire.js | 13 +++-- .../src/test-utils/ReactTestUtils.js | 5 ++ 5 files changed, 75 insertions(+), 63 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOM-test.js b/packages/react-dom/src/__tests__/ReactDOM-test.js index 0dba4c1bfc6ca..e618d3e6ffc63 100644 --- a/packages/react-dom/src/__tests__/ReactDOM-test.js +++ b/packages/react-dom/src/__tests__/ReactDOM-test.js @@ -509,57 +509,4 @@ describe('ReactDOM', () => { ' in App (at **)', ]); }); - - it('can use interact to batch effects', () => { - function App(props) { - React.useEffect(props.callback); - return null; - } - const container = document.createElement('div'); - document.body.appendChild(container); - - try { - let called = false; - ReactDOM.unstable_interact(() => { - ReactDOM.render( - { - called = true; - }} - />, - container, - ); - }); - - expect(called).toBe(true); - } finally { - document.body.removeChild(container); - } - }); - it('can use interact to batch effects on updates too', () => { - function App() { - let [ctr, setCtr] = React.useState(0); - return ( - - ); - } - const container = document.createElement('div'); - document.body.appendChild(container); - let button; - try { - ReactDOM.unstable_interact(() => { - ReactDOM.render(, container); - }); - button = document.getElementById('button'); - expect(button.innerHTML).toBe('0'); - ReactDOM.unstable_interact(() => { - button.dispatchEvent(new MouseEvent('click', {bubbles: true})); - }); - expect(button.innerHTML).toBe('1'); - } finally { - document.body.removeChild(container); - } - }); }); diff --git a/packages/react-dom/src/__tests__/ReactTestUtils-test.js b/packages/react-dom/src/__tests__/ReactTestUtils-test.js index 687dbd1aaec2c..247280aac3d3b 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtils-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtils-test.js @@ -515,4 +515,58 @@ describe('ReactTestUtils', () => { ReactTestUtils.renderIntoDocument(); expect(mockArgs.length).toEqual(0); }); + + it('can use interact to batch effects', () => { + function App(props) { + React.useEffect(props.callback); + return null; + } + const container = document.createElement('div'); + document.body.appendChild(container); + + try { + let called = false; + ReactTestUtils.interact(() => { + ReactDOM.render( + { + called = true; + }} + />, + container, + ); + }); + + expect(called).toBe(true); + } finally { + document.body.removeChild(container); + } + }); + + it('can use interact to batch effects on updates too', () => { + function App() { + let [ctr, setCtr] = React.useState(0); + return ( + + ); + } + const container = document.createElement('div'); + document.body.appendChild(container); + let button; + try { + ReactTestUtils.interact(() => { + ReactDOM.render(, container); + }); + button = document.getElementById('button'); + expect(button.innerHTML).toBe('0'); + ReactTestUtils.interact(() => { + button.dispatchEvent(new MouseEvent('click', {bubbles: true})); + }); + expect(button.innerHTML).toBe('1'); + } finally { + document.body.removeChild(container); + } + }); }); diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index bce9a299ef690..de4727bb54d18 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -612,6 +612,13 @@ function createPortal( return createPortalImpl(children, container, null, key); } +// a helper for tests to scope their actions on the dom +// batches and flushes effects and updates +function batchedInteraction(callback: () => void) { + batchedUpdates(callback); + flushPassiveEffects(); +} + const ReactDOM: Object = { createPortal, @@ -803,11 +810,6 @@ const ReactDOM: Object = { unstable_interactiveUpdates: interactiveUpdates, - unstable_interact(callback: () => void) { - batchedUpdates(callback); - flushPassiveEffects(); - }, - flushSync: flushSync, unstable_createRoot: createRoot, @@ -828,6 +830,7 @@ const ReactDOM: Object = { restoreStateIfNeeded, dispatchEvent, runEventsInBatch, + batchedInteraction, ], }, }; diff --git a/packages/react-dom/src/fire/ReactFire.js b/packages/react-dom/src/fire/ReactFire.js index 550cd1b9d00b5..90eb624c8630a 100644 --- a/packages/react-dom/src/fire/ReactFire.js +++ b/packages/react-dom/src/fire/ReactFire.js @@ -617,6 +617,13 @@ function createPortal( return createPortalImpl(children, container, null, key); } +// a helper for tests to scope their actions on the dom +// batches and flushes effects and updates +function batchedInteraction(callback: () => void) { + batchedUpdates(callback); + flushPassiveEffects(); +} + const ReactDOM: Object = { createPortal, @@ -808,11 +815,6 @@ const ReactDOM: Object = { unstable_interactiveUpdates: interactiveUpdates, - unstable_interact(callback: () => void) { - batchedUpdates(callback); - flushPassiveEffects(); - }, - flushSync: flushSync, unstable_createRoot: createRoot, @@ -833,6 +835,7 @@ const ReactDOM: Object = { restoreStateIfNeeded, dispatchEvent, runEventsInBatch, + batchedInteraction, ], }, }; diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js index b596a1cff58f4..3e726205e2776 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtils.js +++ b/packages/react-dom/src/test-utils/ReactTestUtils.js @@ -38,6 +38,7 @@ const [ restoreStateIfNeeded, dispatchEvent, runEventsInBatch, + batchedInteraction, ] = ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Events; function Event(suffix) {} @@ -380,6 +381,10 @@ const ReactTestUtils = { Simulate: null, SimulateNative: {}, + + interact(callback: () => void) { + batchedInteraction(callback); + }, }; /** From e8a03d04edbe96079ddd3420748456489a9bf6cc Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Fri, 1 Feb 2019 18:02:48 +0000 Subject: [PATCH 03/31] move it all into testutils --- packages/react-dom/src/client/ReactDOM.js | 9 --------- packages/react-dom/src/fire/ReactFire.js | 9 --------- packages/react-dom/src/test-utils/ReactTestUtils.js | 4 ++-- packages/react-reconciler/src/ReactFiberReconciler.js | 1 - 4 files changed, 2 insertions(+), 21 deletions(-) diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index de4727bb54d18..80251c2ac2fe7 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -35,7 +35,6 @@ import { getPublicRootInstance, findHostInstance, findHostInstanceWithWarning, - flushPassiveEffects, } from 'react-reconciler/inline.dom'; import {createPortal as createPortalImpl} from 'shared/ReactPortal'; import {canUseDOM} from 'shared/ExecutionEnvironment'; @@ -612,13 +611,6 @@ function createPortal( return createPortalImpl(children, container, null, key); } -// a helper for tests to scope their actions on the dom -// batches and flushes effects and updates -function batchedInteraction(callback: () => void) { - batchedUpdates(callback); - flushPassiveEffects(); -} - const ReactDOM: Object = { createPortal, @@ -830,7 +822,6 @@ const ReactDOM: Object = { restoreStateIfNeeded, dispatchEvent, runEventsInBatch, - batchedInteraction, ], }, }; diff --git a/packages/react-dom/src/fire/ReactFire.js b/packages/react-dom/src/fire/ReactFire.js index 90eb624c8630a..58a74eba45e92 100644 --- a/packages/react-dom/src/fire/ReactFire.js +++ b/packages/react-dom/src/fire/ReactFire.js @@ -40,7 +40,6 @@ import { getPublicRootInstance, findHostInstance, findHostInstanceWithWarning, - flushPassiveEffects, } from 'react-reconciler/inline.fire'; import {createPortal as createPortalImpl} from 'shared/ReactPortal'; import {canUseDOM} from 'shared/ExecutionEnvironment'; @@ -617,13 +616,6 @@ function createPortal( return createPortalImpl(children, container, null, key); } -// a helper for tests to scope their actions on the dom -// batches and flushes effects and updates -function batchedInteraction(callback: () => void) { - batchedUpdates(callback); - flushPassiveEffects(); -} - const ReactDOM: Object = { createPortal, @@ -835,7 +827,6 @@ const ReactDOM: Object = { restoreStateIfNeeded, dispatchEvent, runEventsInBatch, - batchedInteraction, ], }, }; diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js index 3e726205e2776..849fd5a21c2df 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtils.js +++ b/packages/react-dom/src/test-utils/ReactTestUtils.js @@ -38,7 +38,6 @@ const [ restoreStateIfNeeded, dispatchEvent, runEventsInBatch, - batchedInteraction, ] = ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Events; function Event(suffix) {} @@ -383,7 +382,8 @@ const ReactTestUtils = { SimulateNative: {}, interact(callback: () => void) { - batchedInteraction(callback); + ReactDOM.unstable_batchedUpdates(callback); + ReactDOM.render(null, document.createElement('div')); }, }; diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index e2704f1b62118..acb91ea3bb905 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.js @@ -310,7 +310,6 @@ export { flushInteractiveUpdates, flushControlled, flushSync, - flushPassiveEffects, }; export function getPublicRootInstance( From 723f347d98743a748955c31e815299287bcd4abb Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Fri, 1 Feb 2019 20:13:57 +0000 Subject: [PATCH 04/31] s/interact/act --- .../react-dom/src/__tests__/ReactTestUtils-test.js | 10 +++++----- packages/react-dom/src/test-utils/ReactTestUtils.js | 8 ++++++-- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactTestUtils-test.js b/packages/react-dom/src/__tests__/ReactTestUtils-test.js index 247280aac3d3b..f33b48d4b8a18 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtils-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtils-test.js @@ -516,7 +516,7 @@ describe('ReactTestUtils', () => { expect(mockArgs.length).toEqual(0); }); - it('can use interact to batch effects', () => { + it('can use act to batch effects', () => { function App(props) { React.useEffect(props.callback); return null; @@ -526,7 +526,7 @@ describe('ReactTestUtils', () => { try { let called = false; - ReactTestUtils.interact(() => { + ReactTestUtils.act(() => { ReactDOM.render( { @@ -543,7 +543,7 @@ describe('ReactTestUtils', () => { } }); - it('can use interact to batch effects on updates too', () => { + it('can use act to batch effects on updates too', () => { function App() { let [ctr, setCtr] = React.useState(0); return ( @@ -556,12 +556,12 @@ describe('ReactTestUtils', () => { document.body.appendChild(container); let button; try { - ReactTestUtils.interact(() => { + ReactTestUtils.act(() => { ReactDOM.render(, container); }); button = document.getElementById('button'); expect(button.innerHTML).toBe('0'); - ReactTestUtils.interact(() => { + ReactTestUtils.act(() => { button.dispatchEvent(new MouseEvent('click', {bubbles: true})); }); expect(button.innerHTML).toBe('1'); diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js index 849fd5a21c2df..1ac617adec299 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtils.js +++ b/packages/react-dom/src/test-utils/ReactTestUtils.js @@ -145,6 +145,10 @@ function validateClassInstance(inst, methodName) { ); } +// stub elements used by act() when flushing effects +let actElement =
; +let actContainerElement = document.createElement('div'); + /** * Utilities for making it easy to test React components. * @@ -381,9 +385,9 @@ const ReactTestUtils = { Simulate: null, SimulateNative: {}, - interact(callback: () => void) { + act(callback: () => void) { ReactDOM.unstable_batchedUpdates(callback); - ReactDOM.render(null, document.createElement('div')); + ReactDOM.render(actElement, actContainerElement); }, }; From ac7416da8f8d570cab329c5a5769d543eeabd98f Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Sat, 2 Feb 2019 18:47:18 +0000 Subject: [PATCH 05/31] warn when calling hook-like setState outside batching mode --- .../src/__tests__/ReactTestUtils-test.js | 51 +++++++++++++++++++ .../react-reconciler/src/ReactFiberHooks.js | 4 +- .../src/ReactFiberScheduler.js | 17 +++++++ 3 files changed, 71 insertions(+), 1 deletion(-) diff --git a/packages/react-dom/src/__tests__/ReactTestUtils-test.js b/packages/react-dom/src/__tests__/ReactTestUtils-test.js index 687dbd1aaec2c..73a2fd413c1bf 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtils-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtils-test.js @@ -515,4 +515,55 @@ describe('ReactTestUtils', () => { ReactTestUtils.renderIntoDocument(); expect(mockArgs.length).toEqual(0); }); + + it('detects setState being called outside of .act()', () => { + let setValueRef = null; + function App() { + let [value, setValue] = React.useState(0); + setValueRef = setValue; + return ( + + ); + } + const container = document.createElement('div'); + document.body.appendChild(container); + act(() => { + ReactDOM.render(, container); + container + .querySelector('#button') + .dispatchEvent(new MouseEvent('click', {bubbles: true})); + }); + + expect(button.innerHTML).toBe('2'); + expect(() => setValueRef(1)).toWarnDev( + ["called a hook's setState outside of .act()"], + {withoutStack: 1}, + ); + }); + + it('lets a ticker update', () => { + function App() { + let [toggle, setToggle] = React.useState(0); + React.useEffect(() => { + let timeout = setTimeout(() => { + setToggle(1); + }, 200); + return () => clearTimeout(timeout); + }); + return toggle; + } + const container = document.createElement('div'); + act(() => { + ReactDOM.render(, container); + }); + jest.advanceTimersByTime(250); // this warns!!! + expect(container.innerHTML).toBe('1'); + }); }); + +function act(fn) { + ReactDOM.unstable_batchedUpdates(fn); + ReactDOM.render(null, document.createElement('div')); +} diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 5965f4d77ed41..529f2c77a6a04 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -31,6 +31,7 @@ import { } from './ReactHookEffectTags'; import { scheduleWork, + ensureBatchingAndScheduleWork, computeExpirationForFiber, flushPassiveEffects, requestCurrentTime, @@ -1126,6 +1127,7 @@ function dispatchAction( } } } - scheduleWork(fiber, expirationTime); + ensureBatchingAndScheduleWork(fiber, expirationTime); + // scheduleWork(fiber, expirationTime); } } diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 1563f54b69a82..71f7ca737a23e 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -1789,6 +1789,23 @@ function scheduleWorkToRoot(fiber: Fiber, expirationTime): FiberRoot | null { return root; } +const isJSDOM = + 'undefined' !== typeof global.navigator && + (navigator.userAgent.includes('Node.js') || + navigator.userAgent.includes('jsdom')); + +export function ensureBatchingAndScheduleWork( + fiber: Fiber, + expirationTime: ExpirationTime, +) { + if (__DEV__) { + if (isJSDOM && !isBatchingUpdates) { + warningWithoutStack(false, "called a hook's setState outside of .act()"); + } + } + scheduleWork(fiber, expirationTime); +} + function scheduleWork(fiber: Fiber, expirationTime: ExpirationTime) { const root = scheduleWorkToRoot(fiber, expirationTime); if (root === null) { From 1993be9c33e40a0463db650ae3cf6e9c52261201 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Sat, 2 Feb 2019 19:51:36 +0000 Subject: [PATCH 06/31] pass tests --- .../src/__tests__/ReactTestUtils-test.js | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactTestUtils-test.js b/packages/react-dom/src/__tests__/ReactTestUtils-test.js index 73a2fd413c1bf..3b99d3d8bdb22 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtils-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtils-test.js @@ -528,19 +528,19 @@ describe('ReactTestUtils', () => { ); } const container = document.createElement('div'); - document.body.appendChild(container); + document.body.appendChild(container) + let button act(() => { ReactDOM.render(, container); - container - .querySelector('#button') - .dispatchEvent(new MouseEvent('click', {bubbles: true})); + button = container.querySelector('#button') + button.dispatchEvent(new MouseEvent('click', {bubbles: true})); }); - expect(button.innerHTML).toBe('2'); expect(() => setValueRef(1)).toWarnDev( ["called a hook's setState outside of .act()"], {withoutStack: 1}, ); + document.body.removeChild(container) }); it('lets a ticker update', () => { @@ -555,10 +555,14 @@ describe('ReactTestUtils', () => { return toggle; } const container = document.createElement('div'); + act(() => { - ReactDOM.render(, container); + act(()=> { + ReactDOM.render(, container); + }) + jest.advanceTimersByTime(250); }); - jest.advanceTimersByTime(250); // this warns!!! + expect(container.innerHTML).toBe('1'); }); }); From 3c185176b7d2f0fffb68b62d1c66fc215d3dd93e Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Sat, 2 Feb 2019 19:54:09 +0000 Subject: [PATCH 07/31] merge-temp --- .../src/__tests__/ReactTestUtils-test.js | 56 +------------------ 1 file changed, 1 insertion(+), 55 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactTestUtils-test.js b/packages/react-dom/src/__tests__/ReactTestUtils-test.js index 3b99d3d8bdb22..ff4619aa8462a 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtils-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtils-test.js @@ -515,59 +515,5 @@ describe('ReactTestUtils', () => { ReactTestUtils.renderIntoDocument(); expect(mockArgs.length).toEqual(0); }); - - it('detects setState being called outside of .act()', () => { - let setValueRef = null; - function App() { - let [value, setValue] = React.useState(0); - setValueRef = setValue; - return ( - - ); - } - const container = document.createElement('div'); - document.body.appendChild(container) - let button - act(() => { - ReactDOM.render(, container); - button = container.querySelector('#button') - button.dispatchEvent(new MouseEvent('click', {bubbles: true})); - }); - expect(button.innerHTML).toBe('2'); - expect(() => setValueRef(1)).toWarnDev( - ["called a hook's setState outside of .act()"], - {withoutStack: 1}, - ); - document.body.removeChild(container) - }); - - it('lets a ticker update', () => { - function App() { - let [toggle, setToggle] = React.useState(0); - React.useEffect(() => { - let timeout = setTimeout(() => { - setToggle(1); - }, 200); - return () => clearTimeout(timeout); - }); - return toggle; - } - const container = document.createElement('div'); - - act(() => { - act(()=> { - ReactDOM.render(, container); - }) - jest.advanceTimersByTime(250); - }); - - expect(container.innerHTML).toBe('1'); - }); + }); - -function act(fn) { - ReactDOM.unstable_batchedUpdates(fn); - ReactDOM.render(null, document.createElement('div')); -} From 324fe0ee091814fc73bf8c5a875978653c22d220 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Sat, 2 Feb 2019 20:20:11 +0000 Subject: [PATCH 08/31] move jsdom test to callsite --- .../src/__tests__/ReactTestUtils-test.js | 42 +++++++++---------- .../react-reconciler/src/ReactFiberHooks.js | 22 +++++++++- .../src/ReactFiberScheduler.js | 7 +--- 3 files changed, 40 insertions(+), 31 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactTestUtils-test.js b/packages/react-dom/src/__tests__/ReactTestUtils-test.js index ee7dae8f1aad5..e777b53bd30b4 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtils-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtils-test.js @@ -555,19 +555,16 @@ describe('ReactTestUtils', () => { const container = document.createElement('div'); document.body.appendChild(container); let button; - try { - ReactTestUtils.act(() => { - ReactDOM.render(, container); - }); - button = document.getElementById('button'); - expect(button.innerHTML).toBe('0'); - ReactTestUtils.act(() => { - button.dispatchEvent(new MouseEvent('click', {bubbles: true})); - }); - expect(button.innerHTML).toBe('1'); - } finally { - document.body.removeChild(container); - } + ReactTestUtils.act(() => { + ReactDOM.render(, container); + }); + button = document.getElementById('button'); + expect(button.innerHTML).toBe('0'); + ReactTestUtils.act(() => { + button.dispatchEvent(new MouseEvent('click', {bubbles: true})); + }); + expect(button.innerHTML).toBe('1'); + document.body.removeChild(container); }); it('detects setState being called outside of .act()', () => { @@ -582,11 +579,11 @@ describe('ReactTestUtils', () => { ); } const container = document.createElement('div'); - document.body.appendChild(container) - let button + document.body.appendChild(container); + let button; ReactTestUtils.act(() => { ReactDOM.render(, container); - button = container.querySelector('#button') + button = container.querySelector('#button'); button.dispatchEvent(new MouseEvent('click', {bubbles: true})); }); expect(button.innerHTML).toBe('2'); @@ -594,7 +591,7 @@ describe('ReactTestUtils', () => { ["called a hook's setState outside of .act()"], {withoutStack: 1}, ); - document.body.removeChild(container) + document.body.removeChild(container); }); it('lets a ticker update', () => { @@ -609,15 +606,14 @@ describe('ReactTestUtils', () => { return toggle; } const container = document.createElement('div'); - + ReactTestUtils.act(() => { - ReactTestUtils.act(()=> { - ReactDOM.render(, container); - }) + ReactTestUtils.act(() => { + ReactDOM.render(, container); + }); jest.advanceTimersByTime(250); }); - + expect(container.innerHTML).toBe('1'); }); - }); diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 529f2c77a6a04..3859c363dfaf5 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -41,6 +41,7 @@ import invariant from 'shared/invariant'; import warning from 'shared/warning'; import getComponentName from 'shared/getComponentName'; import is from 'shared/objectIs'; +import {canUseDOM} from 'shared/ExecutionEnvironment'; import {markWorkInProgressReceivedUpdate} from './ReactFiberBeginWork'; type Update = { @@ -1005,6 +1006,14 @@ export function useMemo( return nextValue; } +let isTestEnvironment; + +if (__DEV__) { + isTestEnvironment = canUseDOM && + (navigator.userAgent.includes('Node.js') || + navigator.userAgent.includes('jsdom')); +} + function dispatchAction( fiber: Fiber, queue: UpdateQueue, @@ -1127,7 +1136,16 @@ function dispatchAction( } } } - ensureBatchingAndScheduleWork(fiber, expirationTime); - // scheduleWork(fiber, expirationTime); + if (__DEV__) { + // if we're in a test-like environment, warn if we're calling this + // outside of a batchedUpdates/TestUtils.act scope + if (isTestEnvironment) { + ensureBatchingAndScheduleWork(fiber, expirationTime); + } else { + scheduleWork(fiber, expirationTime); + } + } else { + scheduleWork(fiber, expirationTime); + } } } diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 71f7ca737a23e..178eccc62f6ea 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -1789,17 +1789,12 @@ function scheduleWorkToRoot(fiber: Fiber, expirationTime): FiberRoot | null { return root; } -const isJSDOM = - 'undefined' !== typeof global.navigator && - (navigator.userAgent.includes('Node.js') || - navigator.userAgent.includes('jsdom')); - export function ensureBatchingAndScheduleWork( fiber: Fiber, expirationTime: ExpirationTime, ) { if (__DEV__) { - if (isJSDOM && !isBatchingUpdates) { + if (!isBatchingUpdates) { warningWithoutStack(false, "called a hook's setState outside of .act()"); } } From e7ebd84029f732ec7b39082f389ff0355764bab8 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Sat, 2 Feb 2019 22:05:08 +0000 Subject: [PATCH 09/31] mark failing tests --- packages/react-reconciler/src/ReactFiberHooks.js | 7 ++++--- packages/react-reconciler/src/ReactFiberScheduler.js | 2 +- .../src/__tests__/ReactHooks-test.internal.js | 2 +- .../src/__tests__/ReactSuspense-test.internal.js | 4 ++-- 4 files changed, 8 insertions(+), 7 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 3859c363dfaf5..ff7b1084be790 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -1006,10 +1006,11 @@ export function useMemo( return nextValue; } -let isTestEnvironment; +let isTestDOMEnvironment; if (__DEV__) { - isTestEnvironment = canUseDOM && + isTestDOMEnvironment = + canUseDOM && (navigator.userAgent.includes('Node.js') || navigator.userAgent.includes('jsdom')); } @@ -1139,7 +1140,7 @@ function dispatchAction( if (__DEV__) { // if we're in a test-like environment, warn if we're calling this // outside of a batchedUpdates/TestUtils.act scope - if (isTestEnvironment) { + if (isTestDOMEnvironment === true) { ensureBatchingAndScheduleWork(fiber, expirationTime); } else { scheduleWork(fiber, expirationTime); diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 178eccc62f6ea..945cde9d13be6 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -1794,7 +1794,7 @@ export function ensureBatchingAndScheduleWork( expirationTime: ExpirationTime, ) { if (__DEV__) { - if (!isBatchingUpdates) { + if (isBatchingUpdates === false) { warningWithoutStack(false, "called a hook's setState outside of .act()"); } } diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 862d8fbe2bb2d..cd7089af8ac4a 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -81,7 +81,7 @@ describe('ReactHooks', () => { expect(root).toMatchRenderedOutput('0, 0'); // Normal update - setCounter1(1); + setCounter1(1); // why does this not warn? setCounter2(1); expect(root).toFlushAndYield([ 'Parent: 1, 1', diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 73a55370a187c..784dab97a1442 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -797,7 +797,7 @@ describe('ReactSuspense', () => { ]); expect(root).toMatchRenderedOutput('Tab: 0 + sibling'); - setTab(1); + setTab(1); // why does this warn? why is canUseDOM === true here? expect(ReactTestRenderer).toHaveYielded([ 'Suspend! [Tab: 1]', ' + sibling', @@ -811,7 +811,7 @@ describe('ReactSuspense', () => { ]); expect(root).toMatchRenderedOutput('Tab: 1 + sibling'); - setTab(2); + setTab(2); // this too - why does this warn? expect(ReactTestRenderer).toHaveYielded([ 'Suspend! [Tab: 2]', ' + sibling', From 0d3b45a9c2f0300d8d08990ea34481c185a26024 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Sun, 3 Feb 2019 01:12:56 +0000 Subject: [PATCH 10/31] pass most tests (except one) --- .../src/__tests__/ReactDOMSuspensePlaceholder-test.js | 10 ++++++++-- packages/react-reconciler/src/ReactFiberHooks.js | 2 +- .../src/__tests__/ReactHooks-test.internal.js | 2 +- .../src/__tests__/ReactSuspense-test.internal.js | 8 ++++++-- .../src/__tests__/ReactSuspenseFuzz-test.internal.js | 1 + 5 files changed, 17 insertions(+), 6 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.js b/packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.js index bc2ae71673d22..704cb5ee48602 100644 --- a/packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.js @@ -13,6 +13,7 @@ let React; let ReactDOM; let Suspense; let ReactCache; +let ReactTestUtils; let TextResource; describe('ReactDOMSuspensePlaceholder', () => { @@ -23,6 +24,7 @@ describe('ReactDOMSuspensePlaceholder', () => { React = require('react'); ReactDOM = require('react-dom'); ReactCache = require('react-cache'); + ReactTestUtils = require('react-dom/test-utils'); Suspense = React.Suspense; container = document.createElement('div'); document.body.appendChild(container); @@ -142,12 +144,16 @@ describe('ReactDOMSuspensePlaceholder', () => { ); } - ReactDOM.render(, container); + ReactTestUtils.act(() => { + ReactDOM.render(, container); + }); expect(container.innerHTML).toEqual( 'SiblingLoading...', ); - setIsVisible(true); + ReactTestUtils.act(() => { + setIsVisible(true); + }); expect(container.innerHTML).toEqual( 'SiblingLoading...', ); diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index ff7b1084be790..d24aa1a412d1d 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -1006,7 +1006,7 @@ export function useMemo( return nextValue; } -let isTestDOMEnvironment; +let isTestDOMEnvironment = false; if (__DEV__) { isTestDOMEnvironment = diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index cd7089af8ac4a..862d8fbe2bb2d 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -81,7 +81,7 @@ describe('ReactHooks', () => { expect(root).toMatchRenderedOutput('0, 0'); // Normal update - setCounter1(1); // why does this not warn? + setCounter1(1); setCounter2(1); expect(root).toFlushAndYield([ 'Parent: 1, 1', diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 784dab97a1442..27139903177af 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -1,3 +1,7 @@ +/** + * @jest-environment node + */ + let React; let ReactTestRenderer; let ReactFeatureFlags; @@ -797,7 +801,7 @@ describe('ReactSuspense', () => { ]); expect(root).toMatchRenderedOutput('Tab: 0 + sibling'); - setTab(1); // why does this warn? why is canUseDOM === true here? + setTab(1); expect(ReactTestRenderer).toHaveYielded([ 'Suspend! [Tab: 1]', ' + sibling', @@ -811,7 +815,7 @@ describe('ReactSuspense', () => { ]); expect(root).toMatchRenderedOutput('Tab: 1 + sibling'); - setTab(2); // this too - why does this warn? + setTab(2); expect(ReactTestRenderer).toHaveYielded([ 'Suspend! [Tab: 2]', ' + sibling', diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js index ed3d9322ba1c4..4d9a3e10b3842 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js @@ -1,3 +1,4 @@ +// this one's failing by going into stack overflow, investigating... let React; let Suspense; let ReactTestRenderer; From 20a959db6904db4c60a0918184b32e203c0b341a Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Sun, 3 Feb 2019 01:18:08 +0000 Subject: [PATCH 11/31] augh IE --- packages/react-reconciler/src/ReactFiberHooks.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index d24aa1a412d1d..e28d20a59bf57 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -1011,8 +1011,8 @@ let isTestDOMEnvironment = false; if (__DEV__) { isTestDOMEnvironment = canUseDOM && - (navigator.userAgent.includes('Node.js') || - navigator.userAgent.includes('jsdom')); + (navigator.userAgent.indexOf('Node.js') >= 0 || + navigator.userAgent.indexOf('jsdom') >= 0); } function dispatchAction( From ee4ebb93beefd1b7cd1d83ec4bf86be62cd242d1 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Sun, 3 Feb 2019 01:21:30 +0000 Subject: [PATCH 12/31] pass fuzz tests --- .../src/__tests__/ReactSuspenseFuzz-test.internal.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js index 4d9a3e10b3842..efa111146f90c 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js @@ -1,4 +1,7 @@ -// this one's failing by going into stack overflow, investigating... +/** + * @jest-environment node + */ + let React; let Suspense; let ReactTestRenderer; From 4cb92ad90fc62a96e7ee0cb4c6f1d1b53e9e374d Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Sun, 3 Feb 2019 05:32:33 +0000 Subject: [PATCH 13/31] better warning, expose the right batchedUpdates on TestRenderer for www --- .../src/__tests__/ReactTestUtils-test.js | 4 +++- .../react-reconciler/src/ReactFiberHooks.js | 18 +-------------- .../src/ReactFiberScheduler.js | 22 +++++++++++++++++-- .../src/ReactTestRenderer.js | 2 +- 4 files changed, 25 insertions(+), 21 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactTestUtils-test.js b/packages/react-dom/src/__tests__/ReactTestUtils-test.js index e777b53bd30b4..b5377233a7671 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtils-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtils-test.js @@ -588,7 +588,9 @@ describe('ReactTestUtils', () => { }); expect(button.innerHTML).toBe('2'); expect(() => setValueRef(1)).toWarnDev( - ["called a hook's setState outside of .act()"], + [ + 'It looks like you are in a test environment, trying to set state outside of TestUtils.act(...).', + ], {withoutStack: 1}, ); document.body.removeChild(container); diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index e28d20a59bf57..be29e009f52d2 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -41,7 +41,6 @@ import invariant from 'shared/invariant'; import warning from 'shared/warning'; import getComponentName from 'shared/getComponentName'; import is from 'shared/objectIs'; -import {canUseDOM} from 'shared/ExecutionEnvironment'; import {markWorkInProgressReceivedUpdate} from './ReactFiberBeginWork'; type Update = { @@ -1006,15 +1005,6 @@ export function useMemo( return nextValue; } -let isTestDOMEnvironment = false; - -if (__DEV__) { - isTestDOMEnvironment = - canUseDOM && - (navigator.userAgent.indexOf('Node.js') >= 0 || - navigator.userAgent.indexOf('jsdom') >= 0); -} - function dispatchAction( fiber: Fiber, queue: UpdateQueue, @@ -1138,13 +1128,7 @@ function dispatchAction( } } if (__DEV__) { - // if we're in a test-like environment, warn if we're calling this - // outside of a batchedUpdates/TestUtils.act scope - if (isTestDOMEnvironment === true) { - ensureBatchingAndScheduleWork(fiber, expirationTime); - } else { - scheduleWork(fiber, expirationTime); - } + ensureBatchingAndScheduleWork(fiber, expirationTime); } else { scheduleWork(fiber, expirationTime); } diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 945cde9d13be6..0f51143a717e5 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -65,6 +65,7 @@ import { import getComponentName from 'shared/getComponentName'; import invariant from 'shared/invariant'; import warningWithoutStack from 'shared/warningWithoutStack'; +import {canUseDOM} from 'shared/ExecutionEnvironment'; import ReactFiberInstrumentation from './ReactFiberInstrumentation'; import { @@ -1789,13 +1790,30 @@ function scheduleWorkToRoot(fiber: Fiber, expirationTime): FiberRoot | null { return root; } +let isTestDOMEnvironment = false; + +if (__DEV__) { + isTestDOMEnvironment = + canUseDOM && + (navigator.userAgent.indexOf('Node.js') >= 0 || + navigator.userAgent.indexOf('jsdom') >= 0); +} + +// if we're in a test-like environment, warn if we're calling this +// outside of a batchedUpdates/TestUtils.act scope export function ensureBatchingAndScheduleWork( fiber: Fiber, expirationTime: ExpirationTime, ) { if (__DEV__) { - if (isBatchingUpdates === false) { - warningWithoutStack(false, "called a hook's setState outside of .act()"); + if (isTestDOMEnvironment === true && isBatchingUpdates === false) { + warningWithoutStack( + false, + 'It looks like you are in a test environment, trying to ' + + 'set state outside of TestUtils.act(...). ' + + 'This could lead to unexpected ui while testing. Use ' + + 'ReactTestUtils.act(...) to batch your updates and remove this warning.', + ); } } scheduleWork(fiber, expirationTime); diff --git a/packages/react-test-renderer/src/ReactTestRenderer.js b/packages/react-test-renderer/src/ReactTestRenderer.js index fda60e171f460..b7633a735811d 100644 --- a/packages/react-test-renderer/src/ReactTestRenderer.js +++ b/packages/react-test-renderer/src/ReactTestRenderer.js @@ -17,8 +17,8 @@ import { updateContainer, flushSync, injectIntoDevTools, + batchedUpdates, } from 'react-reconciler/inline.test'; -import {batchedUpdates} from 'events/ReactGenericBatching'; import {findCurrentFiberUsingSlowPath} from 'react-reconciler/reflection'; import { Fragment, From aa1d5311ba0e08d1bbf0d52b73a0476292c61d43 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Sun, 3 Feb 2019 16:07:52 +0000 Subject: [PATCH 14/31] move it into hooks, test for dom --- .../src/test-utils/ReactTestUtils.js | 3 +- .../react-reconciler/src/ReactFiberHooks.js | 60 ++++++++++++++++++- .../src/ReactFiberScheduler.js | 30 +--------- .../__tests__/ReactSuspense-test.internal.js | 4 -- .../ReactSuspenseFuzz-test.internal.js | 4 -- .../src/ReactTestRenderer.js | 2 +- 6 files changed, 64 insertions(+), 39 deletions(-) diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js index 1ac617adec299..07697a61c6ef4 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtils.js +++ b/packages/react-dom/src/test-utils/ReactTestUtils.js @@ -386,8 +386,9 @@ const ReactTestUtils = { SimulateNative: {}, act(callback: () => void) { - ReactDOM.unstable_batchedUpdates(callback); + let result = ReactDOM.unstable_batchedUpdates(callback); ReactDOM.render(actElement, actContainerElement); + return result; }, }; diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index be29e009f52d2..335b8e8c7cd25 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -31,7 +31,7 @@ import { } from './ReactHookEffectTags'; import { scheduleWork, - ensureBatchingAndScheduleWork, + getBatchingStatus, computeExpirationForFiber, flushPassiveEffects, requestCurrentTime, @@ -41,6 +41,7 @@ import invariant from 'shared/invariant'; import warning from 'shared/warning'; import getComponentName from 'shared/getComponentName'; import is from 'shared/objectIs'; +import {canUseDOM} from 'shared/ExecutionEnvironment'; import {markWorkInProgressReceivedUpdate} from './ReactFiberBeginWork'; type Update = { @@ -1134,3 +1135,60 @@ function dispatchAction( } } } + +// in a dom-like test environment, we want to warn if dispatchAction() +// is called outside of a batchedUpdates/TestUtils.act(...) call. + +let isNotDOMRenderer = undefined; +// some folks initialize a jsdom, but still use TestRenderer. +// So when dispatchAction is called first, we'll test the +// fiber tree and set this value. + +let ensureBatchingAndScheduleWork = ( + fiber: Fiber, + expirationTime: ExpirationTime, +) => { + scheduleWork(fiber, expirationTime); +}; +// we'll swap out the function definition when we learn more about the environment + +function ensureBatchingAndScheduleWorkImpl( + fiber: Fiber, + expirationTime: ExpirationTime, +) { + if (__DEV__) { + if (isNotDOMRenderer === undefined) { + let f = fiber; + while (f.return) { + f = f.return; + } + if (f.stateNode.containerInfo instanceof HTMLElement) { + isNotDOMRenderer = false; + } else { + isNotDOMRenderer = true; + ensureBatchingAndScheduleWork = scheduleWork; + } + } + if (isNotDOMRenderer === false && getBatchingStatus() === false) { + warning( + false, + 'It looks like you are in a test environment, trying to ' + + 'set state outside of TestUtils.act(...). ' + + 'This could lead to unexpected ui while testing. Use ' + + 'ReactTestUtils.act(...) to batch your updates and remove this warning.', + ); + } + } + scheduleWork(fiber, expirationTime); +} + +if (__DEV__) { + if ( + canUseDOM && + (navigator.userAgent.indexOf('Node.js') >= 0 || + navigator.userAgent.indexOf('jsdom') >= 0) + // we should probably add puppeteer-like environments here too + ) { + ensureBatchingAndScheduleWork = ensureBatchingAndScheduleWorkImpl; + } +} diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 0f51143a717e5..be6c6aa87e732 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -65,7 +65,6 @@ import { import getComponentName from 'shared/getComponentName'; import invariant from 'shared/invariant'; import warningWithoutStack from 'shared/warningWithoutStack'; -import {canUseDOM} from 'shared/ExecutionEnvironment'; import ReactFiberInstrumentation from './ReactFiberInstrumentation'; import { @@ -1790,33 +1789,8 @@ function scheduleWorkToRoot(fiber: Fiber, expirationTime): FiberRoot | null { return root; } -let isTestDOMEnvironment = false; - -if (__DEV__) { - isTestDOMEnvironment = - canUseDOM && - (navigator.userAgent.indexOf('Node.js') >= 0 || - navigator.userAgent.indexOf('jsdom') >= 0); -} - -// if we're in a test-like environment, warn if we're calling this -// outside of a batchedUpdates/TestUtils.act scope -export function ensureBatchingAndScheduleWork( - fiber: Fiber, - expirationTime: ExpirationTime, -) { - if (__DEV__) { - if (isTestDOMEnvironment === true && isBatchingUpdates === false) { - warningWithoutStack( - false, - 'It looks like you are in a test environment, trying to ' + - 'set state outside of TestUtils.act(...). ' + - 'This could lead to unexpected ui while testing. Use ' + - 'ReactTestUtils.act(...) to batch your updates and remove this warning.', - ); - } - } - scheduleWork(fiber, expirationTime); +export function getBatchingStatus(): boolean { + return isBatchingUpdates; } function scheduleWork(fiber: Fiber, expirationTime: ExpirationTime) { diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 27139903177af..73a55370a187c 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -1,7 +1,3 @@ -/** - * @jest-environment node - */ - let React; let ReactTestRenderer; let ReactFeatureFlags; diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js index efa111146f90c..ed3d9322ba1c4 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js @@ -1,7 +1,3 @@ -/** - * @jest-environment node - */ - let React; let Suspense; let ReactTestRenderer; diff --git a/packages/react-test-renderer/src/ReactTestRenderer.js b/packages/react-test-renderer/src/ReactTestRenderer.js index b7633a735811d..fda60e171f460 100644 --- a/packages/react-test-renderer/src/ReactTestRenderer.js +++ b/packages/react-test-renderer/src/ReactTestRenderer.js @@ -17,8 +17,8 @@ import { updateContainer, flushSync, injectIntoDevTools, - batchedUpdates, } from 'react-reconciler/inline.test'; +import {batchedUpdates} from 'events/ReactGenericBatching'; import {findCurrentFiberUsingSlowPath} from 'react-reconciler/reflection'; import { Fragment, From 00d0614b0c9a69d56ecb52239e634cc2b59af30e Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Sun, 3 Feb 2019 17:10:42 +0000 Subject: [PATCH 15/31] expose a flag on the host config, move stuff around --- .../src/client/ReactDOMHostConfig.js | 18 +++++ .../react-reconciler/src/ReactFiberHooks.js | 67 ++----------------- .../src/ReactFiberScheduler.js | 18 ++++- 3 files changed, 41 insertions(+), 62 deletions(-) diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index 86b88ce2bfada..06d88597a9251 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -39,6 +39,7 @@ import { DOCUMENT_FRAGMENT_NODE, } from '../shared/HTMLNodeType'; import dangerousStyleValue from '../shared/dangerousStyleValue'; +import {canUseDOM} from 'shared/ExecutionEnvironment'; import type {DOMContainer} from './ReactDOM'; @@ -647,3 +648,20 @@ export function didNotFindHydratableTextInstance( warnForInsertedHydratedText(parentInstance, text); } } + +// in a dom-like test environment, we want to warn if dispatchAction() +// is called outside of a batchedUpdates/TestUtils.act(...) call. +let ensureBatchedDispatchedActions = false; + +if (__DEV__) { + if ( + canUseDOM && + (navigator.userAgent.indexOf('Node.js') >= 0 || + navigator.userAgent.indexOf('jsdom') >= 0) + // we should probably add puppeteer-like environments here too + ) { + ensureBatchedDispatchedActions = true; + } +} + +export const shouldBatchDispatchedActions = ensureBatchedDispatchedActions; diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 335b8e8c7cd25..d068a29a11bf1 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -31,17 +31,17 @@ import { } from './ReactHookEffectTags'; import { scheduleWork, - getBatchingStatus, + ensureBatchingAndScheduleWork, computeExpirationForFiber, flushPassiveEffects, requestCurrentTime, } from './ReactFiberScheduler'; +import {shouldBatchDispatchedActions} from './ReactFiberHostConfig'; import invariant from 'shared/invariant'; import warning from 'shared/warning'; import getComponentName from 'shared/getComponentName'; import is from 'shared/objectIs'; -import {canUseDOM} from 'shared/ExecutionEnvironment'; import {markWorkInProgressReceivedUpdate} from './ReactFiberBeginWork'; type Update = { @@ -1129,66 +1129,13 @@ function dispatchAction( } } if (__DEV__) { - ensureBatchingAndScheduleWork(fiber, expirationTime); - } else { - scheduleWork(fiber, expirationTime); - } - } -} - -// in a dom-like test environment, we want to warn if dispatchAction() -// is called outside of a batchedUpdates/TestUtils.act(...) call. - -let isNotDOMRenderer = undefined; -// some folks initialize a jsdom, but still use TestRenderer. -// So when dispatchAction is called first, we'll test the -// fiber tree and set this value. - -let ensureBatchingAndScheduleWork = ( - fiber: Fiber, - expirationTime: ExpirationTime, -) => { - scheduleWork(fiber, expirationTime); -}; -// we'll swap out the function definition when we learn more about the environment - -function ensureBatchingAndScheduleWorkImpl( - fiber: Fiber, - expirationTime: ExpirationTime, -) { - if (__DEV__) { - if (isNotDOMRenderer === undefined) { - let f = fiber; - while (f.return) { - f = f.return; - } - if (f.stateNode.containerInfo instanceof HTMLElement) { - isNotDOMRenderer = false; + if (shouldBatchDispatchedActions === true) { + ensureBatchingAndScheduleWork(fiber, expirationTime); } else { - isNotDOMRenderer = true; - ensureBatchingAndScheduleWork = scheduleWork; + scheduleWork(fiber, expirationTime); } + } else { + scheduleWork(fiber, expirationTime); } - if (isNotDOMRenderer === false && getBatchingStatus() === false) { - warning( - false, - 'It looks like you are in a test environment, trying to ' + - 'set state outside of TestUtils.act(...). ' + - 'This could lead to unexpected ui while testing. Use ' + - 'ReactTestUtils.act(...) to batch your updates and remove this warning.', - ); - } - } - scheduleWork(fiber, expirationTime); -} - -if (__DEV__) { - if ( - canUseDOM && - (navigator.userAgent.indexOf('Node.js') >= 0 || - navigator.userAgent.indexOf('jsdom') >= 0) - // we should probably add puppeteer-like environments here too - ) { - ensureBatchingAndScheduleWork = ensureBatchingAndScheduleWorkImpl; } } diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index be6c6aa87e732..c16d672bb8a2f 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -1789,8 +1789,22 @@ function scheduleWorkToRoot(fiber: Fiber, expirationTime): FiberRoot | null { return root; } -export function getBatchingStatus(): boolean { - return isBatchingUpdates; +export function ensureBatchingAndScheduleWork( + fiber: Fiber, + expirationTime: ExpirationTime, +) { + if (__DEV__) { + if (isBatchingUpdates === false) { + warningWithoutStack( + false, + 'It looks like you are in a test environment, trying to ' + + 'set state outside of TestUtils.act(...). ' + + 'This could lead to unexpected ui while testing. Use ' + + 'ReactTestUtils.act(...) to batch your updates and remove this warning.', + ); + } + } + scheduleWork(fiber, expirationTime); } function scheduleWork(fiber: Fiber, expirationTime: ExpirationTime) { From 1b2e6577ebca88b541ec5d1a07645e64c64fb022 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Sun, 3 Feb 2019 18:14:13 +0000 Subject: [PATCH 16/31] rename, pass flow --- packages/react-dom/src/client/ReactDOMHostConfig.js | 2 +- packages/react-native-renderer/src/ReactFabricHostConfig.js | 2 ++ packages/react-native-renderer/src/ReactNativeHostConfig.js | 2 ++ packages/react-reconciler/src/ReactFiberHooks.js | 4 ++-- .../react-reconciler/src/forks/ReactFiberHostConfig.custom.js | 3 +++ packages/react-test-renderer/src/ReactTestHostConfig.js | 2 ++ 6 files changed, 12 insertions(+), 3 deletions(-) diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index 06d88597a9251..16da1a4eb5602 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -664,4 +664,4 @@ if (__DEV__) { } } -export const shouldBatchDispatchedActions = ensureBatchedDispatchedActions; +export const shouldWarnForUnbatchedSetState = ensureBatchedDispatchedActions; diff --git a/packages/react-native-renderer/src/ReactFabricHostConfig.js b/packages/react-native-renderer/src/ReactFabricHostConfig.js index b431f9bee4ec3..1cf8cbc359079 100644 --- a/packages/react-native-renderer/src/ReactFabricHostConfig.js +++ b/packages/react-native-renderer/src/ReactFabricHostConfig.js @@ -436,3 +436,5 @@ export function replaceContainerChildren( container: Container, newChildren: ChildSet, ): void {} + +export const shouldWarnForUnbatchedSetState = false; diff --git a/packages/react-native-renderer/src/ReactNativeHostConfig.js b/packages/react-native-renderer/src/ReactNativeHostConfig.js index 785e98c149359..2e961e5f4e5c3 100644 --- a/packages/react-native-renderer/src/ReactNativeHostConfig.js +++ b/packages/react-native-renderer/src/ReactNativeHostConfig.js @@ -486,3 +486,5 @@ export function unhideTextInstance( ): void { throw new Error('Not yet implemented.'); } + +export const shouldWarnForUnbatchedSetState = false; diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index d068a29a11bf1..c050b19b50fc3 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -37,7 +37,7 @@ import { requestCurrentTime, } from './ReactFiberScheduler'; -import {shouldBatchDispatchedActions} from './ReactFiberHostConfig'; +import {shouldWarnForUnbatchedSetState} from './ReactFiberHostConfig'; import invariant from 'shared/invariant'; import warning from 'shared/warning'; import getComponentName from 'shared/getComponentName'; @@ -1129,7 +1129,7 @@ function dispatchAction( } } if (__DEV__) { - if (shouldBatchDispatchedActions === true) { + if (shouldWarnForUnbatchedSetState === true) { ensureBatchingAndScheduleWork(fiber, expirationTime); } else { scheduleWork(fiber, expirationTime); diff --git a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js index b217c22f1c6bf..fbbf56b6c5d48 100644 --- a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js +++ b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js @@ -121,3 +121,6 @@ export const didNotFindHydratableInstance = $$$hostConfig.didNotFindHydratableInstance; export const didNotFindHydratableTextInstance = $$$hostConfig.didNotFindHydratableTextInstance; + +export const shouldWarnForUnbatchedSetState = + $$$hostConfig.shouldWarnForUnbatchedSetState; diff --git a/packages/react-test-renderer/src/ReactTestHostConfig.js b/packages/react-test-renderer/src/ReactTestHostConfig.js index b4e6f681fac3b..8226739839070 100644 --- a/packages/react-test-renderer/src/ReactTestHostConfig.js +++ b/packages/react-test-renderer/src/ReactTestHostConfig.js @@ -272,3 +272,5 @@ export function unhideTextInstance( ): void { textInstance.isHidden = false; } + +export const shouldWarnForUnbatchedSetState = false; From 86a443ebd1219a2188c8bf76d74f76a2f5623f94 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Sun, 3 Feb 2019 18:26:06 +0000 Subject: [PATCH 17/31] pass flow... again --- packages/react-art/src/ReactARTHostConfig.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/react-art/src/ReactARTHostConfig.js b/packages/react-art/src/ReactARTHostConfig.js index 034b2d7290a1f..d2c770aed86bd 100644 --- a/packages/react-art/src/ReactARTHostConfig.js +++ b/packages/react-art/src/ReactARTHostConfig.js @@ -424,3 +424,5 @@ export function unhideInstance(instance, props) { export function unhideTextInstance(textInstance, text): void { // Noop } + +export const shouldWarnForUnbatchedSetState = false; From cec1ed109ee007ab84cefe64e867c196bbe81259 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Sun, 3 Feb 2019 23:04:52 +0000 Subject: [PATCH 18/31] tweak .act() type --- packages/react-dom/src/test-utils/ReactTestUtils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js index 07697a61c6ef4..077b3e36c74af 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtils.js +++ b/packages/react-dom/src/test-utils/ReactTestUtils.js @@ -385,7 +385,7 @@ const ReactTestUtils = { Simulate: null, SimulateNative: {}, - act(callback: () => void) { + act(callback: () => X): X { let result = ReactDOM.unstable_batchedUpdates(callback); ReactDOM.render(actElement, actContainerElement); return result; From 6fa420249d2b96b322d39614358fe4df0188d8df Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Mon, 4 Feb 2019 16:14:04 +0000 Subject: [PATCH 19/31] enable for all jest environments/renderers; pass (most) tests. --- packages/react-art/src/ReactARTHostConfig.js | 2 - .../ReactHooksInspectionIntegration-test.js | 12 +- .../ReactDOMSuspensePlaceholder-test.js | 10 +- .../src/__tests__/ReactTestUtils-test.js | 69 ++++++++-- .../src/client/ReactDOMHostConfig.js | 18 --- .../src/test-utils/ReactTestUtils.js | 5 +- .../src/ReactFabricHostConfig.js | 2 - .../src/ReactNativeHostConfig.js | 2 - .../src/createReactNoop.js | 6 + .../react-reconciler/src/ReactFiberHooks.js | 14 +- .../src/ReactFiberScheduler.js | 4 +- .../src/__tests__/ReactHooks-test.internal.js | 129 +++++++++++------- ...eactHooksWithNoopRenderer-test.internal.js | 95 +++++++------ .../__tests__/ReactSuspense-test.internal.js | 8 +- .../src/forks/ReactFiberHostConfig.custom.js | 3 - .../src/ReactTestHostConfig.js | 2 - .../src/ReactTestRenderer.js | 6 +- 17 files changed, 238 insertions(+), 149 deletions(-) diff --git a/packages/react-art/src/ReactARTHostConfig.js b/packages/react-art/src/ReactARTHostConfig.js index d2c770aed86bd..034b2d7290a1f 100644 --- a/packages/react-art/src/ReactARTHostConfig.js +++ b/packages/react-art/src/ReactARTHostConfig.js @@ -424,5 +424,3 @@ export function unhideInstance(instance, props) { export function unhideTextInstance(textInstance, text): void { // Noop } - -export const shouldWarnForUnbatchedSetState = false; diff --git a/packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js b/packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js index 33d7cec90ee68..91c4ee0ef77e3 100644 --- a/packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js +++ b/packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js @@ -13,12 +13,14 @@ let React; let ReactTestRenderer; let ReactDebugTools; +let act; -describe('ReactHooksInspectionIntergration', () => { +describe('ReactHooksInspectionIntegration', () => { beforeEach(() => { jest.resetModules(); React = require('react'); ReactTestRenderer = require('react-test-renderer'); + act = ReactTestRenderer.act; ReactDebugTools = require('react-debug-tools'); }); @@ -47,7 +49,7 @@ describe('ReactHooksInspectionIntergration', () => { onMouseUp: setStateB, } = renderer.root.findByType('div').props; - setStateA('Hi'); + act(() => setStateA('Hi')); childFiber = renderer.root.findByType(Foo)._currentFiber(); tree = ReactDebugTools.inspectHooksOfFiber(childFiber); @@ -57,7 +59,7 @@ describe('ReactHooksInspectionIntergration', () => { {name: 'State', value: 'world', subHooks: []}, ]); - setStateB('world!'); + act(() => setStateB('world!')); childFiber = renderer.root.findByType(Foo)._currentFiber(); tree = ReactDebugTools.inspectHooksOfFiber(childFiber); @@ -120,7 +122,7 @@ describe('ReactHooksInspectionIntergration', () => { {name: 'Callback', value: updateStates, subHooks: []}, ]); - updateStates(); + act(updateStates); childFiber = renderer.root.findByType(Foo)._currentFiber(); tree = ReactDebugTools.inspectHooksOfFiber(childFiber); @@ -132,7 +134,7 @@ describe('ReactHooksInspectionIntergration', () => { {name: 'LayoutEffect', value: effect, subHooks: []}, {name: 'Effect', value: effect, subHooks: []}, {name: 'ImperativeHandle', value: outsideRef.current, subHooks: []}, - {name: 'Memo', value: 'Ab', subHooks: []}, + {name: 'Memo', value: 'AB', subHooks: []}, {name: 'Callback', value: updateStates, subHooks: []}, ]); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.js b/packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.js index 704cb5ee48602..e867bcf04f98d 100644 --- a/packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.js @@ -15,6 +15,7 @@ let Suspense; let ReactCache; let ReactTestUtils; let TextResource; +let act; describe('ReactDOMSuspensePlaceholder', () => { let container; @@ -25,6 +26,7 @@ describe('ReactDOMSuspensePlaceholder', () => { ReactDOM = require('react-dom'); ReactCache = require('react-cache'); ReactTestUtils = require('react-dom/test-utils'); + act = ReactTestUtils.act; Suspense = React.Suspense; container = document.createElement('div'); document.body.appendChild(container); @@ -144,16 +146,12 @@ describe('ReactDOMSuspensePlaceholder', () => { ); } - ReactTestUtils.act(() => { - ReactDOM.render(, container); - }); + act(() => ReactDOM.render(, container)); expect(container.innerHTML).toEqual( 'SiblingLoading...', ); - ReactTestUtils.act(() => { - setIsVisible(true); - }); + act(() => setIsVisible(true)); expect(container.innerHTML).toEqual( 'SiblingLoading...', ); diff --git a/packages/react-dom/src/__tests__/ReactTestUtils-test.js b/packages/react-dom/src/__tests__/ReactTestUtils-test.js index b5377233a7671..cd2f658510a1e 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtils-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtils-test.js @@ -14,6 +14,7 @@ let React; let ReactDOM; let ReactDOMServer; let ReactTestUtils; +let act; function getTestDocument(markup) { const doc = document.implementation.createHTMLDocument(''); @@ -33,6 +34,7 @@ describe('ReactTestUtils', () => { ReactDOM = require('react-dom'); ReactDOMServer = require('react-dom/server'); ReactTestUtils = require('react-dom/test-utils'); + act = ReactTestUtils.act; }); it('Simulate should have locally attached media events', () => { @@ -526,7 +528,7 @@ describe('ReactTestUtils', () => { try { let called = false; - ReactTestUtils.act(() => { + act(() => { ReactDOM.render( { @@ -543,6 +545,51 @@ describe('ReactTestUtils', () => { } }); + it('flushes effects on every call', () => { + function App(props) { + let [ctr, setCtr] = React.useState(0); + React.useEffect(() => { + props.callback(ctr); + }); + return ( + + ); + } + + const container = document.createElement('div'); + document.body.appendChild(container); + let calledCtr = 0; + act(() => + ReactDOM.render( + { + calledCtr = val; + }} + />, + container, + ), + ); + const button = document.getElementById('button'); + function click() { + button.dispatchEvent(new MouseEvent('click', {bubbles: true})); + } + + act(() => { + click(); + click(); + click(); + }); + expect(calledCtr).toBe(3); + act(click); + expect(calledCtr).toBe(4); + act(click); + expect(calledCtr).toBe(5); + + document.body.removeChild(container); + }); + it('can use act to batch effects on updates too', () => { function App() { let [ctr, setCtr] = React.useState(0); @@ -555,19 +602,15 @@ describe('ReactTestUtils', () => { const container = document.createElement('div'); document.body.appendChild(container); let button; - ReactTestUtils.act(() => { - ReactDOM.render(, container); - }); + act(() => ReactDOM.render(, container)); button = document.getElementById('button'); expect(button.innerHTML).toBe('0'); - ReactTestUtils.act(() => { - button.dispatchEvent(new MouseEvent('click', {bubbles: true})); - }); + act(() => button.dispatchEvent(new MouseEvent('click', {bubbles: true}))); expect(button.innerHTML).toBe('1'); document.body.removeChild(container); }); - it('detects setState being called outside of .act()', () => { + it('detects setState being called outside of act(...)', () => { let setValueRef = null; function App() { let [value, setValue] = React.useState(0); @@ -581,7 +624,7 @@ describe('ReactTestUtils', () => { const container = document.createElement('div'); document.body.appendChild(container); let button; - ReactTestUtils.act(() => { + act(() => { ReactDOM.render(, container); button = container.querySelector('#button'); button.dispatchEvent(new MouseEvent('click', {bubbles: true})); @@ -589,7 +632,7 @@ describe('ReactTestUtils', () => { expect(button.innerHTML).toBe('2'); expect(() => setValueRef(1)).toWarnDev( [ - 'It looks like you are in a test environment, trying to set state outside of TestUtils.act(...).', + 'It looks like you are in a test environment, trying to set state outside of an act(...) call.', ], {withoutStack: 1}, ); @@ -609,10 +652,8 @@ describe('ReactTestUtils', () => { } const container = document.createElement('div'); - ReactTestUtils.act(() => { - ReactTestUtils.act(() => { - ReactDOM.render(, container); - }); + act(() => { + act(() => ReactDOM.render(, container)); jest.advanceTimersByTime(250); }); diff --git a/packages/react-dom/src/client/ReactDOMHostConfig.js b/packages/react-dom/src/client/ReactDOMHostConfig.js index 16da1a4eb5602..86b88ce2bfada 100644 --- a/packages/react-dom/src/client/ReactDOMHostConfig.js +++ b/packages/react-dom/src/client/ReactDOMHostConfig.js @@ -39,7 +39,6 @@ import { DOCUMENT_FRAGMENT_NODE, } from '../shared/HTMLNodeType'; import dangerousStyleValue from '../shared/dangerousStyleValue'; -import {canUseDOM} from 'shared/ExecutionEnvironment'; import type {DOMContainer} from './ReactDOM'; @@ -648,20 +647,3 @@ export function didNotFindHydratableTextInstance( warnForInsertedHydratedText(parentInstance, text); } } - -// in a dom-like test environment, we want to warn if dispatchAction() -// is called outside of a batchedUpdates/TestUtils.act(...) call. -let ensureBatchedDispatchedActions = false; - -if (__DEV__) { - if ( - canUseDOM && - (navigator.userAgent.indexOf('Node.js') >= 0 || - navigator.userAgent.indexOf('jsdom') >= 0) - // we should probably add puppeteer-like environments here too - ) { - ensureBatchedDispatchedActions = true; - } -} - -export const shouldWarnForUnbatchedSetState = ensureBatchedDispatchedActions; diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js index 077b3e36c74af..cbd1a7a30a6ab 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtils.js +++ b/packages/react-dom/src/test-utils/ReactTestUtils.js @@ -145,8 +145,7 @@ function validateClassInstance(inst, methodName) { ); } -// stub elements used by act() when flushing effects -let actElement =
; +// stub element used by act() when flushing effects let actContainerElement = document.createElement('div'); /** @@ -387,7 +386,7 @@ const ReactTestUtils = { act(callback: () => X): X { let result = ReactDOM.unstable_batchedUpdates(callback); - ReactDOM.render(actElement, actContainerElement); + ReactDOM.render(
, actContainerElement); return result; }, }; diff --git a/packages/react-native-renderer/src/ReactFabricHostConfig.js b/packages/react-native-renderer/src/ReactFabricHostConfig.js index 1cf8cbc359079..b431f9bee4ec3 100644 --- a/packages/react-native-renderer/src/ReactFabricHostConfig.js +++ b/packages/react-native-renderer/src/ReactFabricHostConfig.js @@ -436,5 +436,3 @@ export function replaceContainerChildren( container: Container, newChildren: ChildSet, ): void {} - -export const shouldWarnForUnbatchedSetState = false; diff --git a/packages/react-native-renderer/src/ReactNativeHostConfig.js b/packages/react-native-renderer/src/ReactNativeHostConfig.js index 2e961e5f4e5c3..785e98c149359 100644 --- a/packages/react-native-renderer/src/ReactNativeHostConfig.js +++ b/packages/react-native-renderer/src/ReactNativeHostConfig.js @@ -486,5 +486,3 @@ export function unhideTextInstance( ): void { throw new Error('Not yet implemented.'); } - -export const shouldWarnForUnbatchedSetState = false; diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 42726f4130359..21b173ed1abd9 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -848,6 +848,12 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { interactiveUpdates: NoopRenderer.interactiveUpdates, + // maybe this should exist only in the test file + act(callback: void => X): X { + return NoopRenderer.batchedUpdates(callback); + // flush updates? + }, + flushSync(fn: () => mixed) { yieldedValues = []; NoopRenderer.flushSync(fn); diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 8e59dc019b648..ab541be4ddfc9 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -36,7 +36,6 @@ import { requestCurrentTime, } from './ReactFiberScheduler'; -import {shouldWarnForUnbatchedSetState} from './ReactFiberHostConfig'; import invariant from 'shared/invariant'; import warning from 'shared/warning'; import getComponentName from 'shared/getComponentName'; @@ -1005,6 +1004,19 @@ function updateMemo( return nextValue; } +// in a test-like environment, we want to warn if dispatchAction() +// is called outside of a batchedUpdates/TestUtils.act(...) call. +let shouldWarnForUnbatchedSetState = false; + +if (__DEV__) { + // jest isnt' a 'global', it's just exposed to tests via a wrapped function + // further, this isn't a test file, so flow doesn't recognize the symbol. So... + // $FlowExpectedError - because requirements don't give a damn about your type sigs. + if ('undefined' !== typeof jest) { + shouldWarnForUnbatchedSetState = true; + } +} + function dispatchAction( fiber: Fiber, queue: UpdateQueue, diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 314705f1519f2..27246cdc49a09 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -1799,9 +1799,9 @@ export function ensureBatchingAndScheduleWork( warningWithoutStack( false, 'It looks like you are in a test environment, trying to ' + - 'set state outside of TestUtils.act(...). ' + + 'set state outside of an act(...) call. ' + 'This could lead to unexpected ui while testing. Use ' + - 'ReactTestUtils.act(...) to batch your updates and remove this warning.', + 'act(...) to batch your updates and remove this warning.', ); } } diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 5b1477a20572f..2761dae6027fe 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -16,6 +16,7 @@ let React; let ReactFeatureFlags; let ReactTestRenderer; let ReactDOMServer; +let act; // Additional tests can be found in ReactHooksWithNoopRenderer. Plan is to // gradually migrate those to this file. @@ -28,6 +29,7 @@ describe('ReactHooks', () => { React = require('react'); ReactTestRenderer = require('react-test-renderer'); ReactDOMServer = require('react-dom/server'); + act = ReactTestRenderer.act; }); if (__DEV__) { @@ -81,8 +83,11 @@ describe('ReactHooks', () => { expect(root).toMatchRenderedOutput('0, 0'); // Normal update - setCounter1(1); - setCounter2(1); + act(() => { + setCounter1(1); + setCounter2(1); + }); + expect(root).toFlushAndYield([ 'Parent: 1, 1', 'Child: 1, 1', @@ -90,13 +95,16 @@ describe('ReactHooks', () => { ]); // Update that bails out. - setCounter1(1); + act(() => setCounter1(1)); expect(root).toFlushAndYield(['Parent: 1, 1']); // This time, one of the state updates but the other one doesn't. So we // can't bail out. - setCounter1(1); - setCounter2(2); + act(() => { + setCounter1(1); + setCounter2(2); + }); + expect(root).toFlushAndYield([ 'Parent: 1, 2', 'Child: 1, 2', @@ -104,12 +112,14 @@ describe('ReactHooks', () => { ]); // Lots of updates that eventually resolve to the current values. - setCounter1(9); - setCounter2(3); - setCounter1(4); - setCounter2(7); - setCounter1(1); - setCounter2(2); + act(() => { + setCounter1(9); + setCounter2(3); + setCounter1(4); + setCounter2(7); + setCounter1(1); + setCounter2(2); + }); // Because the final values are the same as the current values, the // component bails out. @@ -148,21 +158,27 @@ describe('ReactHooks', () => { expect(root).toMatchRenderedOutput('0, 0 (light)'); // Normal update - setCounter1(1); - setCounter2(1); + act(() => { + setCounter1(1); + setCounter2(1); + }); + expect(root).toFlushAndYield([ 'Parent: 1, 1 (light)', 'Child: 1, 1 (light)', ]); // Update that bails out. - setCounter1(1); + act(() => setCounter1(1)); expect(root).toFlushAndYield(['Parent: 1, 1 (light)']); // This time, one of the state updates but the other one doesn't. So we // can't bail out. - setCounter1(1); - setCounter2(2); + act(() => { + setCounter1(1); + setCounter2(2); + }); + expect(root).toFlushAndYield([ 'Parent: 1, 2 (light)', 'Child: 1, 2 (light)', @@ -170,14 +186,20 @@ describe('ReactHooks', () => { // Updates bail out, but component still renders because props // have changed - setCounter1(1); - setCounter2(2); + act(() => { + setCounter1(1); + setCounter2(2); + }); + root.update(); expect(root).toFlushAndYield(['Parent: 1, 2 (dark)', 'Child: 1, 2 (dark)']); // Both props and state bail out - setCounter1(1); - setCounter2(2); + act(() => { + setCounter1(1); + setCounter2(2); + }); + root.update(); expect(root).toFlushAndYield(['Parent: 1, 2 (dark)']); }); @@ -200,9 +222,11 @@ describe('ReactHooks', () => { expect(root).toMatchRenderedOutput('0'); expect(() => { - setCounter(1, () => { - throw new Error('Expected to ignore the callback.'); - }); + act(() => + setCounter(1, () => { + throw new Error('Expected to ignore the callback.'); + }), + ); }).toWarnDev( 'State updates from the useState() and useReducer() Hooks ' + "don't support the second callback argument. " + @@ -232,9 +256,11 @@ describe('ReactHooks', () => { expect(root).toMatchRenderedOutput('0'); expect(() => { - dispatch(1, () => { - throw new Error('Expected to ignore the callback.'); - }); + act(() => + dispatch(1, () => { + throw new Error('Expected to ignore the callback.'); + }), + ); }).toWarnDev( 'State updates from the useState() and useReducer() Hooks ' + "don't support the second callback argument. " + @@ -302,7 +328,7 @@ describe('ReactHooks', () => { expect(root).toMatchRenderedOutput('0 (light)'); // Normal update - setCounter(1); + act(() => setCounter(1)); expect(root).toFlushAndYield([ 'Parent: 1 (light)', 'Child: 1 (light)', @@ -311,14 +337,17 @@ describe('ReactHooks', () => { expect(root).toMatchRenderedOutput('1 (light)'); // Update that doesn't change state, so it bails out - setCounter(1); + act(() => setCounter(1)); expect(root).toFlushAndYield(['Parent: 1 (light)']); expect(root).toMatchRenderedOutput('1 (light)'); // Update that doesn't change state, but the context changes, too, so it // can't bail out - setCounter(1); - setTheme('dark'); + act(() => { + setCounter(1); + setTheme('dark'); + }); + expect(root).toFlushAndYield([ 'Theme: dark', 'Parent: 1 (dark)', @@ -353,7 +382,7 @@ describe('ReactHooks', () => { expect(root).toMatchRenderedOutput('0'); // Normal update - setCounter(1); + act(() => setCounter(1)); expect(root).toFlushAndYield(['Parent: 1', 'Child: 1', 'Effect: 1']); expect(root).toMatchRenderedOutput('1'); @@ -361,18 +390,18 @@ describe('ReactHooks', () => { // because the alterate fiber has pending update priority, so we have to // enter the render phase before we can bail out. But we bail out before // rendering the child, and we don't fire any effects. - setCounter(1); + act(() => setCounter(1)); expect(root).toFlushAndYield(['Parent: 1']); expect(root).toMatchRenderedOutput('1'); // Update to the same state again. This times, neither fiber has pending // update priority, so we can bail out before even entering the render phase. - setCounter(1); + act(() => setCounter(1)); expect(root).toFlushAndYield([]); expect(root).toMatchRenderedOutput('1'); // This changes the state to something different so it renders normally. - setCounter(2); + act(() => setCounter(2)); expect(root).toFlushAndYield(['Parent: 2', 'Child: 2', 'Effect: 2']); expect(root).toMatchRenderedOutput('2'); }); @@ -406,12 +435,14 @@ describe('ReactHooks', () => { return value; }); }; - update(0); - update(0); - update(0); - update(1); - update(2); - update(3); + act(() => { + update(0); + update(0); + update(0); + update(1); + update(2); + update(3); + }); expect(ReactTestRenderer).toHaveYielded([ // The first four updates were eagerly computed, because the queue is @@ -467,7 +498,7 @@ describe('ReactHooks', () => { }; // Update at normal priority - update(n => n * 100); + act(() => update(n => n * 100)); // The new state is eagerly computed. expect(ReactTestRenderer).toHaveYielded(['Compute state (1 -> 100)']); @@ -795,9 +826,11 @@ describe('ReactHooks', () => { class Cls extends React.Component { render() { - _setState(() => { - ReactCurrentDispatcher.current.readContext(ThemeContext); - }); + act(() => + _setState(() => { + ReactCurrentDispatcher.current.readContext(ThemeContext); + }), + ); return null; } } @@ -1250,9 +1283,11 @@ describe('ReactHooks', () => { } function B() { - _setState(() => { - throw new Error('Hello'); - }); + act(() => + _setState(() => { + throw new Error('Hello'); + }), + ); return null; } diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index 2f74fdc9c885b..6b3439283cbdd 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -27,6 +27,7 @@ let useImperativeHandle; let forwardRef; let flushPassiveEffects; let memo; +let act; // These tests use React Noop Renderer. All new tests should use React Test // Renderer and go in ReactHooks-test; plan is gradually migrate the noop tests @@ -73,6 +74,7 @@ describe('ReactHooksWithNoopRenderer', () => { useImperativeHandle = React.useImperativeHandle; forwardRef = React.forwardRef; memo = React.memo; + act = ReactNoop.act; }); function span(prop) { @@ -99,8 +101,11 @@ describe('ReactHooksWithNoopRenderer', () => { expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); // Schedule some updates - counter.current.updateCount(1); - counter.current.updateCount(count => count + 10); + act(() => { + counter.current.updateCount(1); + counter.current.updateCount(count => count + 10); + }); + // Partially flush without committing ReactNoop.flushThrough(['Count: 11']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); @@ -180,11 +185,11 @@ describe('ReactHooksWithNoopRenderer', () => { ReactNoop.flush(); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); - counter.current.updateCount(1); + act(() => counter.current.updateCount(1)); ReactNoop.flush(); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); - counter.current.updateCount(count => count + 10); + act(() => counter.current.updateCount(count => count + 10)); ReactNoop.flush(); expect(ReactNoop.getChildren()).toEqual([span('Count: 11')]); }); @@ -204,7 +209,7 @@ describe('ReactHooksWithNoopRenderer', () => { expect(ReactNoop.flush()).toEqual(['getInitialState', 'Count: 42']); expect(ReactNoop.getChildren()).toEqual([span('Count: 42')]); - counter.current.updateCount(7); + act(() => counter.current.updateCount(7)); expect(ReactNoop.flush()).toEqual(['Count: 7']); expect(ReactNoop.getChildren()).toEqual([span('Count: 7')]); }); @@ -222,10 +227,10 @@ describe('ReactHooksWithNoopRenderer', () => { ReactNoop.flush(); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); - counter.current.updateCount(7); + act(() => counter.current.updateCount(7)); expect(ReactNoop.flush()).toEqual(['Count: 7']); - counter.current.updateLabel('Total'); + act(() => counter.current.updateLabel('Total')); expect(ReactNoop.flush()).toEqual(['Total: 7']); }); @@ -240,11 +245,11 @@ describe('ReactHooksWithNoopRenderer', () => { ReactNoop.flush(); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); - updaters[0](1); + act(() => updaters[0](1)); ReactNoop.flush(); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); - updaters[0](count => count + 10); + act(() => updaters[0](count => count + 10)); ReactNoop.flush(); expect(ReactNoop.getChildren()).toEqual([span('Count: 11')]); @@ -263,7 +268,7 @@ describe('ReactHooksWithNoopRenderer', () => { ReactNoop.flush(); ReactNoop.render(null); ReactNoop.flush(); - expect(() => _updateCount(1)).toWarnDev( + expect(() => act(() => _updateCount(1))).toWarnDev( "Warning: Can't perform a React state update on an unmounted " + 'component. This is a no-op, but it indicates a memory leak in your ' + 'application. To fix, cancel all subscriptions and asynchronous ' + @@ -289,7 +294,7 @@ describe('ReactHooksWithNoopRenderer', () => { expect(ReactNoop.flush()).toEqual([]); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); - _updateCount(1); + act(() => _updateCount(1)); expect(ReactNoop.flush()).toEqual(['Count: 1']); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); }); @@ -513,13 +518,15 @@ describe('ReactHooksWithNoopRenderer', () => { ReactNoop.flush(); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); - counter.current.dispatch(INCREMENT); + act(() => counter.current.dispatch(INCREMENT)); ReactNoop.flush(); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); + act(() => { + counter.current.dispatch(DECREMENT); + counter.current.dispatch(DECREMENT); + counter.current.dispatch(DECREMENT); + }); - counter.current.dispatch(DECREMENT); - counter.current.dispatch(DECREMENT); - counter.current.dispatch(DECREMENT); ReactNoop.flush(); expect(ReactNoop.getChildren()).toEqual([span('Count: -2')]); }); @@ -553,13 +560,16 @@ describe('ReactHooksWithNoopRenderer', () => { expect(ReactNoop.flush()).toEqual(['Init', 'Count: 10']); expect(ReactNoop.getChildren()).toEqual([span('Count: 10')]); - counter.current.dispatch(INCREMENT); + act(() => counter.current.dispatch(INCREMENT)); expect(ReactNoop.flush()).toEqual(['Count: 11']); expect(ReactNoop.getChildren()).toEqual([span('Count: 11')]); - counter.current.dispatch(DECREMENT); - counter.current.dispatch(DECREMENT); - counter.current.dispatch(DECREMENT); + act(() => { + counter.current.dispatch(DECREMENT); + counter.current.dispatch(DECREMENT); + counter.current.dispatch(DECREMENT); + }); + expect(ReactNoop.flush()).toEqual(['Count: 8']); expect(ReactNoop.getChildren()).toEqual([span('Count: 8')]); }); @@ -585,9 +595,12 @@ describe('ReactHooksWithNoopRenderer', () => { ReactNoop.flush(); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); - counter.current.dispatch(INCREMENT); - counter.current.dispatch(INCREMENT); - counter.current.dispatch(INCREMENT); + act(() => { + counter.current.dispatch(INCREMENT); + counter.current.dispatch(INCREMENT); + counter.current.dispatch(INCREMENT); + }); + ReactNoop.flushSync(() => { counter.current.dispatch(INCREMENT); }); @@ -670,8 +683,10 @@ describe('ReactHooksWithNoopRenderer', () => { }); return ; } + ReactNoop.render([, ]); - expect(ReactNoop.flush()).toEqual([ + + expect(act(ReactNoop.flush)).toEqual([ 'Passive', 'Layout', 'Layout effect 0', @@ -769,14 +784,14 @@ describe('ReactHooksWithNoopRenderer', () => { ReactNoop.render(); expect(ReactNoop.flush()).toEqual(['Count: (empty)']); expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); - flushPassiveEffects(); + act(flushPassiveEffects); expect(ReactNoop.clearYields()).toEqual(['Schedule update [0]']); expect(ReactNoop.flush()).toEqual(['Count: 0']); ReactNoop.render(); expect(ReactNoop.flush()).toEqual(['Count: 0']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); - flushPassiveEffects(); + act(flushPassiveEffects); expect(ReactNoop.clearYields()).toEqual(['Schedule update [1]']); expect(ReactNoop.flush()).toEqual(['Count: 1']); }); @@ -798,14 +813,14 @@ describe('ReactHooksWithNoopRenderer', () => { expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); // Rendering again should flush the previous commit's effects - ReactNoop.render(); + act(() => ReactNoop.render()); ReactNoop.flushThrough(['Schedule update [0]', 'Count: 0']); expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); expect(ReactNoop.flush()).toEqual([]); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); - flushPassiveEffects(); + act(flushPassiveEffects); expect(ReactNoop.flush()).toEqual(['Schedule update [1]', 'Count: 1']); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); }); @@ -828,7 +843,7 @@ describe('ReactHooksWithNoopRenderer', () => { // Enqueuing this update forces the passive effect to be flushed -- // updateCount(1) happens first, so 2 wins. - _updateCount(2); + act(() => _updateCount(2)); expect(ReactNoop.flush()).toEqual(['Will set count to 1', 'Count: 2']); expect(ReactNoop.getChildren()).toEqual([span('Count: 2')]); }); @@ -874,7 +889,7 @@ describe('ReactHooksWithNoopRenderer', () => { // Enqueuing this update forces the passive effect to be flushed -- // updateCount(1) happens first, so 2 wins. - _updateCount(2); + act(() => _updateCount(2)); expect(ReactNoop.flush()).toEqual(['Will set count to 1', 'Count: 2']); expect(ReactNoop.getChildren()).toEqual([span('Count: 2')]); @@ -908,7 +923,7 @@ describe('ReactHooksWithNoopRenderer', () => { expect(ReactNoop.flush()).toEqual(['Count: (empty)']); expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); // Now fire the effects - flushPassiveEffects(); + act(flushPassiveEffects); // There were multiple updates, but there should only be a // single render expect(ReactNoop.clearYields()).toEqual(['Count: 0']); @@ -1391,7 +1406,7 @@ describe('ReactHooksWithNoopRenderer', () => { span('Count: 0'), ]); - button.current.increment(); + act(button.current.increment); expect(ReactNoop.flush()).toEqual([ // Button should not re-render, because its props haven't changed // 'Increment', @@ -1415,7 +1430,7 @@ describe('ReactHooksWithNoopRenderer', () => { ]); // Callback should have updated - button.current.increment(); + act(button.current.increment); expect(ReactNoop.flush()).toEqual(['Count: 11']); expect(ReactNoop.getChildren()).toEqual([ span('Increment'), @@ -1623,8 +1638,11 @@ describe('ReactHooksWithNoopRenderer', () => { span('A: 0, B: 0, C: [not loaded]'), ]); - updateA(2); - updateB(3); + act(() => { + updateA(2); + updateB(3); + }); + expect(ReactNoop.flush()).toEqual(['A: 2, B: 3, C: [not loaded]']); expect(ReactNoop.getChildren()).toEqual([ span('A: 2, B: 3, C: [not loaded]'), @@ -1669,10 +1687,11 @@ describe('ReactHooksWithNoopRenderer', () => { ReactNoop.render(); expect(ReactNoop.flush()).toEqual(['A: 0, B: 0, C: 0']); expect(ReactNoop.getChildren()).toEqual([span('A: 0, B: 0, C: 0')]); - - updateA(2); - updateB(3); - updateC(4); + act(() => { + updateA(2); + updateB(3); + updateC(4); + }); expect(ReactNoop.flush()).toEqual(['A: 2, B: 3, C: 4']); expect(ReactNoop.getChildren()).toEqual([span('A: 2, B: 3, C: 4')]); ReactNoop.render(); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 73a55370a187c..69e4135cc70ed 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -3,6 +3,7 @@ let ReactTestRenderer; let ReactFeatureFlags; let ReactCache; let Suspense; +let act; // let JestReact; @@ -19,6 +20,7 @@ describe('ReactSuspense', () => { ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false; React = require('react'); ReactTestRenderer = require('react-test-renderer'); + act = ReactTestRenderer.act; // JestReact = require('jest-react'); ReactCache = require('react-cache'); @@ -797,7 +799,7 @@ describe('ReactSuspense', () => { ]); expect(root).toMatchRenderedOutput('Tab: 0 + sibling'); - setTab(1); + act(() => setTab(1)); expect(ReactTestRenderer).toHaveYielded([ 'Suspend! [Tab: 1]', ' + sibling', @@ -811,7 +813,7 @@ describe('ReactSuspense', () => { ]); expect(root).toMatchRenderedOutput('Tab: 1 + sibling'); - setTab(2); + act(() => setTab(2)); expect(ReactTestRenderer).toHaveYielded([ 'Suspend! [Tab: 2]', ' + sibling', @@ -864,7 +866,7 @@ describe('ReactSuspense', () => { ]); expect(root).toMatchRenderedOutput('A:0'); - setStep(1); + act(() => setStep(1)); expect(ReactTestRenderer).toHaveYielded(['Suspend! [A:1]', 'Loading...']); expect(root).toMatchRenderedOutput('Loading...'); diff --git a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js index fbbf56b6c5d48..b217c22f1c6bf 100644 --- a/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js +++ b/packages/react-reconciler/src/forks/ReactFiberHostConfig.custom.js @@ -121,6 +121,3 @@ export const didNotFindHydratableInstance = $$$hostConfig.didNotFindHydratableInstance; export const didNotFindHydratableTextInstance = $$$hostConfig.didNotFindHydratableTextInstance; - -export const shouldWarnForUnbatchedSetState = - $$$hostConfig.shouldWarnForUnbatchedSetState; diff --git a/packages/react-test-renderer/src/ReactTestHostConfig.js b/packages/react-test-renderer/src/ReactTestHostConfig.js index 8226739839070..b4e6f681fac3b 100644 --- a/packages/react-test-renderer/src/ReactTestHostConfig.js +++ b/packages/react-test-renderer/src/ReactTestHostConfig.js @@ -272,5 +272,3 @@ export function unhideTextInstance( ): void { textInstance.isHidden = false; } - -export const shouldWarnForUnbatchedSetState = false; diff --git a/packages/react-test-renderer/src/ReactTestRenderer.js b/packages/react-test-renderer/src/ReactTestRenderer.js index fda60e171f460..88024666f400e 100644 --- a/packages/react-test-renderer/src/ReactTestRenderer.js +++ b/packages/react-test-renderer/src/ReactTestRenderer.js @@ -17,8 +17,8 @@ import { updateContainer, flushSync, injectIntoDevTools, + batchedUpdates, } from 'react-reconciler/inline.test'; -import {batchedUpdates} from 'events/ReactGenericBatching'; import {findCurrentFiberUsingSlowPath} from 'react-reconciler/reflection'; import { Fragment, @@ -557,6 +557,10 @@ const ReactTestRendererFiber = { /* eslint-enable camelcase */ unstable_setNowImplementation: setNowImplementation, + + act(callback: () => X): X { + return batchedUpdates(callback); + }, }; const fiberToWrapper = new WeakMap(); From 6cba8f5592c6402cf66bc8b3433ba713b4e1b635 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Mon, 4 Feb 2019 16:53:51 +0000 Subject: [PATCH 20/31] pass all tests --- .../src/__tests__/ReactSuspenseFuzz-test.internal.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js index ed3d9322ba1c4..9444bdbf6f019 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseFuzz-test.internal.js @@ -158,7 +158,7 @@ describe('ReactSuspenseFuzz', () => { if ((elapsedTime += 1000) > 1000000) { throw new Error('Something did not resolve properly.'); } - jest.advanceTimersByTime(1000); + ReactTestRenderer.act(() => jest.advanceTimersByTime(1000)); root.unstable_flushAll(); } From f94608947c203819492cb6c6206f074766f8c327 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Mon, 4 Feb 2019 17:10:20 +0000 Subject: [PATCH 21/31] expose just the warning from the scheduler --- packages/react-reconciler/src/ReactFiberHooks.js | 9 +++------ packages/react-reconciler/src/ReactFiberScheduler.js | 6 +----- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index ab541be4ddfc9..6e13694de5372 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -30,7 +30,7 @@ import { } from './ReactHookEffectTags'; import { scheduleWork, - ensureBatchingAndScheduleWork, + warnIfNotCurrentlyBatchingInDev, computeExpirationForFiber, flushPassiveEffects, requestCurrentTime, @@ -1137,13 +1137,10 @@ function dispatchAction( } if (__DEV__) { if (shouldWarnForUnbatchedSetState === true) { - ensureBatchingAndScheduleWork(fiber, expirationTime); - } else { - scheduleWork(fiber, expirationTime); + warnIfNotCurrentlyBatchingInDev(); } - } else { - scheduleWork(fiber, expirationTime); } + scheduleWork(fiber, expirationTime); } } diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 27246cdc49a09..f50235b86a825 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -1790,10 +1790,7 @@ function scheduleWorkToRoot(fiber: Fiber, expirationTime): FiberRoot | null { return root; } -export function ensureBatchingAndScheduleWork( - fiber: Fiber, - expirationTime: ExpirationTime, -) { +export function warnIfNotCurrentlyBatchingInDev(): void { if (__DEV__) { if (isBatchingUpdates === false) { warningWithoutStack( @@ -1805,7 +1802,6 @@ export function ensureBatchingAndScheduleWork( ); } } - scheduleWork(fiber, expirationTime); } function scheduleWork(fiber: Fiber, expirationTime: ExpirationTime) { From 8617b648486960a9f99c2adece61ec9fbaad0fa2 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Tue, 5 Feb 2019 11:24:24 +0000 Subject: [PATCH 22/31] don't return values --- .../ReactDOMSuspensePlaceholder-test.js | 4 +++- .../src/__tests__/ReactTestUtils-test.js | 18 ++++++++++++------ .../react-dom/src/test-utils/ReactTestUtils.js | 14 +++++++++++--- .../src/ReactTestRenderer.js | 2 +- .../ReactTestRenderer-test.internal.js | 18 ++++++++++++++++++ 5 files changed, 45 insertions(+), 11 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.js b/packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.js index e867bcf04f98d..985827e53bfd7 100644 --- a/packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMSuspensePlaceholder-test.js @@ -146,7 +146,9 @@ describe('ReactDOMSuspensePlaceholder', () => { ); } - act(() => ReactDOM.render(, container)); + act(() => { + ReactDOM.render(, container); + }); expect(container.innerHTML).toEqual( 'SiblingLoading...', ); diff --git a/packages/react-dom/src/__tests__/ReactTestUtils-test.js b/packages/react-dom/src/__tests__/ReactTestUtils-test.js index cd2f658510a1e..81c6ac7d60506 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtils-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtils-test.js @@ -561,7 +561,7 @@ describe('ReactTestUtils', () => { const container = document.createElement('div'); document.body.appendChild(container); let calledCtr = 0; - act(() => + act(() => { ReactDOM.render( { @@ -569,8 +569,8 @@ describe('ReactTestUtils', () => { }} />, container, - ), - ); + ); + }); const button = document.getElementById('button'); function click() { button.dispatchEvent(new MouseEvent('click', {bubbles: true})); @@ -602,10 +602,14 @@ describe('ReactTestUtils', () => { const container = document.createElement('div'); document.body.appendChild(container); let button; - act(() => ReactDOM.render(, container)); + act(() => { + ReactDOM.render(, container); + }); button = document.getElementById('button'); expect(button.innerHTML).toBe('0'); - act(() => button.dispatchEvent(new MouseEvent('click', {bubbles: true}))); + act(() => { + button.dispatchEvent(new MouseEvent('click', {bubbles: true})); + }); expect(button.innerHTML).toBe('1'); document.body.removeChild(container); }); @@ -653,7 +657,9 @@ describe('ReactTestUtils', () => { const container = document.createElement('div'); act(() => { - act(() => ReactDOM.render(, container)); + act(() => { + ReactDOM.render(, container); + }); jest.advanceTimersByTime(250); }); diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js index cbd1a7a30a6ab..0157404781e85 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtils.js +++ b/packages/react-dom/src/test-utils/ReactTestUtils.js @@ -18,6 +18,7 @@ import { import SyntheticEvent from 'events/SyntheticEvent'; import invariant from 'shared/invariant'; import lowPriorityWarning from 'shared/lowPriorityWarning'; +import warningWithoutStack from 'shared/warningWithoutStack'; import {ELEMENT_NODE} from '../shared/HTMLNodeType'; import * as DOMTopLevelEventTypes from '../events/DOMTopLevelEventTypes'; @@ -384,10 +385,17 @@ const ReactTestUtils = { Simulate: null, SimulateNative: {}, - act(callback: () => X): X { - let result = ReactDOM.unstable_batchedUpdates(callback); + act(callback: () => void): void { + const result = ReactDOM.unstable_batchedUpdates(callback); + if (result !== undefined) { + warningWithoutStack(false, "Do not return a value from 'act' "); + } ReactDOM.render(
, actContainerElement); - return result; + return { + then() { + warningWithoutStack(false, "Do not return a value from 'act'"); + }, + }; }, }; diff --git a/packages/react-test-renderer/src/ReactTestRenderer.js b/packages/react-test-renderer/src/ReactTestRenderer.js index 88024666f400e..9918b674b37a3 100644 --- a/packages/react-test-renderer/src/ReactTestRenderer.js +++ b/packages/react-test-renderer/src/ReactTestRenderer.js @@ -559,7 +559,7 @@ const ReactTestRendererFiber = { unstable_setNowImplementation: setNowImplementation, act(callback: () => X): X { - return batchedUpdates(callback); + batchedUpdates(callback); }, }; diff --git a/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js index 8240fbe75a8a4..897f331c8f4fb 100644 --- a/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js +++ b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js @@ -1021,4 +1021,22 @@ describe('ReactTestRenderer', () => { ReactNoop.flush(); ReactTestRenderer.create(); }); + + describe.only('act', () => { + it('works', () => { + function App(props){ + React.useEffect(() => { + props.callback() + }) + return null + } + let called = false + let inst + ReactTestRenderer.act(() => { + inst = ReactTestRenderer.create( {called = true}}/>) + }) + + expect(called).toBe(true) + }) + }) }); From c126f1061571f8b8c9e4713024f9dc9c4fbe1311 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Tue, 5 Feb 2019 13:06:27 +0000 Subject: [PATCH 23/31] a bunch of changes. can't return values from .act don't try to await .act calls pass tests --- .../src/__tests__/ReactTestUtils-test.js | 14 +++++ .../src/test-utils/ReactTestUtils.js | 34 +++++++++-- .../src/createReactNoop.js | 41 ++++++++++++- .../src/__tests__/ReactHooks-test.internal.js | 46 +++++++++++---- ...eactHooksWithNoopRenderer-test.internal.js | 43 +++++++++----- .../src/ReactTestRenderer.js | 58 ++++++++++++++++++- .../ReactTestRenderer-test.internal.js | 31 +++++----- 7 files changed, 219 insertions(+), 48 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactTestUtils-test.js b/packages/react-dom/src/__tests__/ReactTestUtils-test.js index 81c6ac7d60506..ad071f19aa177 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtils-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtils-test.js @@ -665,4 +665,18 @@ describe('ReactTestUtils', () => { expect(container.innerHTML).toBe('1'); }); + + it('warns if you return a value inside act', () => { + expect(() => act(() => 123)).toWarnDev( + ['An .act(...) function must not return anything'], + {withoutStack: true}, + ); + }); + + it('warns if you try to await an .act call', () => { + expect(act(() => {}).then).toWarnDev( + ['Do not await an act(...) call, it is not a promise'], + {withoutStack: true}, + ); + }); }); diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js index 0157404781e85..4f5e53e8ce1e8 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtils.js +++ b/packages/react-dom/src/test-utils/ReactTestUtils.js @@ -22,6 +22,11 @@ import warningWithoutStack from 'shared/warningWithoutStack'; import {ELEMENT_NODE} from '../shared/HTMLNodeType'; import * as DOMTopLevelEventTypes from '../events/DOMTopLevelEventTypes'; +// for .act's return value +type Thenable = { + then(resolve: () => mixed, reject?: () => mixed): mixed, +}; + const {findDOMNode} = ReactDOM; // Keep in sync with ReactDOMUnstableNativeDependencies.js // and ReactDOM.js: @@ -385,15 +390,36 @@ const ReactTestUtils = { Simulate: null, SimulateNative: {}, - act(callback: () => void): void { + act(callback: () => void): Thenable { const result = ReactDOM.unstable_batchedUpdates(callback); - if (result !== undefined) { - warningWithoutStack(false, "Do not return a value from 'act' "); + if (__DEV__) { + if (result !== undefined) { + let addendum; + if (typeof result.then === 'function') { + addendum = + '\n\nIt looks like you wrote act(async () => ...) or returned a Promise. ' + + 'Do not write async logic inside act(...)\n'; + } else { + addendum = ' You returned: ' + result; + } + warningWithoutStack( + false, + 'An .act(...) function must not return anything.%s', + addendum, + ); + } } ReactDOM.render(
, actContainerElement); + // we want the user to not expect a return, + // but we want to warn if they use it like they can await on it. return { then() { - warningWithoutStack(false, "Do not return a value from 'act'"); + if (__DEV__) { + warningWithoutStack( + false, + 'Do not await an act(...) call, it is not a promise', + ); + } }, }; }, diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 0d9d326ec9f8f..0de8c98e757d0 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -21,6 +21,12 @@ import type {ReactNodeList} from 'shared/ReactTypes'; import {createPortal} from 'shared/ReactPortal'; import expect from 'expect'; import {REACT_FRAGMENT_TYPE, REACT_ELEMENT_TYPE} from 'shared/ReactSymbols'; +import warningWithoutStack from 'shared/warningWithoutStack'; + +// for .act's return value +type Thenable = { + then(resolve: () => mixed, reject?: () => mixed): mixed, +}; type Container = { rootID: string, @@ -865,9 +871,38 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { interactiveUpdates: NoopRenderer.interactiveUpdates, // maybe this should exist only in the test file - act(callback: void => X): X { - return NoopRenderer.batchedUpdates(callback); - // flush updates? + act(callback: () => void): Thenable { + let result = NoopRenderer.batchedUpdates(callback); + if (__DEV__) { + if (result !== undefined) { + let addendum; + if (typeof result.then === 'function') { + addendum = + '\n\nIt looks like you wrote act(async () => ...) or returned a Promise. ' + + 'Do not write async logic inside act(...)\n'; + } else { + addendum = ' You returned: ' + result; + } + warningWithoutStack( + false, + 'An .act(...) function must not return anything.%s', + addendum, + ); + } + } + ReactNoop.flushPassiveEffects(); + // we want the user to not expect a return, + // but we want to warn if they use it like they can await on it. + return { + then() { + if (__DEV__) { + warningWithoutStack( + false, + 'Do not await an act(...) call, it is not a promise', + ); + } + }, + }; }, flushSync(fn: () => mixed) { diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index 2a01b0291594d..ec5ca92aa060b 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -126,8 +126,10 @@ describe('ReactHooks', () => { expect(root).toFlushAndYield(['Parent: 1, 2']); // prepare to check SameValue - setCounter1(0 / -1); - setCounter2(NaN); + act(() => { + setCounter1(0 / -1); + setCounter2(NaN); + }); expect(root).toFlushAndYield([ 'Parent: 0, NaN', 'Child: 0, NaN', @@ -135,14 +137,19 @@ describe('ReactHooks', () => { ]); // check if re-setting to negative 0 / NaN still bails out - setCounter1(0 / -1); - setCounter2(NaN); - setCounter2(Infinity); - setCounter2(NaN); + act(() => { + setCounter1(0 / -1); + setCounter2(NaN); + setCounter2(Infinity); + setCounter2(NaN); + }); + expect(root).toFlushAndYield(['Parent: 0, NaN']); // check if changing negative 0 to positive 0 does not bail out - setCounter1(0); + act(() => { + setCounter1(0); + }); expect(root).toFlushAndYield([ 'Parent: 0, NaN', 'Child: 0, NaN', @@ -430,22 +437,31 @@ describe('ReactHooks', () => { expect(root).toMatchRenderedOutput('2'); // prepare to check SameValue - setCounter(0); + act(() => { + setCounter(0); + }); expect(root).toFlushAndYield(['Parent: 0', 'Child: 0', 'Effect: 0']); expect(root).toMatchRenderedOutput('0'); // Update to the same state for the first time to flush the queue - setCounter(0); + act(() => { + setCounter(0); + }); + expect(root).toFlushAndYield(['Parent: 0']); expect(root).toMatchRenderedOutput('0'); // Update again to the same state. Should bail out. - setCounter(0); + act(() => { + setCounter(0); + }); expect(root).toFlushAndYield([]); expect(root).toMatchRenderedOutput('0'); // Update to a different state (positive 0 to negative 0) - setCounter(0 / -1); + act(() => { + setCounter(0 / -1); + }); expect(root).toFlushAndYield(['Parent: 0', 'Child: 0', 'Effect: 0']); expect(root).toMatchRenderedOutput('0'); }); @@ -886,7 +902,13 @@ describe('ReactHooks', () => { , ), - ).toWarnDev('Context can only be read while React is rendering'); + ).toWarnDev( + [ + 'Context can only be read while React is rendering', + 'Render methods should be a pure function of props and state', + ], + {withoutStack: 1}, + ); }); it('warns when calling hooks inside useReducer', () => { diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index 23954b51e56be..bef77fae1e456 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -663,14 +663,17 @@ describe('ReactHooksWithNoopRenderer', () => { ReactNoop.render([, ]); - expect(act(ReactNoop.flush)).toEqual([ - 'Passive', - 'Layout', - 'Layout effect 0', - 'Passive effect', - 'Layout', - 'Layout effect 1', - ]); + act(() => { + expect(ReactNoop.flush()).toEqual([ + 'Passive', + 'Layout', + 'Layout effect 0', + 'Passive effect', + 'Layout', + 'Layout effect 1', + ]); + }); + expect(ReactNoop.getChildren()).toEqual([ span('Passive'), span('Layout'), @@ -761,14 +764,18 @@ describe('ReactHooksWithNoopRenderer', () => { ReactNoop.render(); expect(ReactNoop.flush()).toEqual(['Count: (empty)']); expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); - ReactNoop.flushPassiveEffects(); + act(() => { + ReactNoop.flushPassiveEffects(); + }); expect(ReactNoop.clearYields()).toEqual(['Schedule update [0]']); expect(ReactNoop.flush()).toEqual(['Count: 0']); ReactNoop.render(); expect(ReactNoop.flush()).toEqual(['Count: 0']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); - ReactNoop.flushPassiveEffects(); + act(() => { + ReactNoop.flushPassiveEffects(); + }); expect(ReactNoop.clearYields()).toEqual(['Schedule update [1]']); expect(ReactNoop.flush()).toEqual(['Count: 1']); }); @@ -790,14 +797,20 @@ describe('ReactHooksWithNoopRenderer', () => { expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); // Rendering again should flush the previous commit's effects - act(() => ReactNoop.render()); + expect(() => ReactNoop.render()).toWarnDev( + ['It looks like you are in a test environment'], + {withoutStack: true}, + ); ReactNoop.flushThrough(['Schedule update [0]', 'Count: 0']); expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); - expect(ReactNoop.flush()).toEqual([]); + ReactNoop.batchedUpdates(() => { + expect(ReactNoop.flush()).toEqual([]); + }); + expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); - ReactNoop.flushPassiveEffects(); + act(() => ReactNoop.flushPassiveEffects()); expect(ReactNoop.flush()).toEqual(['Schedule update [1]', 'Count: 1']); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); }); @@ -900,7 +913,9 @@ describe('ReactHooksWithNoopRenderer', () => { expect(ReactNoop.flush()).toEqual(['Count: (empty)']); expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); // Now fire the effects - ReactNoop.flushPassiveEffects(); + act(() => { + ReactNoop.flushPassiveEffects(); + }); // There were multiple updates, but there should only be a // single render expect(ReactNoop.clearYields()).toEqual(['Count: 0']); diff --git a/packages/react-test-renderer/src/ReactTestRenderer.js b/packages/react-test-renderer/src/ReactTestRenderer.js index 9918b674b37a3..4e3f1c7e8616d 100644 --- a/packages/react-test-renderer/src/ReactTestRenderer.js +++ b/packages/react-test-renderer/src/ReactTestRenderer.js @@ -39,6 +39,7 @@ import { } from 'shared/ReactWorkTags'; import invariant from 'shared/invariant'; import ReactVersion from 'shared/ReactVersion'; +import warningWithoutStack from 'shared/warningWithoutStack'; import {getPublicInstance} from './ReactTestHostConfig'; import { @@ -70,6 +71,11 @@ type FindOptions = $Shape<{ export type Predicate = (node: ReactTestInstance) => ?boolean; +// for .act's return value +type Thenable = { + then(resolve: () => mixed, reject?: () => mixed): mixed, +}; + const defaultTestOptions = { createNodeMock: function() { return null; @@ -558,11 +564,59 @@ const ReactTestRendererFiber = { unstable_setNowImplementation: setNowImplementation, - act(callback: () => X): X { - batchedUpdates(callback); + act(callback: () => void): Thenable { + let result = batchedUpdates(callback); + // warnings copied from ReactTestUtils + if (__DEV__) { + if (result !== undefined) { + let addendum; + if (typeof result.then === 'function') { + addendum = + '\n\nIt looks like you wrote act(async () => ...) or returned a Promise. ' + + 'Do not write async logic inside act(...)\n'; + } else { + addendum = ' You returned: ' + result; + } + warningWithoutStack( + false, + 'An .act(...) function must not return anything.%s', + addendum, + ); + } + } + flushPassiveEffects(); + // we want the user to not expect a return, + // but we want to warn if they use it like they can await on it. + return { + then() { + if (__DEV__) { + warningWithoutStack( + false, + 'Do not await an act(...) call, it is not a promise', + ); + } + }, + }; }, }; +// root used to flush effects during .act() calls +const actRoot = createContainer( + { + children: [], + createNodeMock: defaultTestOptions.createNodeMock, + tag: 'CONTAINER', + }, + true, + false, +); + +function flushPassiveEffects() { + // Trick to flush passive effects without exposing an internal API: + // Create a throwaway root and schedule a dummy update on it. + updateContainer(null, actRoot, null, null); +} + const fiberToWrapper = new WeakMap(); function wrapFiber(fiber: Fiber): ReactTestInstance { let wrapper = fiberToWrapper.get(fiber); diff --git a/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js index 897f331c8f4fb..0ca6fb54b50a9 100644 --- a/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js +++ b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js @@ -1022,21 +1022,26 @@ describe('ReactTestRenderer', () => { ReactTestRenderer.create(); }); - describe.only('act', () => { + describe('act', () => { it('works', () => { - function App(props){ + function App(props) { React.useEffect(() => { - props.callback() - }) - return null + props.callback(); + }); + return null; } - let called = false - let inst + let called = false; ReactTestRenderer.act(() => { - inst = ReactTestRenderer.create( {called = true}}/>) - }) - - expect(called).toBe(true) - }) - }) + ReactTestRenderer.create( + { + called = true; + }} + />, + ); + }); + + expect(called).toBe(true); + }); + }); }); From 790ce2ac6642df36f727e4fc86b1b9368e2abaf0 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Tue, 5 Feb 2019 15:23:44 +0000 Subject: [PATCH 24/31] fixes and nits --- .../ReactHooksInspectionIntegration-test.js | 12 ++++++++---- .../src/__tests__/ReactTestUtils-test.js | 10 +++++++--- .../src/test-utils/ReactTestUtils.js | 11 +++++++---- .../src/createReactNoop.js | 10 ++++++---- .../react-reconciler/src/ReactFiberHooks.js | 2 +- .../src/ReactFiberScheduler.js | 17 +++++++++++------ ...eactHooksWithNoopRenderer-test.internal.js | 19 +++++-------------- .../src/ReactTestRenderer.js | 10 +++++----- 8 files changed, 50 insertions(+), 41 deletions(-) diff --git a/packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js b/packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js index 91c4ee0ef77e3..3c7972aff4df8 100644 --- a/packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js +++ b/packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js @@ -93,8 +93,12 @@ describe('ReactHooksInspectionIntegration', () => { React.useMemo(() => state1 + state2, [state1]); function update() { - setState('A'); - dispatch({value: 'B'}); + act(() => { + setState('A'); + }); + act(() => { + dispatch({value: 'B'}); + }); ref.current = 'C'; } let memoizedUpdate = React.useCallback(update, []); @@ -122,7 +126,7 @@ describe('ReactHooksInspectionIntegration', () => { {name: 'Callback', value: updateStates, subHooks: []}, ]); - act(updateStates); + updateStates(); childFiber = renderer.root.findByType(Foo)._currentFiber(); tree = ReactDebugTools.inspectHooksOfFiber(childFiber); @@ -134,7 +138,7 @@ describe('ReactHooksInspectionIntegration', () => { {name: 'LayoutEffect', value: effect, subHooks: []}, {name: 'Effect', value: effect, subHooks: []}, {name: 'ImperativeHandle', value: outsideRef.current, subHooks: []}, - {name: 'Memo', value: 'AB', subHooks: []}, + {name: 'Memo', value: 'Ab', subHooks: []}, {name: 'Callback', value: updateStates, subHooks: []}, ]); }); diff --git a/packages/react-dom/src/__tests__/ReactTestUtils-test.js b/packages/react-dom/src/__tests__/ReactTestUtils-test.js index ad071f19aa177..6180bb9b2ae41 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtils-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtils-test.js @@ -636,7 +636,7 @@ describe('ReactTestUtils', () => { expect(button.innerHTML).toBe('2'); expect(() => setValueRef(1)).toWarnDev( [ - 'It looks like you are in a test environment, trying to set state outside of an act(...) call.', + 'An update to App inside a test was not wrapped in ReactTestUtils.act(...).', ], {withoutStack: 1}, ); @@ -668,14 +668,18 @@ describe('ReactTestUtils', () => { it('warns if you return a value inside act', () => { expect(() => act(() => 123)).toWarnDev( - ['An .act(...) function must not return anything'], + [ + 'The callback passed to ReactTestUtils.act(...) function must not return anything.', + ], {withoutStack: true}, ); }); it('warns if you try to await an .act call', () => { expect(act(() => {}).then).toWarnDev( - ['Do not await an act(...) call, it is not a promise'], + [ + 'Do not await the result of calling ReactTestUtils.act(...), it is not a Promise.', + ], {withoutStack: true}, ); }); diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js index 4f5e53e8ce1e8..5bf6755bbbd62 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtils.js +++ b/packages/react-dom/src/test-utils/ReactTestUtils.js @@ -391,20 +391,23 @@ const ReactTestUtils = { SimulateNative: {}, act(callback: () => void): Thenable { + // note: keep these warning messages in sync with + // createReactNoop.js and ReactTestRenderer.js const result = ReactDOM.unstable_batchedUpdates(callback); if (__DEV__) { if (result !== undefined) { let addendum; if (typeof result.then === 'function') { addendum = - '\n\nIt looks like you wrote act(async () => ...) or returned a Promise. ' + - 'Do not write async logic inside act(...)\n'; + '\n\nIt looks like you wrote ReactTestUtils.act(async () => ...), ' + + 'or returned a Promise from the callback passed to it. ' + + 'Putting asynchronous logic inside ReactTestUtils.act(...) is not supported.\n'; } else { addendum = ' You returned: ' + result; } warningWithoutStack( false, - 'An .act(...) function must not return anything.%s', + 'The callback passed to ReactTestUtils.act(...) function must not return anything.%s', addendum, ); } @@ -417,7 +420,7 @@ const ReactTestUtils = { if (__DEV__) { warningWithoutStack( false, - 'Do not await an act(...) call, it is not a promise', + 'Do not await the result of calling ReactTestUtils.act(...), it is not a Promise.', ); } }, diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 0de8c98e757d0..491ab5948ba0b 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -872,20 +872,22 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { // maybe this should exist only in the test file act(callback: () => void): Thenable { + // note: keep these warning messages in sync with + // ReactTestRenderer.js and ReactTestUtils.js let result = NoopRenderer.batchedUpdates(callback); if (__DEV__) { if (result !== undefined) { let addendum; if (typeof result.then === 'function') { addendum = - '\n\nIt looks like you wrote act(async () => ...) or returned a Promise. ' + - 'Do not write async logic inside act(...)\n'; + "\n\nIt looks like you wrote ReactNoop.act(async () => ...) or returned a Promise from it's callback. " + + 'Putting asynchronous logic inside ReactNoop.act(...) is not supported.\n'; } else { addendum = ' You returned: ' + result; } warningWithoutStack( false, - 'An .act(...) function must not return anything.%s', + 'The callback passed to ReactNoop.act(...) function must not return anything.%s', addendum, ); } @@ -898,7 +900,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { if (__DEV__) { warningWithoutStack( false, - 'Do not await an act(...) call, it is not a promise', + 'Do not await the result of calling ReactNoop.act(...), it is not a Promise.', ); } }, diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 994ae26c04c22..1443fb544cdd1 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -1137,7 +1137,7 @@ function dispatchAction( } if (__DEV__) { if (shouldWarnForUnbatchedSetState === true) { - warnIfNotCurrentlyBatchingInDev(); + warnIfNotCurrentlyBatchingInDev(fiber); } } scheduleWork(fiber, expirationTime); diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index d2a0ee7aba8a2..d11ec0252191a 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -1790,15 +1790,20 @@ function scheduleWorkToRoot(fiber: Fiber, expirationTime): FiberRoot | null { return root; } -export function warnIfNotCurrentlyBatchingInDev(): void { +export function warnIfNotCurrentlyBatchingInDev(fiber: Fiber): void { if (__DEV__) { - if (isBatchingUpdates === false) { + if (isRendering === false && isBatchingUpdates === false) { warningWithoutStack( false, - 'It looks like you are in a test environment, trying to ' + - 'set state outside of an act(...) call. ' + - 'This could lead to unexpected ui while testing. Use ' + - 'act(...) to batch your updates and remove this warning.', + 'An update to %s inside a test was not wrapped in ReactTestUtils.act(...).\n\n' + + 'When testing, code that causes React state updates should be wrapped into ReactTestUtils.act(...):\n\n' + + 'ReactTestUtils.act(() => {\n' + + ' /* fire events */\n' + + '});\n' + + '/* assert on the output */\n\n' + + "This ensures that you're testing the behavior the user would see in the browser." + + ' Learn more at https://fb.me/react-testutils-act', + getComponentName(fiber.type), ); } } diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js index bef77fae1e456..446dd1bf7e78d 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.internal.js @@ -764,18 +764,14 @@ describe('ReactHooksWithNoopRenderer', () => { ReactNoop.render(); expect(ReactNoop.flush()).toEqual(['Count: (empty)']); expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); - act(() => { - ReactNoop.flushPassiveEffects(); - }); + ReactNoop.flushPassiveEffects(); expect(ReactNoop.clearYields()).toEqual(['Schedule update [0]']); expect(ReactNoop.flush()).toEqual(['Count: 0']); ReactNoop.render(); expect(ReactNoop.flush()).toEqual(['Count: 0']); expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); - act(() => { - ReactNoop.flushPassiveEffects(); - }); + ReactNoop.flushPassiveEffects(); expect(ReactNoop.clearYields()).toEqual(['Schedule update [1]']); expect(ReactNoop.flush()).toEqual(['Count: 1']); }); @@ -797,10 +793,7 @@ describe('ReactHooksWithNoopRenderer', () => { expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); // Rendering again should flush the previous commit's effects - expect(() => ReactNoop.render()).toWarnDev( - ['It looks like you are in a test environment'], - {withoutStack: true}, - ); + ReactNoop.render(); ReactNoop.flushThrough(['Schedule update [0]', 'Count: 0']); expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); @@ -810,7 +803,7 @@ describe('ReactHooksWithNoopRenderer', () => { expect(ReactNoop.getChildren()).toEqual([span('Count: 0')]); - act(() => ReactNoop.flushPassiveEffects()); + ReactNoop.flushPassiveEffects(); expect(ReactNoop.flush()).toEqual(['Schedule update [1]', 'Count: 1']); expect(ReactNoop.getChildren()).toEqual([span('Count: 1')]); }); @@ -913,9 +906,7 @@ describe('ReactHooksWithNoopRenderer', () => { expect(ReactNoop.flush()).toEqual(['Count: (empty)']); expect(ReactNoop.getChildren()).toEqual([span('Count: (empty)')]); // Now fire the effects - act(() => { - ReactNoop.flushPassiveEffects(); - }); + ReactNoop.flushPassiveEffects(); // There were multiple updates, but there should only be a // single render expect(ReactNoop.clearYields()).toEqual(['Count: 0']); diff --git a/packages/react-test-renderer/src/ReactTestRenderer.js b/packages/react-test-renderer/src/ReactTestRenderer.js index 4e3f1c7e8616d..bca65cdd05f55 100644 --- a/packages/react-test-renderer/src/ReactTestRenderer.js +++ b/packages/react-test-renderer/src/ReactTestRenderer.js @@ -565,21 +565,21 @@ const ReactTestRendererFiber = { unstable_setNowImplementation: setNowImplementation, act(callback: () => void): Thenable { + // note: keep these warning messages in sync with let result = batchedUpdates(callback); - // warnings copied from ReactTestUtils if (__DEV__) { if (result !== undefined) { let addendum; if (typeof result.then === 'function') { addendum = - '\n\nIt looks like you wrote act(async () => ...) or returned a Promise. ' + - 'Do not write async logic inside act(...)\n'; + "\n\nIt looks like you wrote TestRenderer.act(async () => ...) or returned a Promise from it's callback. " + + 'Putting asynchronous logic inside TestRenderer.act(...) is not supported.\n'; } else { addendum = ' You returned: ' + result; } warningWithoutStack( false, - 'An .act(...) function must not return anything.%s', + 'The callback passed to TestRenderer.act(...) function must not return anything.%s', addendum, ); } @@ -592,7 +592,7 @@ const ReactTestRendererFiber = { if (__DEV__) { warningWithoutStack( false, - 'Do not await an act(...) call, it is not a promise', + 'Do not await the result of calling TestRenderer.act(...), it is not a Promise.', ); } }, From 32c7e1a7b8b173278f1cd53c182aa7368242bc63 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Tue, 5 Feb 2019 15:25:48 +0000 Subject: [PATCH 25/31] "fire events that udpates state" --- packages/react-reconciler/src/ReactFiberScheduler.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index d11ec0252191a..131fa9d147c5d 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -1798,7 +1798,7 @@ export function warnIfNotCurrentlyBatchingInDev(fiber: Fiber): void { 'An update to %s inside a test was not wrapped in ReactTestUtils.act(...).\n\n' + 'When testing, code that causes React state updates should be wrapped into ReactTestUtils.act(...):\n\n' + 'ReactTestUtils.act(() => {\n' + - ' /* fire events */\n' + + ' /* fire events that update state */\n' + '});\n' + '/* assert on the output */\n\n' + "This ensures that you're testing the behavior the user would see in the browser." + From 587331798ab22b2e270907dbd6137ae811d06b4d Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Tue, 5 Feb 2019 15:37:19 +0000 Subject: [PATCH 26/31] nit --- packages/react-reconciler/src/ReactFiberScheduler.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 131fa9d147c5d..5712dbc9cd6d1 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -1795,14 +1795,14 @@ export function warnIfNotCurrentlyBatchingInDev(fiber: Fiber): void { if (isRendering === false && isBatchingUpdates === false) { warningWithoutStack( false, - 'An update to %s inside a test was not wrapped in ReactTestUtils.act(...).\n\n' + - 'When testing, code that causes React state updates should be wrapped into ReactTestUtils.act(...):\n\n' + - 'ReactTestUtils.act(() => {\n' + + 'An update to %s inside a test was not wrapped in act(...).\n\n' + + 'When testing, code that causes React state updates should be wrapped into act(...):\n\n' + + 'act(() => {\n' + ' /* fire events that update state */\n' + '});\n' + '/* assert on the output */\n\n' + "This ensures that you're testing the behavior the user would see in the browser." + - ' Learn more at https://fb.me/react-testutils-act', + ' Learn more at https://fb.me/react-wrap-tests-with-act', getComponentName(fiber.type), ); } From 0a4b3cfe492af7c00e837f4eecff854ae36d493c Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Tue, 5 Feb 2019 15:50:11 +0000 Subject: [PATCH 27/31] =?UTF-8?q?=F0=9F=99=84?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/react-dom/src/__tests__/ReactTestUtils-test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react-dom/src/__tests__/ReactTestUtils-test.js b/packages/react-dom/src/__tests__/ReactTestUtils-test.js index 6180bb9b2ae41..6abeab3ae7417 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtils-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtils-test.js @@ -636,7 +636,7 @@ describe('ReactTestUtils', () => { expect(button.innerHTML).toBe('2'); expect(() => setValueRef(1)).toWarnDev( [ - 'An update to App inside a test was not wrapped in ReactTestUtils.act(...).', + 'An update to App inside a test was not wrapped in act(...).', ], {withoutStack: 1}, ); From b6689e18703f93d6e0203ded396027c90cf83088 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Tue, 5 Feb 2019 15:51:36 +0000 Subject: [PATCH 28/31] my bad --- packages/react-test-renderer/src/ReactTestRenderer.js | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/react-test-renderer/src/ReactTestRenderer.js b/packages/react-test-renderer/src/ReactTestRenderer.js index bca65cdd05f55..e3b911f95843a 100644 --- a/packages/react-test-renderer/src/ReactTestRenderer.js +++ b/packages/react-test-renderer/src/ReactTestRenderer.js @@ -566,6 +566,7 @@ const ReactTestRendererFiber = { act(callback: () => void): Thenable { // note: keep these warning messages in sync with + // createNoop.js and ReactTestUtils.js let result = batchedUpdates(callback); if (__DEV__) { if (result !== undefined) { From 26a6db54c8b0377f071c85ed72020275831e37b4 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Tue, 5 Feb 2019 15:58:50 +0000 Subject: [PATCH 29/31] hi andrew (prettier fix) --- packages/react-dom/src/__tests__/ReactTestUtils-test.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactTestUtils-test.js b/packages/react-dom/src/__tests__/ReactTestUtils-test.js index 6abeab3ae7417..49d64fe3ba2bb 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtils-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtils-test.js @@ -635,9 +635,7 @@ describe('ReactTestUtils', () => { }); expect(button.innerHTML).toBe('2'); expect(() => setValueRef(1)).toWarnDev( - [ - 'An update to App inside a test was not wrapped in act(...).', - ], + ['An update to App inside a test was not wrapped in act(...).'], {withoutStack: 1}, ); document.body.removeChild(container); From 25148d7b05629595db1c2f7c8c9c3095f8016484 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Tue, 5 Feb 2019 17:18:17 +0000 Subject: [PATCH 30/31] fix .act return value testing when result === null --- packages/react-dom/src/__tests__/ReactTestUtils-test.js | 6 ++++++ packages/react-dom/src/test-utils/ReactTestUtils.js | 2 +- packages/react-noop-renderer/src/createReactNoop.js | 2 +- packages/react-test-renderer/src/ReactTestRenderer.js | 2 +- 4 files changed, 9 insertions(+), 3 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactTestUtils-test.js b/packages/react-dom/src/__tests__/ReactTestUtils-test.js index 49d64fe3ba2bb..280908c77a0c5 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtils-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtils-test.js @@ -665,6 +665,12 @@ describe('ReactTestUtils', () => { }); it('warns if you return a value inside act', () => { + expect(() => act(() => null)).toWarnDev( + [ + 'The callback passed to ReactTestUtils.act(...) function must not return anything.', + ], + {withoutStack: true}, + ); expect(() => act(() => 123)).toWarnDev( [ 'The callback passed to ReactTestUtils.act(...) function must not return anything.', diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js index 5bf6755bbbd62..cf2493dae5195 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtils.js +++ b/packages/react-dom/src/test-utils/ReactTestUtils.js @@ -397,7 +397,7 @@ const ReactTestUtils = { if (__DEV__) { if (result !== undefined) { let addendum; - if (typeof result.then === 'function') { + if (result && typeof result.then === 'function') { addendum = '\n\nIt looks like you wrote ReactTestUtils.act(async () => ...), ' + 'or returned a Promise from the callback passed to it. ' + diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 491ab5948ba0b..d583509e9d1de 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -878,7 +878,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { if (__DEV__) { if (result !== undefined) { let addendum; - if (typeof result.then === 'function') { + if (result && typeof result.then === 'function') { addendum = "\n\nIt looks like you wrote ReactNoop.act(async () => ...) or returned a Promise from it's callback. " + 'Putting asynchronous logic inside ReactNoop.act(...) is not supported.\n'; diff --git a/packages/react-test-renderer/src/ReactTestRenderer.js b/packages/react-test-renderer/src/ReactTestRenderer.js index e3b911f95843a..a38bf2631ffda 100644 --- a/packages/react-test-renderer/src/ReactTestRenderer.js +++ b/packages/react-test-renderer/src/ReactTestRenderer.js @@ -571,7 +571,7 @@ const ReactTestRendererFiber = { if (__DEV__) { if (result !== undefined) { let addendum; - if (typeof result.then === 'function') { + if (result && typeof result.then === 'function') { addendum = "\n\nIt looks like you wrote TestRenderer.act(async () => ...) or returned a Promise from it's callback. " + 'Putting asynchronous logic inside TestRenderer.act(...) is not supported.\n'; From 06921000d8e788853daa6366a3fbb3e2dd99e11e Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Tue, 5 Feb 2019 17:29:54 +0000 Subject: [PATCH 31/31] nit --- packages/react-dom/src/test-utils/ReactTestUtils.js | 2 +- packages/react-noop-renderer/src/createReactNoop.js | 2 +- packages/react-test-renderer/src/ReactTestRenderer.js | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js index cf2493dae5195..dcb20d88cf9f7 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtils.js +++ b/packages/react-dom/src/test-utils/ReactTestUtils.js @@ -397,7 +397,7 @@ const ReactTestUtils = { if (__DEV__) { if (result !== undefined) { let addendum; - if (result && typeof result.then === 'function') { + if (result !== null && typeof result.then === 'function') { addendum = '\n\nIt looks like you wrote ReactTestUtils.act(async () => ...), ' + 'or returned a Promise from the callback passed to it. ' + diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index d583509e9d1de..b9832e6221fba 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -878,7 +878,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { if (__DEV__) { if (result !== undefined) { let addendum; - if (result && typeof result.then === 'function') { + if (result !== null && typeof result.then === 'function') { addendum = "\n\nIt looks like you wrote ReactNoop.act(async () => ...) or returned a Promise from it's callback. " + 'Putting asynchronous logic inside ReactNoop.act(...) is not supported.\n'; diff --git a/packages/react-test-renderer/src/ReactTestRenderer.js b/packages/react-test-renderer/src/ReactTestRenderer.js index a38bf2631ffda..39767e5bc77ed 100644 --- a/packages/react-test-renderer/src/ReactTestRenderer.js +++ b/packages/react-test-renderer/src/ReactTestRenderer.js @@ -571,7 +571,7 @@ const ReactTestRendererFiber = { if (__DEV__) { if (result !== undefined) { let addendum; - if (result && typeof result.then === 'function') { + if (result !== null && typeof result.then === 'function') { addendum = "\n\nIt looks like you wrote TestRenderer.act(async () => ...) or returned a Promise from it's callback. " + 'Putting asynchronous logic inside TestRenderer.act(...) is not supported.\n';