From 9791a6b669ee97877cc659b928a297864e1915af Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Tue, 12 Feb 2019 16:57:44 +0000 Subject: [PATCH 01/55] hacked up act(async () => {...}) --- .../src/__tests__/ReactTestUtils-test.js | 46 ++++++++-- packages/react-dom/src/client/ReactDOM.js | 2 + .../src/test-utils/ReactTestUtils.js | 88 +++++++++++++------ .../react-reconciler/src/ReactFiberHooks.js | 34 +++++-- .../src/ReactFiberReconciler.js | 2 + .../src/ReactFiberScheduler.js | 19 ---- 6 files changed, 133 insertions(+), 58 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactTestUtils-test.js b/packages/react-dom/src/__tests__/ReactTestUtils-test.js index 280908c77a0c..875af0bd7f8a 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtils-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtils-test.js @@ -649,7 +649,7 @@ describe('ReactTestUtils', () => { setToggle(1); }, 200); return () => clearTimeout(timeout); - }); + }, []); return toggle; } const container = document.createElement('div'); @@ -658,7 +658,7 @@ describe('ReactTestUtils', () => { act(() => { ReactDOM.render(, container); }); - jest.advanceTimersByTime(250); + jest.runAllTimers(); }); expect(container.innerHTML).toBe('1'); @@ -667,13 +667,13 @@ 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.', + 'The callback passed to ReactTestUtils.act(...) function must return undefined, or a Promise.', ], {withoutStack: true}, ); expect(() => act(() => 123)).toWarnDev( [ - 'The callback passed to ReactTestUtils.act(...) function must not return anything.', + 'The callback passed to ReactTestUtils.act(...) function must return undefined, or a Promise.', ], {withoutStack: true}, ); @@ -682,9 +682,45 @@ describe('ReactTestUtils', () => { it('warns if you try to await an .act call', () => { expect(act(() => {}).then).toWarnDev( [ - 'Do not await the result of calling ReactTestUtils.act(...), it is not a Promise.', + 'Do not await the result of calling ReactTestUtils.act(...) with sync logic, it is not a Promise.', ], {withoutStack: true}, ); }); + it('does the async stuff', async () => { + jest.useRealTimers(); + function App() { + let [ctr, setCtr] = React.useState(0); + function doSomething() { + setTimeout(() => { + console.log('callingsdad'); + setCtr(1); + }, 200); + } + + React.useEffect(() => { + doSomething(); + }, []); + return ctr; + } + const el = document.createElement('div'); + await act(async () => { + act(() => { + ReactDOM.render(, el); + }); + + await sleep(500); + expect(el.innerHTML).toBe('1') + + }); + jest.useFakeTimers(); + }); }); + +function sleep(period) { + return new Promise(resolve => { + setTimeout(() => { + resolve(true); + }, period); + }); +} diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index 0eac062bed74..a71550d07306 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, + setIsActingInDEV, } from 'react-reconciler/inline.dom'; import {createPortal as createPortalImpl} from 'shared/ReactPortal'; import {canUseDOM} from 'shared/ExecutionEnvironment'; @@ -822,6 +823,7 @@ const ReactDOM: Object = { restoreStateIfNeeded, dispatchEvent, runEventsInBatch, + setIsActingInDEV, ], }, }; diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js index 649598c759a1..cc3175dd13a2 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtils.js +++ b/packages/react-dom/src/test-utils/ReactTestUtils.js @@ -44,6 +44,7 @@ const [ restoreStateIfNeeded, dispatchEvent, runEventsInBatch, + setIsActingInDev, ] = ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Events; function Event(suffix) {} @@ -390,7 +391,7 @@ const ReactTestUtils = { Simulate: null, SimulateNative: {}, - act(callback: () => void): Thenable { + act(callback: () => void | Promise): Thenable { if (actContainerElement === null) { // warn if we can't actually create the stub element if (__DEV__) { @@ -406,44 +407,73 @@ const ReactTestUtils = { // then make it actContainerElement = document.createElement('div'); } + let prevCtr = actingCtr.current; + actingCtr.current++; + setIsActingInDev(true); const result = ReactDOM.unstable_batchedUpdates(callback); - // note: keep these warning messages in sync with - // createReactNoop.js and ReactTestRenderer.js - if (__DEV__) { - if (result !== undefined) { - let addendum; - 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. ' + - 'Putting asynchronous logic inside ReactTestUtils.act(...) is not supported.\n'; - } else { - addendum = ' You returned: ' + result; + if (result && typeof result.then === 'function') { + // the returned thenable MUST be called + let called = false; + setImmediate(() => { + if (!called) { + warningWithoutStack(null, 'you need to await your async acts'); } - warningWithoutStack( - false, - 'The callback passed to ReactTestUtils.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() { - if (__DEV__) { + }); + return { + then(fn) { + called = true; + result.then(() => { + ReactDOM.render(
, actContainerElement); + // await null? + if (actingCtr.current - 1 !== prevCtr) { + warningWithoutStack( + null, + 'you did not resolve a previous act call', + ); + } + actingCtr.current--; + + if (actingCtr.current === 0) { + setIsActingInDev(false); + } + fn(); + }); + }, + }; + } else { + if (__DEV__) { + if (result !== undefined) { warningWithoutStack( false, - 'Do not await the result of calling ReactTestUtils.act(...), it is not a Promise.', + 'The callback passed to ReactTestUtils.act(...) function ' + + 'must return undefined, or a Promise. You returned %s', + result, ); } - }, - }; + } + ReactDOM.render(
, actContainerElement); + actingCtr.current--; + + if (actingCtr.current === 0) { + setIsActingInDev(false); + } + return { + then() { + if (__DEV__) { + warningWithoutStack( + false, + 'Do not await the result of calling ReactTestUtils.act(...) with sync logic, it is not a Promise.', + ); + } + }, + }; + } }, }; +let actingCtr = {current: 0}; + /** * Exports: * diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 6eceed84f7e4..b97bd3a78709 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -30,7 +30,6 @@ import { } from './ReactHookEffectTags'; import { scheduleWork, - warnIfNotCurrentlyBatchingInDev, computeExpirationForFiber, flushPassiveEffects, requestCurrentTime, @@ -1004,16 +1003,41 @@ function updateMemo( return nextValue; } +let isActing = false; + +export function warnIfNotCurrentlyActingInDev(fiber: Fiber): void { + if (__DEV__) { + if (isActing === false) { + warning( + false, + '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-wrap-tests-with-act', + getComponentName(fiber.type), + ); + } + } +} + +export function setIsActingInDEV(toggle: boolean) { + isActing = toggle; +} + // in a test-like environment, we want to warn if dispatchAction() // is called outside of a batchedUpdates/TestUtils.act(...) call. -let shouldWarnForUnbatchedSetState = false; +let shouldWarnForUnactedSetState = false; if (__DEV__) { // jest isn't 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; + shouldWarnForUnactedSetState = true; } } @@ -1136,8 +1160,8 @@ function dispatchAction( } } if (__DEV__) { - if (shouldWarnForUnbatchedSetState === true) { - warnIfNotCurrentlyBatchingInDev(fiber); + if (shouldWarnForUnactedSetState === true) { + warnIfNotCurrentlyActingInDev(fiber); } } scheduleWork(fiber, expirationTime); diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index acb91ea3bb90..d1a219d2627b 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.js @@ -64,6 +64,7 @@ import { } from './ReactCurrentFiber'; import {StrictMode} from './ReactTypeOfMode'; import {Sync} from './ReactFiberExpirationTime'; +import {setIsActingInDEV} from './ReactFiberHooks'; type OpaqueRoot = FiberRoot; @@ -310,6 +311,7 @@ export { flushInteractiveUpdates, flushControlled, flushSync, + setIsActingInDEV, }; export function getPublicRootInstance( diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index de4fa2b0a053..fb0a89c09d63 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -1803,25 +1803,6 @@ function scheduleWorkToRoot(fiber: Fiber, expirationTime): FiberRoot | null { return root; } -export function warnIfNotCurrentlyBatchingInDev(fiber: Fiber): void { - if (__DEV__) { - if (isRendering === false && isBatchingUpdates === false) { - warningWithoutStack( - false, - '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-wrap-tests-with-act', - getComponentName(fiber.type), - ); - } - } -} - function scheduleWork(fiber: Fiber, expirationTime: ExpirationTime) { const root = scheduleWorkToRoot(fiber, expirationTime); if (root === null) { From 49876c3156b1d267f7c8876a44f4322b23db2207 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Thu, 14 Feb 2019 15:20:00 +0000 Subject: [PATCH 02/55] move stuff around --- .../src/__tests__/ReactTestUtils-test.js | 208 ------------- .../src/__tests__/ReactTestUtilsAct-test.js | 294 ++++++++++++++++++ packages/react-dom/src/client/ReactDOM.js | 4 +- .../src/test-utils/ReactTestUtils.js | 96 +----- .../src/test-utils/ReactTestUtilsAct.js | 129 ++++++++ .../react-reconciler/src/ReactFiberHooks.js | 19 +- .../src/ReactFiberReconciler.js | 4 +- 7 files changed, 442 insertions(+), 312 deletions(-) create mode 100644 packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js create mode 100644 packages/react-dom/src/test-utils/ReactTestUtilsAct.js diff --git a/packages/react-dom/src/__tests__/ReactTestUtils-test.js b/packages/react-dom/src/__tests__/ReactTestUtils-test.js index 875af0bd7f8a..687dbd1aaec2 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtils-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtils-test.js @@ -14,7 +14,6 @@ let React; let ReactDOM; let ReactDOMServer; let ReactTestUtils; -let act; function getTestDocument(markup) { const doc = document.implementation.createHTMLDocument(''); @@ -34,7 +33,6 @@ 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', () => { @@ -517,210 +515,4 @@ describe('ReactTestUtils', () => { ReactTestUtils.renderIntoDocument(); expect(mockArgs.length).toEqual(0); }); - - it('can use act 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; - act(() => { - ReactDOM.render( - { - called = true; - }} - />, - container, - ); - }); - - expect(called).toBe(true); - } finally { - document.body.removeChild(container); - } - }); - - 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); - return ( - - ); - } - const container = document.createElement('div'); - document.body.appendChild(container); - let button; - act(() => { - ReactDOM.render(, container); - }); - button = document.getElementById('button'); - expect(button.innerHTML).toBe('0'); - 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(...)', () => { - 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( - ['An update to App inside a test was not wrapped in 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.runAllTimers(); - }); - - expect(container.innerHTML).toBe('1'); - }); - - it('warns if you return a value inside act', () => { - expect(() => act(() => null)).toWarnDev( - [ - 'The callback passed to ReactTestUtils.act(...) function must return undefined, or a Promise.', - ], - {withoutStack: true}, - ); - expect(() => act(() => 123)).toWarnDev( - [ - 'The callback passed to ReactTestUtils.act(...) function must return undefined, or a Promise.', - ], - {withoutStack: true}, - ); - }); - - it('warns if you try to await an .act call', () => { - expect(act(() => {}).then).toWarnDev( - [ - 'Do not await the result of calling ReactTestUtils.act(...) with sync logic, it is not a Promise.', - ], - {withoutStack: true}, - ); - }); - it('does the async stuff', async () => { - jest.useRealTimers(); - function App() { - let [ctr, setCtr] = React.useState(0); - function doSomething() { - setTimeout(() => { - console.log('callingsdad'); - setCtr(1); - }, 200); - } - - React.useEffect(() => { - doSomething(); - }, []); - return ctr; - } - const el = document.createElement('div'); - await act(async () => { - act(() => { - ReactDOM.render(, el); - }); - - await sleep(500); - expect(el.innerHTML).toBe('1') - - }); - jest.useFakeTimers(); - }); }); - -function sleep(period) { - return new Promise(resolve => { - setTimeout(() => { - resolve(true); - }, period); - }); -} diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js new file mode 100644 index 000000000000..2fcf0c6b6f4b --- /dev/null +++ b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js @@ -0,0 +1,294 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @emails react-core + */ + +let React; +let ReactDOM; +let ReactTestUtils; +let act; + +jest.useRealTimers(); + +function sleep(period) { + return new Promise(resolve => { + setTimeout(() => { + resolve(true); + }, period); + }); +} + +describe('act', () => { + beforeEach(() => { + jest.resetModules(); + React = require('react'); + ReactDOM = require('react-dom'); + ReactTestUtils = require('react-dom/test-utils'); + act = ReactTestUtils.act; + }); + + describe('sync', () => { + it('can use act 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; + act(() => { + ReactDOM.render( + { + called = true; + }} + />, + container, + ); + }); + + expect(called).toBe(true); + } finally { + document.body.removeChild(container); + } + }); + + 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); + return ( + + ); + } + const container = document.createElement('div'); + document.body.appendChild(container); + let button; + act(() => { + ReactDOM.render(, container); + }); + button = document.getElementById('button'); + expect(button.innerHTML).toBe('0'); + 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(...)', () => { + 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( + ['An update to App inside a test was not wrapped in act(...).'], + {withoutStack: 1}, + ); + document.body.removeChild(container); + }); + describe('fake timers', () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + afterEach(() => { + jest.useRealTimers(); + }); + 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.runAllTimers(); + }); + + expect(container.innerHTML).toBe('1'); + }); + }); + + it('warns if you return a value inside act', () => { + expect(() => act(() => null)).toWarnDev( + [ + 'The callback passed to ReactTestUtils.act(...) function must return undefined, or a Promise.', + ], + {withoutStack: true}, + ); + expect(() => act(() => 123)).toWarnDev( + [ + 'The callback passed to ReactTestUtils.act(...) function must return undefined, or a Promise.', + ], + {withoutStack: true}, + ); + }); + + it('warns if you try to await an .act call', () => { + expect(act(() => {}).then).toWarnDev( + [ + 'Do not await the result of calling ReactTestUtils.act(...) with sync logic, it is not a Promise.', + ], + {withoutStack: true}, + ); + }); + }); + describe('async', () => { + it('does the async stuff', async () => { + function App() { + let [ctr, setCtr] = React.useState(0); + function doSomething() { + setTimeout(() => { + setCtr(1); + }, 200); + } + + React.useEffect(() => { + doSomething(); + }, []); + return ctr; + } + const el = document.createElement('div'); + await act(async () => { + act(() => { + ReactDOM.render(, el); + }); + + await sleep(500); + expect(el.innerHTML).toBe('1'); + }); + }); + + it('warns if you do not await an act call', async () => { + spyOnDevAndProd(console, 'error'); + act(async () => {}); + // it's annoying that we have to wait a tick before this warning comes in + await sleep(0); + expect(console.error).toHaveBeenCalledTimes(1); + }); + + it('it warns if you try to interleave multiple act calls', async () => { + spyOnDevAndProd(console, 'error'); + (async () => { + await act(async () => { + await sleep(200); + }); + })(); + + await act(async () => { + await sleep(500); + }); + + await sleep(1000); + expect(console.error).toHaveBeenCalledTimes(1); + }); + + it('commits are effects are guaranteed to be flushed', async () => { + function App(props) { + let [state, setState] = React.useState(0); + async function something() { + await null; + setState(1); + } + React.useEffect(() => { + something(); + }, []); + React.useEffect(() => { + props.callback(); + }); + return state; + } + let ctr = 0; + const div = document.createElement('div'); + + await act(async () => { + act(() => { + ReactDOM.render( ctr++} />, div); + }); + expect(div.innerHTML).toBe('0'); + expect(ctr).toBe(1); + }); + + expect(div.innerHTML).toBe('1'); + expect(ctr).toBe(2); + }); + }); +}); + +// todo - errors are caught as expected diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index a71550d07306..21a60ce13b68 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -35,7 +35,7 @@ import { getPublicRootInstance, findHostInstance, findHostInstanceWithWarning, - setIsActingInDEV, + setIsActingUpdatesInDev, } from 'react-reconciler/inline.dom'; import {createPortal as createPortalImpl} from 'shared/ReactPortal'; import {canUseDOM} from 'shared/ExecutionEnvironment'; @@ -823,7 +823,7 @@ const ReactDOM: Object = { restoreStateIfNeeded, dispatchEvent, runEventsInBatch, - setIsActingInDEV, + setIsActingUpdatesInDev, ], }, }; diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js index cc3175dd13a2..8fc29df317d6 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtils.js +++ b/packages/react-dom/src/test-utils/ReactTestUtils.js @@ -18,14 +18,9 @@ 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'; - -// for .act's return value -type Thenable = { - then(resolve: () => mixed, reject?: () => mixed): mixed, -}; +import act from './ReactTestUtilsAct'; const {findDOMNode} = ReactDOM; // Keep in sync with ReactDOMUnstableNativeDependencies.js @@ -44,7 +39,8 @@ const [ restoreStateIfNeeded, dispatchEvent, runEventsInBatch, - setIsActingInDev, + // eslint-disable-next-line no-unused-vars + setIsActingUpdatesInDev, ] = ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Events; function Event(suffix) {} @@ -152,9 +148,6 @@ function validateClassInstance(inst, methodName) { ); } -// a stub element, lazily initialized, used by act() when flushing effects -let actContainerElement = null; - /** * Utilities for making it easy to test React components. * @@ -390,90 +383,9 @@ const ReactTestUtils = { Simulate: null, SimulateNative: {}, - - act(callback: () => void | Promise): Thenable { - if (actContainerElement === null) { - // warn if we can't actually create the stub element - if (__DEV__) { - warningWithoutStack( - typeof document !== 'undefined' && - document !== null && - typeof document.createElement === 'function', - 'It looks like you called TestUtils.act(...) in a non-browser environment. ' + - "If you're using TestRenderer for your tests, you should call " + - 'TestRenderer.act(...) instead of TestUtils.act(...).', - ); - } - // then make it - actContainerElement = document.createElement('div'); - } - let prevCtr = actingCtr.current; - actingCtr.current++; - setIsActingInDev(true); - - const result = ReactDOM.unstable_batchedUpdates(callback); - if (result && typeof result.then === 'function') { - // the returned thenable MUST be called - let called = false; - setImmediate(() => { - if (!called) { - warningWithoutStack(null, 'you need to await your async acts'); - } - }); - return { - then(fn) { - called = true; - result.then(() => { - ReactDOM.render(
, actContainerElement); - // await null? - if (actingCtr.current - 1 !== prevCtr) { - warningWithoutStack( - null, - 'you did not resolve a previous act call', - ); - } - actingCtr.current--; - - if (actingCtr.current === 0) { - setIsActingInDev(false); - } - fn(); - }); - }, - }; - } else { - if (__DEV__) { - if (result !== undefined) { - warningWithoutStack( - false, - 'The callback passed to ReactTestUtils.act(...) function ' + - 'must return undefined, or a Promise. You returned %s', - result, - ); - } - } - ReactDOM.render(
, actContainerElement); - actingCtr.current--; - - if (actingCtr.current === 0) { - setIsActingInDev(false); - } - return { - then() { - if (__DEV__) { - warningWithoutStack( - false, - 'Do not await the result of calling ReactTestUtils.act(...) with sync logic, it is not a Promise.', - ); - } - }, - }; - } - }, + act, }; -let actingCtr = {current: 0}; - /** * Exports: * diff --git a/packages/react-dom/src/test-utils/ReactTestUtilsAct.js b/packages/react-dom/src/test-utils/ReactTestUtilsAct.js new file mode 100644 index 000000000000..ffe4f30a7cb8 --- /dev/null +++ b/packages/react-dom/src/test-utils/ReactTestUtilsAct.js @@ -0,0 +1,129 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + */ + +import React from 'react'; +import ReactDOM from 'react-dom'; +import warningWithoutStack from 'shared/warningWithoutStack'; + +const [ + /* eslint-disable no-unused-vars */ + _getInstanceFromNode, + _getNodeFromInstance, + _getFiberCurrentPropsFromNode, + _injectEventPluginsByName, + _eventNameDispatchConfigs, + _accumulateTwoPhaseDispatches, + _accumulateDirectDispatches, + _enqueueStateRestore, + _restoreStateIfNeeded, + _dispatchEvent, + _runEventsInBatch, + /* eslint-enable no-unused-vars */ + setIsActingUpdatesInDev, +] = ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Events; + +// for .act's return value +type Thenable = { + then(resolve: () => void, reject?: () => void): void, + // ^ todo - maybe this should return a promise... +}; + +// a stub element, lazily initialized, used by act() when flushing effects +let actContainerElement = null; +// a step counter when we descend/ascend from .act calls +const actingScopeDepth = {current: 0}; + +export default function act(callback: () => void | Promise): Thenable { + if (actContainerElement === null) { + // warn if we can't actually create the stub element + if (__DEV__) { + warningWithoutStack( + typeof document !== 'undefined' && + document !== null && + typeof document.createElement === 'function', + 'It looks like you called TestUtils.act(...) in a non-browser environment. ' + + "If you're using TestRenderer for your tests, you should call " + + 'TestRenderer.act(...) instead of TestUtils.act(...).', + ); + } + // then make it + actContainerElement = document.createElement('div'); + } + + const previousActingScopeDepth = actingScopeDepth.current; + actingScopeDepth.current++; + if (previousActingScopeDepth === 0) { + setIsActingUpdatesInDev(true); + } + + const result = ReactDOM.unstable_batchedUpdates(callback); + if (result && typeof result.then === 'function') { + // the returned thenable MUST be called + let called = false; + setImmediate(() => { + if (!called) { + warningWithoutStack( + null, + 'You called .act() without awaiting its result. ' + + 'This could lead to unexpected testing behaviour, interleaving multiple act ' + + 'calls and mixing their scopes. You should - await act(async () => ...);', + // todo - a better warning here. open to suggestions. + ); + } + }); + return { + then(fn, errorFn) { + called = true; + result.then(() => { + ReactDOM.render(
, actContainerElement); + if (actingScopeDepth.current - 1 > previousActingScopeDepth) { + // if it's _less than_ previousActingScopeDepth, then we can assume the 'other' one has warned + warningWithoutStack( + null, + 'You seem to have interleaved multiple act() calls, this is not supported. ' + + 'Be sure to await previous sibling act calls before making a new one. ', + // todo - a better warning here. open to suggestions. + ); + } + actingScopeDepth.current--; + if (actingScopeDepth.current === 0) { + setIsActingUpdatesInDev(false); + } + fn(); + }, errorFn); + }, + }; + } else { + if (__DEV__) { + if (result !== undefined) { + warningWithoutStack( + false, + 'The callback passed to ReactTestUtils.act(...) function ' + + 'must return undefined, or a Promise. You returned %s', + result, + ); + } + } + ReactDOM.render(
, actContainerElement); + actingScopeDepth.current--; + + if (actingScopeDepth.current === 0) { + setIsActingUpdatesInDev(false); + } + return { + then() { + if (__DEV__) { + warningWithoutStack( + false, + // todo - well... why not? maybe this would be fine. + 'Do not await the result of calling ReactTestUtils.act(...) with sync logic, it is not a Promise.', + ); + } + }, + }; + } +} diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index b97bd3a78709..e686e060ea92 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -1003,11 +1003,20 @@ function updateMemo( return nextValue; } -let isActing = false; +// in a test-like environment, we want to warn if dispatchAction() +// is called outside of a TestUtils.act(...) call. + +let isActingUpdates: boolean = false; + +export function setIsActingUpdatesInDev(toggle: boolean) { + if (__DEV__) { + isActingUpdates = toggle; + } +} export function warnIfNotCurrentlyActingInDev(fiber: Fiber): void { if (__DEV__) { - if (isActing === false) { + if (isActingUpdates === false) { warning( false, 'An update to %s inside a test was not wrapped in act(...).\n\n' + @@ -1024,12 +1033,6 @@ export function warnIfNotCurrentlyActingInDev(fiber: Fiber): void { } } -export function setIsActingInDEV(toggle: boolean) { - isActing = toggle; -} - -// in a test-like environment, we want to warn if dispatchAction() -// is called outside of a batchedUpdates/TestUtils.act(...) call. let shouldWarnForUnactedSetState = false; if (__DEV__) { diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index d1a219d2627b..1d9b36a89b78 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.js @@ -64,7 +64,7 @@ import { } from './ReactCurrentFiber'; import {StrictMode} from './ReactTypeOfMode'; import {Sync} from './ReactFiberExpirationTime'; -import {setIsActingInDEV} from './ReactFiberHooks'; +import {setIsActingUpdatesInDev} from './ReactFiberHooks'; type OpaqueRoot = FiberRoot; @@ -311,7 +311,7 @@ export { flushInteractiveUpdates, flushControlled, flushSync, - setIsActingInDEV, + setIsActingUpdatesInDev, }; export function getPublicRootInstance( From 5cf26ba168de46de417a78bbe61e4e9ccdb5af04 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Thu, 14 Feb 2019 18:29:48 +0000 Subject: [PATCH 03/55] merge changes --- packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js | 7 +++---- packages/react-reconciler/src/ReactFiberHooks.js | 5 ++++- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js index 2fcf0c6b6f4b..acca4c52d46c 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js @@ -148,10 +148,9 @@ describe('act', () => { button.dispatchEvent(new MouseEvent('click', {bubbles: true})); }); expect(button.innerHTML).toBe('2'); - expect(() => setValueRef(1)).toWarnDev( - ['An update to App inside a test was not wrapped in act(...).'], - {withoutStack: 1}, - ); + expect(() => setValueRef(1)).toWarnDev([ + 'An update to App inside a test was not wrapped in act(...).', + ]); document.body.removeChild(container); }); describe('fake timers', () => { diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index ea223af3a4a0..35cd0e1f3c0e 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -34,6 +34,7 @@ import { flushPassiveEffects, requestCurrentTime, } from './ReactFiberScheduler'; +import {getStackByFiberInDevAndProd} from './ReactCurrentFiber'; import invariant from 'shared/invariant'; import warning from 'shared/warning'; @@ -1015,8 +1016,10 @@ export function warnIfNotCurrentlyActingInDev(fiber: Fiber): void { '});\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-wrap-tests-with-act', + ' Learn more at https://fb.me/react-wrap-tests-with-act' + + '%s', getComponentName(fiber.type), + getStackByFiberInDevAndProd(fiber), ); } } From d2ea31e047b7a81f112dbdc6abf180eb0c1016c5 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Fri, 15 Feb 2019 11:45:36 +0000 Subject: [PATCH 04/55] abstract .act warnings and stuff. all renderers. pass all tests. --- .../src/__tests__/ReactTestUtilsAct-test.js | 2 +- .../src/test-utils/ReactTestUtilsAct.js | 95 ++++------------ .../src/createReactNoop.js | 51 ++------- .../react-reconciler/src/ReactFiberHooks.js | 34 +----- .../src/ReactFiberReconciler.js | 2 +- .../src/ReactFiberScheduler.js | 36 ++++++ .../src/ReactTestRenderer.js | 49 ++------- .../ReactTestRenderer-test.internal.js | 2 +- packages/shared/createAct.js | 104 ++++++++++++++++++ 9 files changed, 180 insertions(+), 195 deletions(-) create mode 100644 packages/shared/createAct.js diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js index acca4c52d46c..0f853dcc75e7 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js @@ -258,7 +258,7 @@ describe('act', () => { expect(console.error).toHaveBeenCalledTimes(1); }); - it('commits are effects are guaranteed to be flushed', async () => { + it('commits and effects are guaranteed to be flushed', async () => { function App(props) { let [state, setState] = React.useState(0); async function something() { diff --git a/packages/react-dom/src/test-utils/ReactTestUtilsAct.js b/packages/react-dom/src/test-utils/ReactTestUtilsAct.js index ffe4f30a7cb8..47fd967c1dc0 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtilsAct.js +++ b/packages/react-dom/src/test-utils/ReactTestUtilsAct.js @@ -3,11 +3,14 @@ * * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. + * + * @flow */ import React from 'react'; import ReactDOM from 'react-dom'; import warningWithoutStack from 'shared/warningWithoutStack'; +import createAct from 'shared/createAct'; const [ /* eslint-disable no-unused-vars */ @@ -34,8 +37,15 @@ type Thenable = { // a stub element, lazily initialized, used by act() when flushing effects let actContainerElement = null; -// a step counter when we descend/ascend from .act calls -const actingScopeDepth = {current: 0}; + +const createdAct = createAct( + 'ReactTestUtils', + setIsActingUpdatesInDev, + () => { + ReactDOM.render(
, actContainerElement); + }, + ReactDOM.unstable_batchedUpdates, +); export default function act(callback: () => void | Promise): Thenable { if (actContainerElement === null) { @@ -45,85 +55,18 @@ export default function act(callback: () => void | Promise): Thenable { typeof document !== 'undefined' && document !== null && typeof document.createElement === 'function', - 'It looks like you called TestUtils.act(...) in a non-browser environment. ' + + 'It looks like you called ReactTestUtils.act(...) in a non-browser environment. ' + "If you're using TestRenderer for your tests, you should call " + - 'TestRenderer.act(...) instead of TestUtils.act(...).', + 'ReactTestRenderer.act(...) instead of TestUtils.act(...).', ); } // then make it actContainerElement = document.createElement('div'); } - const previousActingScopeDepth = actingScopeDepth.current; - actingScopeDepth.current++; - if (previousActingScopeDepth === 0) { - setIsActingUpdatesInDev(true); - } - - const result = ReactDOM.unstable_batchedUpdates(callback); - if (result && typeof result.then === 'function') { - // the returned thenable MUST be called - let called = false; - setImmediate(() => { - if (!called) { - warningWithoutStack( - null, - 'You called .act() without awaiting its result. ' + - 'This could lead to unexpected testing behaviour, interleaving multiple act ' + - 'calls and mixing their scopes. You should - await act(async () => ...);', - // todo - a better warning here. open to suggestions. - ); - } - }); - return { - then(fn, errorFn) { - called = true; - result.then(() => { - ReactDOM.render(
, actContainerElement); - if (actingScopeDepth.current - 1 > previousActingScopeDepth) { - // if it's _less than_ previousActingScopeDepth, then we can assume the 'other' one has warned - warningWithoutStack( - null, - 'You seem to have interleaved multiple act() calls, this is not supported. ' + - 'Be sure to await previous sibling act calls before making a new one. ', - // todo - a better warning here. open to suggestions. - ); - } - actingScopeDepth.current--; - if (actingScopeDepth.current === 0) { - setIsActingUpdatesInDev(false); - } - fn(); - }, errorFn); - }, - }; - } else { - if (__DEV__) { - if (result !== undefined) { - warningWithoutStack( - false, - 'The callback passed to ReactTestUtils.act(...) function ' + - 'must return undefined, or a Promise. You returned %s', - result, - ); - } - } - ReactDOM.render(
, actContainerElement); - actingScopeDepth.current--; - - if (actingScopeDepth.current === 0) { - setIsActingUpdatesInDev(false); - } - return { - then() { - if (__DEV__) { - warningWithoutStack( - false, - // todo - well... why not? maybe this would be fine. - 'Do not await the result of calling ReactTestUtils.act(...) with sync logic, it is not a Promise.', - ); - } - }, - }; - } + return createdAct(callback); } + +// setIsActingUpdatesInDev +// flushUpdates +// batchUpdates diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index b9832e6221fb..52db0a48a2b3 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -21,12 +21,7 @@ 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, -}; +import createAct from 'shared/createAct'; type Container = { rootID: string, @@ -870,42 +865,14 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { interactiveUpdates: NoopRenderer.interactiveUpdates, - // 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 (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'; - } else { - addendum = ' You returned: ' + result; - } - warningWithoutStack( - false, - 'The callback passed to ReactNoop.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 the result of calling ReactNoop.act(...), it is not a Promise.', - ); - } - }, - }; - }, + act: createAct( + 'ReactNoopRenderer', + NoopRenderer.setIsActingUpdatesInDev, + () => { + ReactNoop.flushPassiveEffects(); + }, + NoopRenderer.batchedUpdates, + ), flushSync(fn: () => mixed) { yieldedValues = []; diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 35cd0e1f3c0e..49c306e2afce 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -33,8 +33,8 @@ import { computeExpirationForFiber, flushPassiveEffects, requestCurrentTime, + warnIfNotCurrentlyActingInDev, } from './ReactFiberScheduler'; -import {getStackByFiberInDevAndProd} from './ReactCurrentFiber'; import invariant from 'shared/invariant'; import warning from 'shared/warning'; @@ -993,38 +993,6 @@ function updateMemo( return nextValue; } -// in a test-like environment, we want to warn if dispatchAction() -// is called outside of a TestUtils.act(...) call. - -let isActingUpdates: boolean = false; - -export function setIsActingUpdatesInDev(toggle: boolean) { - if (__DEV__) { - isActingUpdates = toggle; - } -} - -export function warnIfNotCurrentlyActingInDev(fiber: Fiber): void { - if (__DEV__) { - if (isActingUpdates === false) { - warning( - false, - '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-wrap-tests-with-act' + - '%s', - getComponentName(fiber.type), - getStackByFiberInDevAndProd(fiber), - ); - } - } -} - let shouldWarnForUnactedSetState = false; if (__DEV__) { diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index 1d9b36a89b78..c745b98d494a 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.js @@ -54,6 +54,7 @@ import { interactiveUpdates, flushInteractiveUpdates, flushPassiveEffects, + setIsActingUpdatesInDev, } from './ReactFiberScheduler'; import {createUpdate, enqueueUpdate} from './ReactUpdateQueue'; import ReactFiberInstrumentation from './ReactFiberInstrumentation'; @@ -64,7 +65,6 @@ import { } from './ReactCurrentFiber'; import {StrictMode} from './ReactTypeOfMode'; import {Sync} from './ReactFiberExpirationTime'; -import {setIsActingUpdatesInDev} from './ReactFiberHooks'; type OpaqueRoot = FiberRoot; diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 7b2c4780f723..817d0ce8c80d 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -1827,6 +1827,42 @@ function scheduleWorkToRoot(fiber: Fiber, expirationTime): FiberRoot | null { return root; } +// in a test-like environment, we want to warn if dispatchAction() +// is called outside of a TestUtils.act(...) call. + +let isActingUpdates: boolean = false; + +export function setIsActingUpdatesInDev(toggle: boolean) { + if (__DEV__) { + isActingUpdates = toggle; + } +} + +export function warnIfNotCurrentlyActingInDev(fiber: Fiber): void { + if (__DEV__) { + if ( + isActingUpdates === false && + isRendering === false && + isBatchingUpdates === false + ) { + warningWithoutStack( + false, + '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-wrap-tests-with-act' + + '%s', + getComponentName(fiber.type), + getStackByFiberInDevAndProd(fiber), + ); + } + } +} + function scheduleWork(fiber: Fiber, expirationTime: ExpirationTime) { const root = scheduleWorkToRoot(fiber, expirationTime); if (root === null) { diff --git a/packages/react-test-renderer/src/ReactTestRenderer.js b/packages/react-test-renderer/src/ReactTestRenderer.js index 39767e5bc77e..b5f0e06f8b35 100644 --- a/packages/react-test-renderer/src/ReactTestRenderer.js +++ b/packages/react-test-renderer/src/ReactTestRenderer.js @@ -18,6 +18,7 @@ import { flushSync, injectIntoDevTools, batchedUpdates, + setIsActingUpdatesInDev, } from 'react-reconciler/inline.test'; import {findCurrentFiberUsingSlowPath} from 'react-reconciler/reflection'; import { @@ -39,7 +40,7 @@ import { } from 'shared/ReactWorkTags'; import invariant from 'shared/invariant'; import ReactVersion from 'shared/ReactVersion'; -import warningWithoutStack from 'shared/warningWithoutStack'; +import createAct from 'shared/createAct'; import {getPublicInstance} from './ReactTestHostConfig'; import { @@ -71,11 +72,6 @@ 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; @@ -564,41 +560,12 @@ const ReactTestRendererFiber = { unstable_setNowImplementation: setNowImplementation, - 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) { - let addendum; - 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'; - } else { - addendum = ' You returned: ' + result; - } - warningWithoutStack( - false, - 'The callback passed to TestRenderer.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 the result of calling TestRenderer.act(...), it is not a Promise.', - ); - } - }, - }; - }, + act: createAct( + 'ReactTestRenderer', + setIsActingUpdatesInDev, + flushPassiveEffects, + batchedUpdates, + ), }; // root used to flush effects during .act() calls 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 1ce786c27b8c..5f82b3782059 100644 --- a/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js +++ b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js @@ -1052,7 +1052,7 @@ describe('ReactTestRenderer', () => { expect(() => act(() => {})).toThrow('document is not defined'); }).toWarnDev( [ - 'It looks like you called TestUtils.act(...) in a non-browser environment', + 'It looks like you called ReactTestUtils.act(...) in a non-browser environment', ], {withoutStack: 1}, ); diff --git a/packages/shared/createAct.js b/packages/shared/createAct.js new file mode 100644 index 000000000000..145c69df3d91 --- /dev/null +++ b/packages/shared/createAct.js @@ -0,0 +1,104 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +import warningWithoutStack from 'shared/warningWithoutStack'; + +// for .act's return value +type Thenable = { + then(resolve: () => void, reject?: () => void): void, + // ^ todo - maybe this should return a promise... +}; + +// a step counter when we descend/ascend from .act calls +let actingScopeDepth = 0; + +export default function createAct( + name: string, + setIsActingUpdatesInDev: boolean => void, + flushEffects: () => void, + batchedUpdates: *, // todo +) { + return function act(callback: () => void | Promise): Thenable { + const previousActingScopeDepth = actingScopeDepth; + actingScopeDepth++; + if (previousActingScopeDepth === 0) { + setIsActingUpdatesInDev(true); + } + + const result = batchedUpdates(callback); + if (result && typeof result.then === 'function') { + // the returned thenable MUST be called + let called = false; + setImmediate(() => { + if (!called) { + warningWithoutStack( + null, + 'You called %s.act() without awaiting its result. ' + + 'This could lead to unexpected testing behaviour, interleaving multiple act ' + + 'calls and mixing their scopes. You should - await act(async () => ...);', + name, + // todo - a better warning here. open to suggestions. + ); + } + }); + return { + then(fn, errorFn) { + called = true; + result.then(() => { + flushEffects(); + if (actingScopeDepth - 1 > previousActingScopeDepth) { + // if it's _less than_ previousActingScopeDepth, then we can assume the 'other' one has warned + warningWithoutStack( + null, + 'You seem to have interleaved multiple act() calls, this is not supported. ' + + 'Be sure to await previous sibling act calls before making a new one. ', + // todo - a better warning here. open to suggestions. + ); + } + actingScopeDepth--; + if (actingScopeDepth === 0) { + setIsActingUpdatesInDev(false); + } + fn(); + }, errorFn); + }, + }; + } else { + if (__DEV__) { + if (result !== undefined) { + warningWithoutStack( + false, + 'The callback passed to %s.act(...) function ' + + 'must return undefined, or a Promise. You returned %s', + name, + result, + ); + } + } + flushEffects(); + actingScopeDepth--; + + if (actingScopeDepth === 0) { + setIsActingUpdatesInDev(false); + } + return { + then() { + if (__DEV__) { + warningWithoutStack( + false, + // todo - well... why not? maybe this would be fine. + 'Do not await the result of calling %s.act(...) with sync logic, it is not a Promise.', + name, + ); + } + }, + }; + } + }; +} From 3c4142bc39211b94fe61b70fade997497f9ec1aa Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Fri, 15 Feb 2019 11:58:06 +0000 Subject: [PATCH 05/55] move testutils.act back into testutils --- .../src/test-utils/ReactTestUtils.js | 42 ++++++++++- .../src/test-utils/ReactTestUtilsAct.js | 72 ------------------- 2 files changed, 40 insertions(+), 74 deletions(-) delete mode 100644 packages/react-dom/src/test-utils/ReactTestUtilsAct.js diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js index 01e12d2ebee3..c9e96bed77a6 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtils.js +++ b/packages/react-dom/src/test-utils/ReactTestUtils.js @@ -18,9 +18,10 @@ 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 createAct from 'shared/createAct'; import * as DOMTopLevelEventTypes from '../events/DOMTopLevelEventTypes'; -import act from './ReactTestUtilsAct'; const {findDOMNode} = ReactDOM; // Keep in sync with ReactDOMUnstableNativeDependencies.js @@ -39,7 +40,6 @@ const [ restoreStateIfNeeded, dispatchEvent, runEventsInBatch, - // eslint-disable-next-line no-unused-vars setIsActingUpdatesInDev, ] = ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Events; @@ -148,6 +148,44 @@ function validateClassInstance(inst, methodName) { ); } +// for .act's return value +type Thenable = { + then(resolve: () => void, reject?: () => void): void, + // ^ todo - maybe this should return a promise... +}; + +// a stub element, lazily initialized, used by act() when flushing effects +let actContainerElement = null; + +const createdAct = createAct( + 'ReactTestUtils', + setIsActingUpdatesInDev, + () => { + ReactDOM.render(
, actContainerElement); + }, + ReactDOM.unstable_batchedUpdates, +); + +function act(callback: () => void | Promise): Thenable { + if (actContainerElement === null) { + // warn if we can't actually create the stub element + if (__DEV__) { + warningWithoutStack( + typeof document !== 'undefined' && + document !== null && + typeof document.createElement === 'function', + 'It looks like you called ReactTestUtils.act(...) in a non-browser environment. ' + + "If you're using TestRenderer for your tests, you should call " + + 'ReactTestRenderer.act(...) instead of TestUtils.act(...).', + ); + } + // then make it + actContainerElement = document.createElement('div'); + } + + return createdAct(callback); +} + /** * Utilities for making it easy to test React components. * diff --git a/packages/react-dom/src/test-utils/ReactTestUtilsAct.js b/packages/react-dom/src/test-utils/ReactTestUtilsAct.js deleted file mode 100644 index 47fd967c1dc0..000000000000 --- a/packages/react-dom/src/test-utils/ReactTestUtilsAct.js +++ /dev/null @@ -1,72 +0,0 @@ -/** - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - * - * @flow - */ - -import React from 'react'; -import ReactDOM from 'react-dom'; -import warningWithoutStack from 'shared/warningWithoutStack'; -import createAct from 'shared/createAct'; - -const [ - /* eslint-disable no-unused-vars */ - _getInstanceFromNode, - _getNodeFromInstance, - _getFiberCurrentPropsFromNode, - _injectEventPluginsByName, - _eventNameDispatchConfigs, - _accumulateTwoPhaseDispatches, - _accumulateDirectDispatches, - _enqueueStateRestore, - _restoreStateIfNeeded, - _dispatchEvent, - _runEventsInBatch, - /* eslint-enable no-unused-vars */ - setIsActingUpdatesInDev, -] = ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Events; - -// for .act's return value -type Thenable = { - then(resolve: () => void, reject?: () => void): void, - // ^ todo - maybe this should return a promise... -}; - -// a stub element, lazily initialized, used by act() when flushing effects -let actContainerElement = null; - -const createdAct = createAct( - 'ReactTestUtils', - setIsActingUpdatesInDev, - () => { - ReactDOM.render(
, actContainerElement); - }, - ReactDOM.unstable_batchedUpdates, -); - -export default function act(callback: () => void | Promise): Thenable { - if (actContainerElement === null) { - // warn if we can't actually create the stub element - if (__DEV__) { - warningWithoutStack( - typeof document !== 'undefined' && - document !== null && - typeof document.createElement === 'function', - 'It looks like you called ReactTestUtils.act(...) in a non-browser environment. ' + - "If you're using TestRenderer for your tests, you should call " + - 'ReactTestRenderer.act(...) instead of TestUtils.act(...).', - ); - } - // then make it - actContainerElement = document.createElement('div'); - } - - return createdAct(callback); -} - -// setIsActingUpdatesInDev -// flushUpdates -// batchUpdates From 331d9ebc4e2f8f718d80fa2408fcd902b6c4cab0 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Fri, 15 Feb 2019 15:01:37 +0000 Subject: [PATCH 06/55] move into scheduler, rename some bits --- .../src/__tests__/ReactTestUtilsAct-test.js | 6 +- packages/react-dom/src/client/ReactDOM.js | 4 +- .../src/test-utils/ReactTestUtils.js | 14 +-- .../src/createReactNoop.js | 10 +- .../react-reconciler/src/ReactFiberHooks.js | 4 +- .../src/ReactFiberReconciler.js | 4 +- .../src/ReactFiberScheduler.js | 87 +++++++++++++-- .../src/ReactTestRenderer.js | 27 +---- .../src/__tests__/ReactTestRenderer-test.js | 17 +++ packages/shared/createAct.js | 104 ------------------ 10 files changed, 110 insertions(+), 167 deletions(-) delete mode 100644 packages/shared/createAct.js diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js index 0f853dcc75e7..ec7b7f55c086 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js @@ -187,13 +187,13 @@ describe('act', () => { it('warns if you return a value inside act', () => { expect(() => act(() => null)).toWarnDev( [ - 'The callback passed to ReactTestUtils.act(...) function must return undefined, or a Promise.', + 'The callback passed to act(...) function must return undefined, or a Promise.', ], {withoutStack: true}, ); expect(() => act(() => 123)).toWarnDev( [ - 'The callback passed to ReactTestUtils.act(...) function must return undefined, or a Promise.', + 'The callback passed to act(...) function must return undefined, or a Promise.', ], {withoutStack: true}, ); @@ -202,7 +202,7 @@ describe('act', () => { it('warns if you try to await an .act call', () => { expect(act(() => {}).then).toWarnDev( [ - 'Do not await the result of calling ReactTestUtils.act(...) with sync logic, it is not a Promise.', + 'Do not await the result of calling act(...) with sync logic, it is not a Promise.', ], {withoutStack: true}, ); diff --git a/packages/react-dom/src/client/ReactDOM.js b/packages/react-dom/src/client/ReactDOM.js index 21a60ce13b68..d2e064041f68 100644 --- a/packages/react-dom/src/client/ReactDOM.js +++ b/packages/react-dom/src/client/ReactDOM.js @@ -35,7 +35,7 @@ import { getPublicRootInstance, findHostInstance, findHostInstanceWithWarning, - setIsActingUpdatesInDev, + actedUpdates, } from 'react-reconciler/inline.dom'; import {createPortal as createPortalImpl} from 'shared/ReactPortal'; import {canUseDOM} from 'shared/ExecutionEnvironment'; @@ -823,7 +823,7 @@ const ReactDOM: Object = { restoreStateIfNeeded, dispatchEvent, runEventsInBatch, - setIsActingUpdatesInDev, + actedUpdates, ], }, }; diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js index c9e96bed77a6..6063ea9b0e12 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtils.js +++ b/packages/react-dom/src/test-utils/ReactTestUtils.js @@ -20,7 +20,6 @@ import invariant from 'shared/invariant'; import lowPriorityWarning from 'shared/lowPriorityWarning'; import warningWithoutStack from 'shared/warningWithoutStack'; import {ELEMENT_NODE} from '../shared/HTMLNodeType'; -import createAct from 'shared/createAct'; import * as DOMTopLevelEventTypes from '../events/DOMTopLevelEventTypes'; const {findDOMNode} = ReactDOM; @@ -40,7 +39,7 @@ const [ restoreStateIfNeeded, dispatchEvent, runEventsInBatch, - setIsActingUpdatesInDev, + actedUpdates, ] = ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Events; function Event(suffix) {} @@ -157,15 +156,6 @@ type Thenable = { // a stub element, lazily initialized, used by act() when flushing effects let actContainerElement = null; -const createdAct = createAct( - 'ReactTestUtils', - setIsActingUpdatesInDev, - () => { - ReactDOM.render(
, actContainerElement); - }, - ReactDOM.unstable_batchedUpdates, -); - function act(callback: () => void | Promise): Thenable { if (actContainerElement === null) { // warn if we can't actually create the stub element @@ -183,7 +173,7 @@ function act(callback: () => void | Promise): Thenable { actContainerElement = document.createElement('div'); } - return createdAct(callback); + return actedUpdates(callback); } /** diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index 52db0a48a2b3..a4c9856a96bb 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -21,7 +21,6 @@ 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 createAct from 'shared/createAct'; type Container = { rootID: string, @@ -865,14 +864,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { interactiveUpdates: NoopRenderer.interactiveUpdates, - act: createAct( - 'ReactNoopRenderer', - NoopRenderer.setIsActingUpdatesInDev, - () => { - ReactNoop.flushPassiveEffects(); - }, - NoopRenderer.batchedUpdates, - ), + act: NoopRenderer.actedUpdates, flushSync(fn: () => mixed) { yieldedValues = []; diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index 49c306e2afce..dcbf1488d841 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -33,7 +33,7 @@ import { computeExpirationForFiber, flushPassiveEffects, requestCurrentTime, - warnIfNotCurrentlyActingInDev, + warnIfNotCurrentlyActingUpdatesInDev, } from './ReactFiberScheduler'; import invariant from 'shared/invariant'; @@ -1124,7 +1124,7 @@ function dispatchAction( } if (__DEV__) { if (shouldWarnForUnactedSetState === true) { - warnIfNotCurrentlyActingInDev(fiber); + warnIfNotCurrentlyActingUpdatesInDev(fiber); } } scheduleWork(fiber, expirationTime); diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index c745b98d494a..fa6d95b0a825 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.js @@ -54,7 +54,7 @@ import { interactiveUpdates, flushInteractiveUpdates, flushPassiveEffects, - setIsActingUpdatesInDev, + actedUpdates, } from './ReactFiberScheduler'; import {createUpdate, enqueueUpdate} from './ReactUpdateQueue'; import ReactFiberInstrumentation from './ReactFiberInstrumentation'; @@ -311,7 +311,7 @@ export { flushInteractiveUpdates, flushControlled, flushSync, - setIsActingUpdatesInDev, + actedUpdates, }; export function getPublicRootInstance( diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 817d0ce8c80d..ab86ae0376c6 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -1827,21 +1827,92 @@ function scheduleWorkToRoot(fiber: Fiber, expirationTime): FiberRoot | null { return root; } -// in a test-like environment, we want to warn if dispatchAction() -// is called outside of a TestUtils.act(...) call. +// in a test-like environment, we want to warn if dispatchAction() is +// called outside of a TestUtils.act(...)/batchedUpdates/render call. +// so we have a a step counter for when we descend/ascend from +// actedUpdates() calls, and test on it for when to warn +let actingUpdatesScopeDepth = 0; + +export function actedUpdates(callback: () => void | Promise): Thenable { + let previousActingUpdatesScopeDepth; + if (__DEV__) { + previousActingUpdatesScopeDepth = actingUpdatesScopeDepth; + actingUpdatesScopeDepth++; + } -let isActingUpdates: boolean = false; + const result = batchedUpdates(callback); + if (result && typeof result.then === 'function') { + // the returned thenable MUST be called + let called = false; + setTimeout(() => { + if (__DEV__) { + if (!called) { + warningWithoutStack( + null, + 'You called act() without awaiting its result. ' + + 'This could lead to unexpected testing behaviour, interleaving multiple act ' + + 'calls and mixing their scopes. You should - await act(async () => ...);', + // todo - a better warning here. open to suggestions. + ); + } + } + }, 0); + return { + then(fn, errorFn) { + called = true; + result.then(() => { + flushPassiveEffects(); + if (__DEV__) { + if (actingUpdatesScopeDepth - 1 > previousActingUpdatesScopeDepth) { + // if it's _less than_ previousActingUpdatesScopeDepth, then we can assume the 'other' one has warned + warningWithoutStack( + null, + 'You seem to have interleaved multiple act() calls, this is not supported. ' + + 'Be sure to await previous sibling act calls before making a new one. ', + // todo - a better warning here. open to suggestions. + ); + } + actingUpdatesScopeDepth--; + } -export function setIsActingUpdatesInDev(toggle: boolean) { - if (__DEV__) { - isActingUpdates = toggle; + fn(); + }, errorFn); + }, + }; + } else { + if (__DEV__) { + if (result !== undefined) { + warningWithoutStack( + false, + 'The callback passed to act(...) function ' + + 'must return undefined, or a Promise. You returned %s', + result, + ); + } + } + flushPassiveEffects(); + if (__DEV__) { + actingUpdatesScopeDepth--; + } + + return { + then() { + if (__DEV__) { + warningWithoutStack( + false, + // todo - well... why not? maybe this would be fine. + 'Do not await the result of calling act(...) with sync logic, it is not a Promise.', + ); + } + }, + }; } } -export function warnIfNotCurrentlyActingInDev(fiber: Fiber): void { +export function warnIfNotCurrentlyActingUpdatesInDev(fiber: Fiber): void { if (__DEV__) { if ( - isActingUpdates === false && + actingUpdatesScopeDepth === 0 && isRendering === false && isBatchingUpdates === false ) { diff --git a/packages/react-test-renderer/src/ReactTestRenderer.js b/packages/react-test-renderer/src/ReactTestRenderer.js index b5f0e06f8b35..3f30cb20f32d 100644 --- a/packages/react-test-renderer/src/ReactTestRenderer.js +++ b/packages/react-test-renderer/src/ReactTestRenderer.js @@ -18,7 +18,7 @@ import { flushSync, injectIntoDevTools, batchedUpdates, - setIsActingUpdatesInDev, + actedUpdates, } from 'react-reconciler/inline.test'; import {findCurrentFiberUsingSlowPath} from 'react-reconciler/reflection'; import { @@ -40,7 +40,6 @@ import { } from 'shared/ReactWorkTags'; import invariant from 'shared/invariant'; import ReactVersion from 'shared/ReactVersion'; -import createAct from 'shared/createAct'; import {getPublicInstance} from './ReactTestHostConfig'; import { @@ -560,31 +559,9 @@ const ReactTestRendererFiber = { unstable_setNowImplementation: setNowImplementation, - act: createAct( - 'ReactTestRenderer', - setIsActingUpdatesInDev, - flushPassiveEffects, - batchedUpdates, - ), + act: actedUpdates, }; -// 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.js b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js index 177ac9e11d89..29eb6d00d409 100644 --- a/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js +++ b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js @@ -29,6 +29,23 @@ describe('ReactTestRenderer', () => { }); }); + describe('act', () => { + it('should work', () => { + function App() { + let [ctr, setCtr] = React.useState(0); + React.useEffect(() => { + setCtr(1); + }); + return ctr; + } + let root; + ReactTestRenderer.act(() => { + root = ReactTestRenderer.create(); + }); + expect(root.toJSON()).toEqual('1'); + }); + }); + describe('timed out Suspense hidden subtrees should not be observable via toJSON', () => { let AsyncText; let PendingResources; diff --git a/packages/shared/createAct.js b/packages/shared/createAct.js deleted file mode 100644 index 145c69df3d91..000000000000 --- a/packages/shared/createAct.js +++ /dev/null @@ -1,104 +0,0 @@ -/** - * Copyright (c) Facebook, Inc. and its affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - * - * @flow - */ - -import warningWithoutStack from 'shared/warningWithoutStack'; - -// for .act's return value -type Thenable = { - then(resolve: () => void, reject?: () => void): void, - // ^ todo - maybe this should return a promise... -}; - -// a step counter when we descend/ascend from .act calls -let actingScopeDepth = 0; - -export default function createAct( - name: string, - setIsActingUpdatesInDev: boolean => void, - flushEffects: () => void, - batchedUpdates: *, // todo -) { - return function act(callback: () => void | Promise): Thenable { - const previousActingScopeDepth = actingScopeDepth; - actingScopeDepth++; - if (previousActingScopeDepth === 0) { - setIsActingUpdatesInDev(true); - } - - const result = batchedUpdates(callback); - if (result && typeof result.then === 'function') { - // the returned thenable MUST be called - let called = false; - setImmediate(() => { - if (!called) { - warningWithoutStack( - null, - 'You called %s.act() without awaiting its result. ' + - 'This could lead to unexpected testing behaviour, interleaving multiple act ' + - 'calls and mixing their scopes. You should - await act(async () => ...);', - name, - // todo - a better warning here. open to suggestions. - ); - } - }); - return { - then(fn, errorFn) { - called = true; - result.then(() => { - flushEffects(); - if (actingScopeDepth - 1 > previousActingScopeDepth) { - // if it's _less than_ previousActingScopeDepth, then we can assume the 'other' one has warned - warningWithoutStack( - null, - 'You seem to have interleaved multiple act() calls, this is not supported. ' + - 'Be sure to await previous sibling act calls before making a new one. ', - // todo - a better warning here. open to suggestions. - ); - } - actingScopeDepth--; - if (actingScopeDepth === 0) { - setIsActingUpdatesInDev(false); - } - fn(); - }, errorFn); - }, - }; - } else { - if (__DEV__) { - if (result !== undefined) { - warningWithoutStack( - false, - 'The callback passed to %s.act(...) function ' + - 'must return undefined, or a Promise. You returned %s', - name, - result, - ); - } - } - flushEffects(); - actingScopeDepth--; - - if (actingScopeDepth === 0) { - setIsActingUpdatesInDev(false); - } - return { - then() { - if (__DEV__) { - warningWithoutStack( - false, - // todo - well... why not? maybe this would be fine. - 'Do not await the result of calling %s.act(...) with sync logic, it is not a Promise.', - name, - ); - } - }, - }; - } - }; -} From d0daebf9447554abb06dbc7709b1f35e6688ac7b Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Fri, 15 Feb 2019 15:40:24 +0000 Subject: [PATCH 07/55] smaller bundle --- .../react-reconciler/src/ReactFiberScheduler.js | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index ab86ae0376c6..a6e93fa850dd 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -1830,7 +1830,7 @@ function scheduleWorkToRoot(fiber: Fiber, expirationTime): FiberRoot | null { // in a test-like environment, we want to warn if dispatchAction() is // called outside of a TestUtils.act(...)/batchedUpdates/render call. // so we have a a step counter for when we descend/ascend from -// actedUpdates() calls, and test on it for when to warn +// actedUpdates() calls, and test on it for when to warn let actingUpdatesScopeDepth = 0; export function actedUpdates(callback: () => void | Promise): Thenable { @@ -1841,11 +1841,11 @@ export function actedUpdates(callback: () => void | Promise): Thenable { } const result = batchedUpdates(callback); - if (result && typeof result.then === 'function') { + if (result && result.then) { // saving a few bytes without the typeof === 'function' check // the returned thenable MUST be called let called = false; - setTimeout(() => { - if (__DEV__) { + if (__DEV__) { + setTimeout(() => { if (!called) { warningWithoutStack( null, @@ -1855,10 +1855,10 @@ export function actedUpdates(callback: () => void | Promise): Thenable { // todo - a better warning here. open to suggestions. ); } - } - }, 0); + }, 0); + } return { - then(fn, errorFn) { + then(successFn, errorFn) { called = true; result.then(() => { flushPassiveEffects(); @@ -1874,8 +1874,7 @@ export function actedUpdates(callback: () => void | Promise): Thenable { } actingUpdatesScopeDepth--; } - - fn(); + successFn(); }, errorFn); }, }; From 00c2fd6d1ae1f972c3a6767e65549718e29b4d1f Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Fri, 15 Feb 2019 15:52:02 +0000 Subject: [PATCH 08/55] a comment for why we don't do typeof === 'function' --- packages/react-reconciler/src/ReactFiberScheduler.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index a6e93fa850dd..d183f1a8576d 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -1841,7 +1841,8 @@ export function actedUpdates(callback: () => void | Promise): Thenable { } const result = batchedUpdates(callback); - if (result && result.then) { // saving a few bytes without the typeof === 'function' check + if (result && result.then) { + // saving a few bytes without the typeof === 'function' check // the returned thenable MUST be called let called = false; if (__DEV__) { From 788fd73f643ef72b2ce821c3d2924b67595207be Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Fri, 15 Feb 2019 17:17:44 +0000 Subject: [PATCH 09/55] fix test --- .../src/__tests__/ReactHooks-test.internal.js | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js index f7ab26011b64..23fdcb852b70 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactHooks-test.internal.js @@ -886,11 +886,10 @@ describe('ReactHooks', () => { class Cls extends React.Component { render() { - act(() => - _setState(() => { - ReactCurrentDispatcher.current.readContext(ThemeContext); - }), + _setState(() => + ReactCurrentDispatcher.current.readContext(ThemeContext), ); + return null; } } @@ -902,13 +901,7 @@ describe('ReactHooks', () => { , ), - ).toWarnDev( - [ - 'Context can only be read while React is rendering', - 'Render methods should be a pure function of props and state', - ], - {withoutStack: 1}, - ); + ).toWarnDev(['Context can only be read while React is rendering']); }); it('warns when calling hooks inside useReducer', () => { From cd2f191352fd6b87e0a99ca84739c6dabdf05f2a Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Fri, 15 Feb 2019 18:00:12 +0000 Subject: [PATCH 10/55] pass tests - fire, prod --- .../react-dom/src/__tests__/ReactTestUtilsAct-test.js | 8 ++++++-- packages/react-dom/src/fire/ReactFire.js | 2 ++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js index ec7b7f55c086..906de7f316e9 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js @@ -239,7 +239,9 @@ describe('act', () => { act(async () => {}); // it's annoying that we have to wait a tick before this warning comes in await sleep(0); - expect(console.error).toHaveBeenCalledTimes(1); + if (__DEV__) { + expect(console.error).toHaveBeenCalledTimes(1); + } }); it('it warns if you try to interleave multiple act calls', async () => { @@ -255,7 +257,9 @@ describe('act', () => { }); await sleep(1000); - expect(console.error).toHaveBeenCalledTimes(1); + if (__DEV__) { + expect(console.error).toHaveBeenCalledTimes(1); + } }); it('commits and effects are guaranteed to be flushed', async () => { diff --git a/packages/react-dom/src/fire/ReactFire.js b/packages/react-dom/src/fire/ReactFire.js index 3e2f00b21f9f..6265a734ba3a 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, + actedUpdates, } from 'react-reconciler/inline.fire'; import {createPortal as createPortalImpl} from 'shared/ReactPortal'; import {canUseDOM} from 'shared/ExecutionEnvironment'; @@ -827,6 +828,7 @@ const ReactDOM: Object = { restoreStateIfNeeded, dispatchEvent, runEventsInBatch, + actedUpdates, ], }, }; From 6af7e80167848ece4719cbe71f4edc9088199272 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Fri, 15 Feb 2019 21:09:36 +0000 Subject: [PATCH 11/55] lose actContainerElement --- .../src/test-utils/ReactTestUtils.js | 28 ++++++++----------- .../ReactTestRenderer-test.internal.js | 4 +-- 2 files changed, 13 insertions(+), 19 deletions(-) diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js index 6063ea9b0e12..2f6448f0204f 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtils.js +++ b/packages/react-dom/src/test-utils/ReactTestUtils.js @@ -147,32 +147,28 @@ function validateClassInstance(inst, methodName) { ); } -// for .act's return value -type Thenable = { - then(resolve: () => void, reject?: () => void): void, - // ^ todo - maybe this should return a promise... -}; - -// a stub element, lazily initialized, used by act() when flushing effects -let actContainerElement = null; +let didWarnAboutActInNodejs = false; +let isBrowserLikeEnvironment = + typeof document !== 'undefined' && document !== null; -function act(callback: () => void | Promise): Thenable { - if (actContainerElement === null) { - // warn if we can't actually create the stub element - if (__DEV__) { +function act(callback: () => void | Promise) { + if (__DEV__) { + // warn if we're trying to use this in something like node (without jsdom) + if ( + didWarnAboutActInNodejs === false && + isBrowserLikeEnvironment === false + ) { + didWarnAboutActInNodejs = true; warningWithoutStack( typeof document !== 'undefined' && document !== null && typeof document.createElement === 'function', 'It looks like you called ReactTestUtils.act(...) in a non-browser environment. ' + "If you're using TestRenderer for your tests, you should call " + - 'ReactTestRenderer.act(...) instead of TestUtils.act(...).', + 'ReactTestRenderer.act(...) instead of ReactTestUtils.act(...).', ); } - // then make it - actContainerElement = document.createElement('div'); } - return actedUpdates(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 5f82b3782059..91525f50fd98 100644 --- a/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js +++ b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js @@ -1048,9 +1048,7 @@ describe('ReactTestRenderer', () => { // so as suggested, we call resetModules() to carry on with the test jest.resetModules(); const {act} = require('react-dom/test-utils'); - expect(() => { - expect(() => act(() => {})).toThrow('document is not defined'); - }).toWarnDev( + expect(() => act(() => {})).toWarnDev( [ 'It looks like you called ReactTestUtils.act(...) in a non-browser environment', ], From a8fb80b1ee7da9f6740b7da990c08a588373146c Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Fri, 15 Feb 2019 21:44:10 +0000 Subject: [PATCH 12/55] tighter --- packages/react-dom/src/test-utils/ReactTestUtils.js | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js index 2f6448f0204f..828ab9955965 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtils.js +++ b/packages/react-dom/src/test-utils/ReactTestUtils.js @@ -148,21 +148,13 @@ function validateClassInstance(inst, methodName) { } let didWarnAboutActInNodejs = false; -let isBrowserLikeEnvironment = - typeof document !== 'undefined' && document !== null; - function act(callback: () => void | Promise) { if (__DEV__) { // warn if we're trying to use this in something like node (without jsdom) - if ( - didWarnAboutActInNodejs === false && - isBrowserLikeEnvironment === false - ) { + if (didWarnAboutActInNodejs === false) { didWarnAboutActInNodejs = true; warningWithoutStack( - typeof document !== 'undefined' && - document !== null && - typeof document.createElement === 'function', + typeof document !== 'undefined' && document !== null, 'It looks like you called ReactTestUtils.act(...) in a non-browser environment. ' + "If you're using TestRenderer for your tests, you should call " + 'ReactTestRenderer.act(...) instead of ReactTestUtils.act(...).', From 955d590b28708371d524d8111da1516a040b04ad Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Sat, 16 Feb 2019 02:25:27 +0000 Subject: [PATCH 13/55] write a test for TestRenderer it's an odd one, because not only does sync act not flush effects correctly, but the async one does (wut). verified it's fine with the dom version. --- .../src/__tests__/ReactTestUtilsAct-test.js | 23 +++++++++++ .../src/__tests__/ReactTestRenderer-test.js | 39 +++++++++++++++++++ 2 files changed, 62 insertions(+) diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js index 906de7f316e9..f6bf57e3b367 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js @@ -234,6 +234,29 @@ describe('act', () => { }); }); + it('can handle async await', async () => { + function App() { + let [ctr, setCtr] = React.useState(0); + async function someAsyncFunction() { + await sleep(0); + setCtr(1); + } + React.useEffect(() => { + someAsyncFunction(); + }, []); + return ctr; + } + const el = document.createElement('div'); + + await act(async () => { + act(() => { + ReactDOM.render(, el); + }); + await sleep(100); + }); + expect(el.innerHTML).toEqual('1'); + }); + it('warns if you do not await an act call', async () => { spyOnDevAndProd(console, 'error'); act(async () => {}); diff --git a/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js index 29eb6d00d409..c0e4c3029993 100644 --- a/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js +++ b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js @@ -17,6 +17,14 @@ const React = require('react'); const ReactCache = require('react-cache'); const ReactTestRenderer = require('react-test-renderer'); +function sleep(period) { + return new Promise(resolve => { + setTimeout(() => { + resolve(true); + }, period); + }); +} + describe('ReactTestRenderer', () => { it('should warn if used to render a ReactDOM portal', () => { const container = document.createElement('div'); @@ -44,6 +52,37 @@ describe('ReactTestRenderer', () => { }); expect(root.toJSON()).toEqual('1'); }); + describe('async', () => { + beforeEach(() => { + jest.useRealTimers() + }) + afterEach(() => { + jest.useFakeTimers() + }) + it('should work async too', async () => { + function App() { + let [ctr, setCtr] = React.useState(0); + async function someAsyncFunction() { + await sleep(100); + setCtr(1); + } + React.useEffect(() => { + someAsyncFunction(); + }, []); + return ctr; + } + let root; + await ReactTestRenderer.act(async () => { + // todo - I don't understand why a nested async act call + // flushes the effect correctly + await ReactTestRenderer.act(async () => { + root = ReactTestRenderer.create(); + }); + await sleep(200); + }); + expect(root.toJSON()).toEqual('1'); + }); + }); }); describe('timed out Suspense hidden subtrees should not be observable via toJSON', () => { From ee8086ae04624993a02e7b5a71b31f333b57ebf5 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Sat, 16 Feb 2019 02:26:21 +0000 Subject: [PATCH 14/55] lint --- .../src/__tests__/ReactTestRenderer-test.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js index c0e4c3029993..e13c516d5f85 100644 --- a/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js +++ b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js @@ -54,12 +54,12 @@ describe('ReactTestRenderer', () => { }); describe('async', () => { beforeEach(() => { - jest.useRealTimers() - }) + jest.useRealTimers(); + }); afterEach(() => { - jest.useFakeTimers() - }) - it('should work async too', async () => { + jest.useFakeTimers(); + }); + it('should work async too', async () => { function App() { let [ctr, setCtr] = React.useState(0); async function someAsyncFunction() { From 441e5979ec9887f9197e15da37069b88de8fd458 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Sat, 16 Feb 2019 15:38:10 +0000 Subject: [PATCH 15/55] rewrote to move flushing logic closer to the renderer the scheduler's `flushPassiveEffects` didn't work as expected for the test renderer, so I decided to go back to the hack (rendering a dumb container) This also makes reactdom not as heavy (by a few bytes, but still). --- .../src/__tests__/ReactTestUtilsAct-test.js | 2 +- .../src/test-utils/ReactTestUtils.js | 68 ++++++++++++++++--- .../src/createReactNoop.js | 45 +++++++++++- .../src/ReactFiberScheduler.js | 58 +++------------- .../src/ReactTestRenderer.js | 64 ++++++++++++++++- .../ReactTestRenderer-test.internal.js | 21 +++++- 6 files changed, 196 insertions(+), 62 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js index f6bf57e3b367..e916fcb420ed 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js @@ -238,7 +238,7 @@ describe('act', () => { function App() { let [ctr, setCtr] = React.useState(0); async function someAsyncFunction() { - await sleep(0); + await null; setCtr(1); } React.useEffect(() => { diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js index 828ab9955965..35ab0352924c 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtils.js +++ b/packages/react-dom/src/test-utils/ReactTestUtils.js @@ -147,21 +147,67 @@ function validateClassInstance(inst, methodName) { ); } +// a stub element, lazily initialized, used by act() when flushing effects +let actContainerElement = null; let didWarnAboutActInNodejs = false; function act(callback: () => void | Promise) { - if (__DEV__) { - // warn if we're trying to use this in something like node (without jsdom) - if (didWarnAboutActInNodejs === false) { - didWarnAboutActInNodejs = true; - warningWithoutStack( - typeof document !== 'undefined' && document !== null, - 'It looks like you called ReactTestUtils.act(...) in a non-browser environment. ' + - "If you're using TestRenderer for your tests, you should call " + - 'ReactTestRenderer.act(...) instead of ReactTestUtils.act(...).', - ); + if (actContainerElement === null) { + if (__DEV__) { + // warn if we're trying to use this in something like node (without jsdom) + if (didWarnAboutActInNodejs === false) { + didWarnAboutActInNodejs = true; + warningWithoutStack( + typeof document !== 'undefined' && document !== null, + 'It looks like you called ReactTestUtils.act(...) in a non-browser environment. ' + + "If you're using TestRenderer for your tests, you should call " + + 'ReactTestRenderer.act(...) instead of ReactTestUtils.act(...).', + ); + } } + // now make the stub element + actContainerElement = document.createElement('div'); + } + // note: keep these warning messages in sync with + // createReactNoop.js and ReactTestRenderer.js + const result = actedUpdates(callback); + if (result && result.then) { + let called = false; + if (__DEV__) { + setTimeout(() => { + if (!called) { + warningWithoutStack( + null, + 'You called act() without awaiting its result. ' + + 'This could lead to unexpected testing behaviour, interleaving multiple act ' + + 'calls and mixing their scopes. You should - await act(async () => ...);', + // todo - a better warning here. open to suggestions. + ); + } + }, 0); + } + return { + then(successFn, errorFn) { + called = true; + return result.then(() => { + ReactDOM.render(
, actContainerElement); + successFn(); + }, errorFn); + }, + }; + } else { + ReactDOM.render(
, actContainerElement); + return { + then() { + if (__DEV__) { + warningWithoutStack( + false, + // todo - well... why not? maybe this would be fine. + 'Do not await the result of calling act(...) with sync logic, it is not a Promise.', + ); + } + }, + }; } - return actedUpdates(callback); } /** diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index a4c9856a96bb..e286bf8d09a6 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -21,6 +21,7 @@ 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'; type Container = { rootID: string, @@ -864,7 +865,49 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { interactiveUpdates: NoopRenderer.interactiveUpdates, - act: NoopRenderer.actedUpdates, + act(callback: () => void | Promise) { + // note: keep these warning messages in sync with + // ReactTestRenderer.js and ReactTestUtils.js + const result = NoopRenderer.actedUpdates(callback); + if (result && result.then) { + let called = false; + if (__DEV__) { + setTimeout(() => { + if (!called) { + warningWithoutStack( + null, + 'You called act() without awaiting its result. ' + + 'This could lead to unexpected testing behaviour, interleaving multiple act ' + + 'calls and mixing their scopes. You should - await act(async () => ...);', + // todo - a better warning here. open to suggestions. + ); + } + }, 0); + } + return { + then(successFn: (*) => *, errorFn: (*) => *) { + called = true; + return result.then(() => { + ReactNoop.flushPassiveEffects(); + successFn(); + }, errorFn); + }, + }; + } else { + ReactNoop.flushPassiveEffects(); + return { + then() { + if (__DEV__) { + warningWithoutStack( + false, + // todo - well... why not? maybe this would be fine. + 'Do not await the result of calling act(...) with sync logic, it is not a Promise.', + ); + } + }, + }; + } + }, flushSync(fn: () => mixed) { yieldedValues = []; diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index d183f1a8576d..edb98dfb9b03 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -1833,7 +1833,7 @@ function scheduleWorkToRoot(fiber: Fiber, expirationTime): FiberRoot | null { // actedUpdates() calls, and test on it for when to warn let actingUpdatesScopeDepth = 0; -export function actedUpdates(callback: () => void | Promise): Thenable { +export function actedUpdates(callback: () => void | Promise) { let previousActingUpdatesScopeDepth; if (__DEV__) { previousActingUpdatesScopeDepth = actingUpdatesScopeDepth; @@ -1843,42 +1843,20 @@ export function actedUpdates(callback: () => void | Promise): Thenable { const result = batchedUpdates(callback); if (result && result.then) { // saving a few bytes without the typeof === 'function' check - // the returned thenable MUST be called - let called = false; - if (__DEV__) { - setTimeout(() => { - if (!called) { + return result.then(() => { + if (__DEV__) { + if (actingUpdatesScopeDepth - 1 > previousActingUpdatesScopeDepth) { + // if it's _less than_ previousActingUpdatesScopeDepth, then we can assume the 'other' one has warned warningWithoutStack( null, - 'You called act() without awaiting its result. ' + - 'This could lead to unexpected testing behaviour, interleaving multiple act ' + - 'calls and mixing their scopes. You should - await act(async () => ...);', + 'You seem to have interleaved multiple act() calls, this is not supported. ' + + 'Be sure to await previous sibling act calls before making a new one. ', // todo - a better warning here. open to suggestions. ); } - }, 0); - } - return { - then(successFn, errorFn) { - called = true; - result.then(() => { - flushPassiveEffects(); - if (__DEV__) { - if (actingUpdatesScopeDepth - 1 > previousActingUpdatesScopeDepth) { - // if it's _less than_ previousActingUpdatesScopeDepth, then we can assume the 'other' one has warned - warningWithoutStack( - null, - 'You seem to have interleaved multiple act() calls, this is not supported. ' + - 'Be sure to await previous sibling act calls before making a new one. ', - // todo - a better warning here. open to suggestions. - ); - } - actingUpdatesScopeDepth--; - } - successFn(); - }, errorFn); - }, - }; + actingUpdatesScopeDepth--; + } + }); } else { if (__DEV__) { if (result !== undefined) { @@ -1889,23 +1867,9 @@ export function actedUpdates(callback: () => void | Promise): Thenable { result, ); } - } - flushPassiveEffects(); - if (__DEV__) { actingUpdatesScopeDepth--; + // todo - this needs a warning too } - - return { - then() { - if (__DEV__) { - warningWithoutStack( - false, - // todo - well... why not? maybe this would be fine. - 'Do not await the result of calling act(...) with sync logic, it is not a Promise.', - ); - } - }, - }; } } diff --git a/packages/react-test-renderer/src/ReactTestRenderer.js b/packages/react-test-renderer/src/ReactTestRenderer.js index 3f30cb20f32d..fe63abe09aa1 100644 --- a/packages/react-test-renderer/src/ReactTestRenderer.js +++ b/packages/react-test-renderer/src/ReactTestRenderer.js @@ -40,6 +40,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 { @@ -559,9 +560,70 @@ const ReactTestRendererFiber = { unstable_setNowImplementation: setNowImplementation, - act: actedUpdates, + act, }; +// 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); +} + +function act(callback: () => void | Promise) { + // note: keep these warning messages in sync with + // createReactNoop.js and ReactTestUtils.js + const result = actedUpdates(callback); + if (result && result.then) { + let called = false; + if (__DEV__) { + setTimeout(() => { + if (!called) { + warningWithoutStack( + null, + 'You called act() without awaiting its result. ' + + 'This could lead to unexpected testing behaviour, interleaving multiple act ' + + 'calls and mixing their scopes. You should - await act(async () => ...);', + // todo - a better warning here. open to suggestions. + ); + } + }, 0); + } + return { + then(successFn: (*) => *, errorFn: (*) => *) { + called = true; + return result.then(() => { + flushPassiveEffects(); + successFn(); + }, errorFn); + }, + }; + } else { + flushPassiveEffects(); + return { + then() { + if (__DEV__) { + warningWithoutStack( + false, + // todo - well... why not? maybe this would be fine. + 'Do not await the result of calling act(...) with sync logic, it is not a Promise.', + ); + } + }, + }; + } +} + 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 91525f50fd98..cfc04047cd35 100644 --- a/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js +++ b/packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.internal.js @@ -1043,12 +1043,31 @@ describe('ReactTestRenderer', () => { expect(called).toBe(true); }); + it("warns if you don't use .act", () => { + let setCtr; + function App(props) { + let [ctr, _setCtr] = React.useState(0); + setCtr = _setCtr; + return ctr; + } + + ReactTestRenderer.create(); + + expect(() => { + setCtr(1); + }).toWarnDev([ + 'An update to App inside a test was not wrapped in act(...)', + ]); + }); + it('warns and throws if you use TestUtils.act instead of TestRenderer.act in node', () => { // we warn when you try to load 2 renderers in the same 'scope' // so as suggested, we call resetModules() to carry on with the test jest.resetModules(); const {act} = require('react-dom/test-utils'); - expect(() => act(() => {})).toWarnDev( + expect(() => { + expect(() => act(() => {})).toThrow('document is not defined'); + }).toWarnDev( [ 'It looks like you called ReactTestUtils.act(...) in a non-browser environment', ], From c07174bd3b3d207f884cb0ff8b451a7ec396ccc5 Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Sat, 16 Feb 2019 15:46:06 +0000 Subject: [PATCH 16/55] move it around so the delta isn't too bad --- .../src/test-utils/ReactTestUtils.js | 122 +++++++++--------- .../src/ReactTestRenderer.js | 88 ++++++------- 2 files changed, 104 insertions(+), 106 deletions(-) diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js index 35ab0352924c..a736e26a0db4 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtils.js +++ b/packages/react-dom/src/test-utils/ReactTestUtils.js @@ -147,68 +147,10 @@ function validateClassInstance(inst, methodName) { ); } -// a stub element, lazily initialized, used by act() when flushing effects +// a plain dom element, lazily initialized, used by act() when flushing effects let actContainerElement = null; + let didWarnAboutActInNodejs = false; -function act(callback: () => void | Promise) { - if (actContainerElement === null) { - if (__DEV__) { - // warn if we're trying to use this in something like node (without jsdom) - if (didWarnAboutActInNodejs === false) { - didWarnAboutActInNodejs = true; - warningWithoutStack( - typeof document !== 'undefined' && document !== null, - 'It looks like you called ReactTestUtils.act(...) in a non-browser environment. ' + - "If you're using TestRenderer for your tests, you should call " + - 'ReactTestRenderer.act(...) instead of ReactTestUtils.act(...).', - ); - } - } - // now make the stub element - actContainerElement = document.createElement('div'); - } - // note: keep these warning messages in sync with - // createReactNoop.js and ReactTestRenderer.js - const result = actedUpdates(callback); - if (result && result.then) { - let called = false; - if (__DEV__) { - setTimeout(() => { - if (!called) { - warningWithoutStack( - null, - 'You called act() without awaiting its result. ' + - 'This could lead to unexpected testing behaviour, interleaving multiple act ' + - 'calls and mixing their scopes. You should - await act(async () => ...);', - // todo - a better warning here. open to suggestions. - ); - } - }, 0); - } - return { - then(successFn, errorFn) { - called = true; - return result.then(() => { - ReactDOM.render(
, actContainerElement); - successFn(); - }, errorFn); - }, - }; - } else { - ReactDOM.render(
, actContainerElement); - return { - then() { - if (__DEV__) { - warningWithoutStack( - false, - // todo - well... why not? maybe this would be fine. - 'Do not await the result of calling act(...) with sync logic, it is not a Promise.', - ); - } - }, - }; - } -} /** * Utilities for making it easy to test React components. @@ -445,7 +387,65 @@ const ReactTestUtils = { Simulate: null, SimulateNative: {}, - act, + act(callback: () => void | Promise) { + if (actContainerElement === null) { + if (__DEV__) { + // warn if we're trying to use this in something like node (without jsdom) + if (didWarnAboutActInNodejs === false) { + didWarnAboutActInNodejs = true; + warningWithoutStack( + typeof document !== 'undefined' && document !== null, + 'It looks like you called ReactTestUtils.act(...) in a non-browser environment. ' + + "If you're using TestRenderer for your tests, you should call " + + 'ReactTestRenderer.act(...) instead of ReactTestUtils.act(...).', + ); + } + } + // now make the stub element + actContainerElement = document.createElement('div'); + } + // note: keep these warning messages in sync with + // createReactNoop.js and ReactTestRenderer.js + const result = actedUpdates(callback); + if (result && result.then) { + let called = false; + if (__DEV__) { + setTimeout(() => { + if (!called) { + warningWithoutStack( + null, + 'You called act() without awaiting its result. ' + + 'This could lead to unexpected testing behaviour, interleaving multiple act ' + + 'calls and mixing their scopes. You should - await act(async () => ...);', + // todo - a better warning here. open to suggestions. + ); + } + }, 0); + } + return { + then(successFn, errorFn) { + called = true; + return result.then(() => { + ReactDOM.render(
, actContainerElement); + successFn(); + }, errorFn); + }, + }; + } else { + ReactDOM.render(
, actContainerElement); + return { + then() { + if (__DEV__) { + warningWithoutStack( + false, + // todo - well... why not? maybe this would be fine. + 'Do not await the result of calling act(...) with sync logic, it is not a Promise.', + ); + } + }, + }; + } + }, }; /** diff --git a/packages/react-test-renderer/src/ReactTestRenderer.js b/packages/react-test-renderer/src/ReactTestRenderer.js index fe63abe09aa1..e6eafbb1ace8 100644 --- a/packages/react-test-renderer/src/ReactTestRenderer.js +++ b/packages/react-test-renderer/src/ReactTestRenderer.js @@ -560,7 +560,49 @@ const ReactTestRendererFiber = { unstable_setNowImplementation: setNowImplementation, - act, + act(callback: () => void | Promise) { + // note: keep these warning messages in sync with + // createReactNoop.js and ReactTestUtils.js + const result = actedUpdates(callback); + if (result && result.then) { + let called = false; + if (__DEV__) { + setTimeout(() => { + if (!called) { + warningWithoutStack( + null, + 'You called act() without awaiting its result. ' + + 'This could lead to unexpected testing behaviour, interleaving multiple act ' + + 'calls and mixing their scopes. You should - await act(async () => ...);', + // todo - a better warning here. open to suggestions. + ); + } + }, 0); + } + return { + then(successFn: (*) => *, errorFn: (*) => *) { + called = true; + return result.then(() => { + flushPassiveEffects(); + successFn(); + }, errorFn); + }, + }; + } else { + flushPassiveEffects(); + return { + then() { + if (__DEV__) { + warningWithoutStack( + false, + // todo - well... why not? maybe this would be fine. + 'Do not await the result of calling act(...) with sync logic, it is not a Promise.', + ); + } + }, + }; + } + }, }; // root used to flush effects during .act() calls @@ -580,50 +622,6 @@ function flushPassiveEffects() { updateContainer(null, actRoot, null, null); } -function act(callback: () => void | Promise) { - // note: keep these warning messages in sync with - // createReactNoop.js and ReactTestUtils.js - const result = actedUpdates(callback); - if (result && result.then) { - let called = false; - if (__DEV__) { - setTimeout(() => { - if (!called) { - warningWithoutStack( - null, - 'You called act() without awaiting its result. ' + - 'This could lead to unexpected testing behaviour, interleaving multiple act ' + - 'calls and mixing their scopes. You should - await act(async () => ...);', - // todo - a better warning here. open to suggestions. - ); - } - }, 0); - } - return { - then(successFn: (*) => *, errorFn: (*) => *) { - called = true; - return result.then(() => { - flushPassiveEffects(); - successFn(); - }, errorFn); - }, - }; - } else { - flushPassiveEffects(); - return { - then() { - if (__DEV__) { - warningWithoutStack( - false, - // todo - well... why not? maybe this would be fine. - 'Do not await the result of calling act(...) with sync logic, it is not a Promise.', - ); - } - }, - }; - } -} - const fiberToWrapper = new WeakMap(); function wrapFiber(fiber: Fiber): ReactTestInstance { let wrapper = fiberToWrapper.get(fiber); From 0fe5318473cc56f12058edd483bc113e2c13f53c Mon Sep 17 00:00:00 2001 From: Sunil Pai Date: Mon, 25 Feb 2019 20:00:49 +0000 Subject: [PATCH 17/55] cleanups fix promise chaining propagate errors correctly test for thenable the 'right' way more tests! tidier! ponies! --- .../src/__tests__/ReactTestUtilsAct-test.js | 101 ++++++++---------- .../src/test-utils/ReactTestUtils.js | 10 +- .../__tests__/ReactNoopRendererAct-test.js | 50 +++++++++ .../src/createReactNoop.js | 10 +- .../src/ReactFiberScheduler.js | 49 ++++++--- .../src/ReactTestRenderer.js | 10 +- .../ReactTestRenderer-test.internal.js | 66 +++--------- .../src/__tests__/ReactTestRenderer-test.js | 71 +++++++++--- 8 files changed, 217 insertions(+), 150 deletions(-) create mode 100644 packages/react-noop-renderer/__tests__/ReactNoopRendererAct-test.js diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js index e916fcb420ed..e3ed3911f57d 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js @@ -32,31 +32,25 @@ describe('act', () => { }); describe('sync', () => { - it('can use act to batch effects', () => { + it('can use act to flush effects', () => { function App(props) { React.useEffect(props.callback); return null; } - const container = document.createElement('div'); - document.body.appendChild(container); - try { - let called = false; - act(() => { - ReactDOM.render( - { - called = true; - }} - />, - container, - ); - }); + let called = false; + act(() => { + ReactDOM.render( + { + called = true; + }} + />, + document.createElement('div'), + ); + }); - expect(called).toBe(true); - } finally { - document.body.removeChild(container); - } + expect(called).toBe(true); }); it('flushes effects on every call', () => { @@ -67,12 +61,13 @@ describe('act', () => { }); return ( ); } const container = document.createElement('div'); + // attach to body so events works document.body.appendChild(container); let calledCtr = 0; act(() => { @@ -100,39 +95,16 @@ describe('act', () => { expect(calledCtr).toBe(4); act(click); expect(calledCtr).toBe(5); + expect(button.innerHTML).toBe('5'); document.body.removeChild(container); }); - it('can use act 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; - act(() => { - ReactDOM.render(, container); - }); - button = document.getElementById('button'); - expect(button.innerHTML).toBe('0'); - 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(...)', () => { - let setValueRef = null; + let setValue = null; function App() { - let [value, setValue] = React.useState(0); - setValueRef = setValue; + let [value, _setValue] = React.useState(0); + setValue = _setValue; return (