diff --git a/fixtures/dom/package.json b/fixtures/dom/package.json index 3282c85fa58a7..9bf118f0c29cd 100644 --- a/fixtures/dom/package.json +++ b/fixtures/dom/package.json @@ -18,7 +18,7 @@ }, "scripts": { "start": "react-scripts start", - "prestart": "cp ../../build/node_modules/scheduler/umd/scheduler-unstable_mock.development.js ../../build/node_modules/scheduler/umd/scheduler-unstable_mock.production.min.js ../../build/node_modules/react/umd/react.development.js ../../build/node_modules/react-dom/umd/react-dom.development.js ../../build/node_modules/react/umd/react.production.min.js ../../build/node_modules/react-dom/umd/react-dom.production.min.js ../../build/node_modules/react-dom/umd/react-dom-server.browser.development.js ../../build/node_modules/react-dom/umd/react-dom-server.browser.production.min.js ../../build/node_modules/react-dom/umd/react-dom-test-utils.development.js ../../build/node_modules/react-dom/umd/react-dom-test-utils.production.min.js public/", + "prestart": "cp ../../build/node_modules/scheduler/umd/scheduler-unstable_mock.development.js ../../build/node_modules/scheduler/umd/scheduler-unstable_mock.production.min.js ../../build/node_modules/react/umd/react.development.js ../../build/node_modules/react-dom/umd/react-dom.development.js ../../build/node_modules/react/umd/react.production.min.js ../../build/node_modules/react-dom/umd/react-dom.production.min.js ../../build/node_modules/react-dom/umd/react-dom-server.browser.development.js ../../build/node_modules/react-dom/umd/react-dom-server.browser.production.min.js ../../build/node_modules/react-dom/umd/react-dom-test-utils.development.js ../../build/node_modules/react-dom/umd/react-dom-test-utils.production.min.js public/ && cp -a ../../build/node_modules/. node_modules", "build": "react-scripts build && cp build/index.html build/200.html", "test": "react-scripts test --env=jsdom", "eject": "react-scripts eject" diff --git a/fixtures/dom/public/act-dom.html b/fixtures/dom/public/act-dom.html index 2100026a5ba45..29152dc22dcb1 100644 --- a/fixtures/dom/public/act-dom.html +++ b/fixtures/dom/public/act-dom.html @@ -15,7 +15,6 @@ - + + async function testAsyncAct() { + const el = document.createElement("div"); + await ReactTestUtils.act(async () => { + ReactDOM.render(React.createElement(App), el); + }); + // all 5 ticks present and accounted for + console.log(el.innerHTML); + } + + testAsyncAct(); + + diff --git a/fixtures/dom/src/index.test.js b/fixtures/dom/src/index.test.js new file mode 100644 index 0000000000000..8c9f88600bc1a --- /dev/null +++ b/fixtures/dom/src/index.test.js @@ -0,0 +1,98 @@ +import React from 'react'; +import ReactDOM from 'react-dom'; +import TestUtils from 'react-dom/test-utils'; +import TestRenderer from 'react-test-renderer'; + +let spy; +beforeEach(() => { + spy = jest.spyOn(console, 'error').mockImplementation(() => {}); +}); + +function confirmWarning() { + expect(spy).toHaveBeenCalledWith( + expect.stringContaining( + "It looks like you're using the wrong act() around your test interactions." + ), + '' + ); +} + +function App(props) { + return 'hello world'; +} + +it("doesn't warn when you use the right act + renderer: dom", () => { + TestUtils.act(() => { + TestUtils.renderIntoDocument(); + }); + expect(spy).not.toHaveBeenCalled(); +}); + +it("doesn't warn when you use the right act + renderer: test", () => { + TestRenderer.act(() => { + TestRenderer.create(); + }); + expect(spy).not.toHaveBeenCalled(); +}); + +it('works with createRoot().render combo', () => { + const root = ReactDOM.unstable_createRoot(document.createElement('div')); + TestRenderer.act(() => { + root.render(); + }); + confirmWarning(); +}); + +it('warns when using the wrong act version - test + dom: render', () => { + TestRenderer.act(() => { + TestUtils.renderIntoDocument(); + }); + confirmWarning(); +}); + +it('warns when using the wrong act version - test + dom: updates', () => { + let setCtr; + function Counter(props) { + const [ctr, _setCtr] = React.useState(0); + setCtr = _setCtr; + return ctr; + } + TestUtils.renderIntoDocument(); + TestRenderer.act(() => { + setCtr(1); + }); + confirmWarning(); +}); + +it('warns when using the wrong act version - dom + test: .create()', () => { + TestUtils.act(() => { + TestRenderer.create(); + }); + confirmWarning(); +}); + +it('warns when using the wrong act version - dom + test: .update()', () => { + let root; + // use the right one here so we don't get the first warning + TestRenderer.act(() => { + root = TestRenderer.create(); + }); + TestUtils.act(() => { + root.update(); + }); + confirmWarning(); +}); + +it('warns when using the wrong act version - dom + test: updates', () => { + let setCtr; + function Counter(props) { + const [ctr, _setCtr] = React.useState(0); + setCtr = _setCtr; + return ctr; + } + const root = TestRenderer.create(); + TestUtils.act(() => { + setCtr(1); + }); + confirmWarning(); +}); diff --git a/package.json b/package.json index 97998b6b6d4f7..03ba206a0d3bd 100644 --- a/package.json +++ b/package.json @@ -107,6 +107,7 @@ "test-prod-build": "yarn test-build-prod", "test-build": "cross-env NODE_ENV=development jest --config ./scripts/jest/config.build.js", "test-build-prod": "cross-env NODE_ENV=production jest --config ./scripts/jest/config.build.js", + "test-dom-fixture": "cd fixtures/dom && yarn && yarn prestart && yarn test", "flow": "node ./scripts/tasks/flow.js", "flow-ci": "node ./scripts/tasks/flow-ci.js", "prettier": "node ./scripts/prettier/index.js write-changed", diff --git a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js index fd32092584d06..be0a832531535 100644 --- a/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js +++ b/packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js @@ -259,13 +259,13 @@ function runActTests(label, render, unmount) { it('warns if you return a value inside act', () => { expect(() => act(() => null)).toWarnDev( [ - 'The callback passed to 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 act(...) function must return undefined, or a Promise.', + 'The callback passed to act(...) function must return undefined or a Promise.', ], {withoutStack: true}, ); @@ -274,7 +274,7 @@ function runActTests(label, render, unmount) { it('warns if you try to await a sync .act call', () => { expect(() => act(() => {}).then(() => {})).toWarnDev( [ - 'Do not await the result of calling act(...) with sync logic, it is not a Promise.', + 'Do not await the result of calling a synchronous act(...), it is not a Promise.', ], {withoutStack: true}, ); @@ -337,7 +337,7 @@ function runActTests(label, render, unmount) { if (__DEV__) { expect(console.error.calls.count()).toEqual(1); expect(console.error.calls.argsFor(0)[0]).toMatch( - 'You called act(async () => ...) without await.', + 'You called act(async () => ...) without awaiting its result.', ); } }); diff --git a/packages/react-dom/src/test-utils/ReactTestUtils.js b/packages/react-dom/src/test-utils/ReactTestUtils.js index f463cc4aa6a32..a9285db8e385a 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtils.js +++ b/packages/react-dom/src/test-utils/ReactTestUtils.js @@ -42,8 +42,9 @@ const [ restoreStateIfNeeded, dispatchEvent, runEventsInBatch, - // eslint-disable-next-line no-unused-vars + /* eslint-disable no-unused-vars */ flushPassiveEffects, + /* eslint-enable no-unused-vars */ ] = ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Events; function Event(suffix) {} diff --git a/packages/react-dom/src/test-utils/ReactTestUtilsAct.js b/packages/react-dom/src/test-utils/ReactTestUtilsAct.js index 9667c1fbd5bbe..602e28c095dbe 100644 --- a/packages/react-dom/src/test-utils/ReactTestUtilsAct.js +++ b/packages/react-dom/src/test-utils/ReactTestUtilsAct.js @@ -37,7 +37,7 @@ const [ const batchedUpdates = ReactDOM.unstable_batchedUpdates; -const {ReactShouldWarnActingUpdates} = ReactSharedInternals; +const {ReactActingRendererSigil} = ReactSharedInternals; // this implementation should be exactly the same in // ReactTestUtilsAct.js, ReactTestRendererAct.js, createReactNoop.js @@ -85,18 +85,20 @@ let actingUpdatesScopeDepth = 0; function act(callback: () => Thenable) { let previousActingUpdatesScopeDepth; + let previousActingUpdatesSigil; if (__DEV__) { previousActingUpdatesScopeDepth = actingUpdatesScopeDepth; + previousActingUpdatesSigil = ReactActingRendererSigil.current; actingUpdatesScopeDepth++; - ReactShouldWarnActingUpdates.current = true; + // we use the function flushPassiveEffects directly as the sigil, + // since it's unique to a renderer + ReactActingRendererSigil.current = flushPassiveEffects; } function onDone() { if (__DEV__) { actingUpdatesScopeDepth--; - if (actingUpdatesScopeDepth === 0) { - ReactShouldWarnActingUpdates.current = false; - } + ReactActingRendererSigil.current = previousActingUpdatesSigil; if (actingUpdatesScopeDepth > previousActingUpdatesScopeDepth) { // if it's _less than_ previousActingUpdatesScopeDepth, then we can assume the 'other' one has warned warningWithoutStack( @@ -126,9 +128,10 @@ function act(callback: () => Thenable) { if (called === false) { warningWithoutStack( null, - 'You called act(async () => ...) without await. ' + + 'You called act(async () => ...) without awaiting its result. ' + 'This could lead to unexpected testing behaviour, interleaving multiple act ' + - 'calls and mixing their scopes. You should - await act(async () => ...);', + 'calls and mixing their scopes. You should await asynchronous act() calls:\n' + + 'await act(async () => ...);\n', ); } }); @@ -164,7 +167,7 @@ function act(callback: () => Thenable) { warningWithoutStack( result === undefined, 'The callback passed to act(...) function ' + - 'must return undefined, or a Promise. You returned %s', + 'must return undefined or a Promise. You returned %s', result, ); } @@ -184,7 +187,8 @@ function act(callback: () => Thenable) { if (__DEV__) { warningWithoutStack( false, - 'Do not await the result of calling act(...) with sync logic, it is not a Promise.', + 'Do not await the result of calling a synchronous act(...), it is not a Promise. \n' + + 'Remove the `await` statement before this act() call.', ); } resolve(); diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index e7bd09890a7bc..bdef88ce35010 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -81,7 +81,7 @@ type TextInstance = {| |}; type HostContext = Object; -const {ReactShouldWarnActingUpdates} = ReactSharedInternals; +const {ReactActingRendererSigil} = ReactSharedInternals; const NO_CONTEXT = {}; const UPPERCASE_CONTEXT = {}; @@ -698,18 +698,20 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { function act(callback: () => Thenable) { let previousActingUpdatesScopeDepth; + let previousActingUpdatesSigil; if (__DEV__) { previousActingUpdatesScopeDepth = actingUpdatesScopeDepth; + previousActingUpdatesSigil = ReactActingRendererSigil.current; actingUpdatesScopeDepth++; - ReactShouldWarnActingUpdates.current = true; + // we use the function flushPassiveEffects directly as the sigil, + // since it's unique to a renderer + ReactActingRendererSigil.current = flushPassiveEffects; } function onDone() { if (__DEV__) { actingUpdatesScopeDepth--; - if (actingUpdatesScopeDepth === 0) { - ReactShouldWarnActingUpdates.current = false; - } + ReactActingRendererSigil.current = previousActingUpdatesSigil; if (actingUpdatesScopeDepth > previousActingUpdatesScopeDepth) { // if it's _less than_ previousActingUpdatesScopeDepth, then we can assume the 'other' one has warned warningWithoutStack( @@ -739,9 +741,10 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { if (called === false) { warningWithoutStack( null, - 'You called act(async () => ...) without await. ' + + 'You called act(async () => ...) without awaiting its result. ' + 'This could lead to unexpected testing behaviour, interleaving multiple act ' + - 'calls and mixing their scopes. You should - await act(async () => ...);', + 'calls and mixing their scopes. You should await asynchronous act() calls:\n' + + 'await act(async () => ...);\n', ); } }); @@ -777,7 +780,7 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { warningWithoutStack( result === undefined, 'The callback passed to act(...) function ' + - 'must return undefined, or a Promise. You returned %s', + 'must return undefined or a Promise. You returned %s', result, ); } @@ -797,7 +800,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { if (__DEV__) { warningWithoutStack( false, - 'Do not await the result of calling act(...) with sync logic, it is not a Promise.', + 'Do not await the result of calling a synchronous act(...), it is not a Promise. \n' + + 'Remove the `await` statement before this act() call.', ); } resolve(); diff --git a/packages/react-reconciler/src/ReactFiberHooks.js b/packages/react-reconciler/src/ReactFiberHooks.js index f7eec92b3d2b7..433efcfd07766 100644 --- a/packages/react-reconciler/src/ReactFiberHooks.js +++ b/packages/react-reconciler/src/ReactFiberHooks.js @@ -36,6 +36,7 @@ import { requestCurrentTime, warnIfNotCurrentlyActingUpdatesInDev, markRenderEventTimeAndConfig, + warnIfNotScopedWithMatchingAct, } from './ReactFiberScheduler'; import invariant from 'shared/invariant'; @@ -1211,6 +1212,7 @@ function dispatchAction( // 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) { + warnIfNotScopedWithMatchingAct(fiber); warnIfNotCurrentlyActingUpdatesInDev(fiber); } } diff --git a/packages/react-reconciler/src/ReactFiberReconciler.js b/packages/react-reconciler/src/ReactFiberReconciler.js index 84291f4b7a16f..b94c014369f05 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.js @@ -55,6 +55,7 @@ import { interactiveUpdates, flushInteractiveUpdates, flushPassiveEffects, + warnIfNotScopedWithMatchingAct, } from './ReactFiberScheduler'; import {createUpdate, enqueueUpdate} from './ReactUpdateQueue'; import ReactFiberInstrumentation from './ReactFiberInstrumentation'; @@ -302,6 +303,14 @@ export function updateContainer( ): ExpirationTime { const current = container.current; const currentTime = requestCurrentTime(); + 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) { + warnIfNotScopedWithMatchingAct(current); + } + } const suspenseConfig = requestCurrentSuspenseConfig(); const expirationTime = computeExpirationForFiber( currentTime, diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index 9fd797baa10d2..562a46e4a5820 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -175,7 +175,7 @@ const ceil = Math.ceil; const { ReactCurrentDispatcher, ReactCurrentOwner, - ReactShouldWarnActingUpdates, + ReactActingRendererSigil, } = ReactSharedInternals; type WorkPhase = 0 | 1 | 2 | 3 | 4 | 5; @@ -2255,23 +2255,58 @@ function warnAboutInvalidUpdatesOnClassComponentsInDEV(fiber) { } } +export function warnIfNotScopedWithMatchingAct(fiber: Fiber): void { + if (__DEV__) { + if ( + ReactActingRendererSigil.current !== null && + // use the function flushPassiveEffects directly as the sigil + // so this comparison is expected here + ReactActingRendererSigil.current !== flushPassiveEffects + ) { + // it looks like we're using the wrong matching act(), so log a warning + warningWithoutStack( + false, + "It looks like you're using the wrong act() around your test interactions.\n" + + 'Be sure to use the matching version of act() corresponding to your renderer:\n\n' + + '// for react-dom:\n' + + "import {act} from 'react-test-utils';\n" + + '//...\n' + + 'act(() => ...);\n\n' + + '// for react-test-renderer:\n' + + "import TestRenderer from 'react-test-renderer';\n" + + 'const {act} = TestRenderer;\n' + + '//...\n' + + 'act(() => ...);' + + '%s', + getStackByFiberInDevAndProd(fiber), + ); + } + } +} + +// 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 +// act() calls, and test on it for when to warn +// It's a tuple with a single value. Look into ReactTestUtilsAct as an +// example of how we change the value + function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void { if (__DEV__) { if ( workPhase === NotWorking && - ReactShouldWarnActingUpdates.current === false + ReactActingRendererSigil.current !== flushPassiveEffects ) { 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' + + 'wrapped inside act(...):\n\n' + + 'await act(async () => {\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.' + + "This ensures that you're testing behavior similar to what a user would experience.\n" + ' Learn more at https://fb.me/react-wrap-tests-with-act' + '%s', getComponentName(fiber.type), diff --git a/packages/react-test-renderer/src/ReactTestRendererAct.js b/packages/react-test-renderer/src/ReactTestRendererAct.js index a3d1e4e9cc460..886e4d2beabb7 100644 --- a/packages/react-test-renderer/src/ReactTestRendererAct.js +++ b/packages/react-test-renderer/src/ReactTestRendererAct.js @@ -18,7 +18,7 @@ import {warnAboutMissingMockScheduler} from 'shared/ReactFeatureFlags'; import enqueueTask from 'shared/enqueueTask'; import * as Scheduler from 'scheduler'; -const {ReactShouldWarnActingUpdates} = ReactSharedInternals; +const {ReactActingRendererSigil} = ReactSharedInternals; // this implementation should be exactly the same in // ReactTestUtilsAct.js, ReactTestRendererAct.js, createReactNoop.js @@ -66,18 +66,20 @@ let actingUpdatesScopeDepth = 0; function act(callback: () => Thenable) { let previousActingUpdatesScopeDepth; + let previousActingUpdatesSigil; if (__DEV__) { previousActingUpdatesScopeDepth = actingUpdatesScopeDepth; + previousActingUpdatesSigil = ReactActingRendererSigil.current; actingUpdatesScopeDepth++; - ReactShouldWarnActingUpdates.current = true; + // we use the function flushPassiveEffects directly as the sigil, + // since it's unique to a renderer + ReactActingRendererSigil.current = flushPassiveEffects; } function onDone() { if (__DEV__) { actingUpdatesScopeDepth--; - if (actingUpdatesScopeDepth === 0) { - ReactShouldWarnActingUpdates.current = false; - } + ReactActingRendererSigil.current = previousActingUpdatesSigil; if (actingUpdatesScopeDepth > previousActingUpdatesScopeDepth) { // if it's _less than_ previousActingUpdatesScopeDepth, then we can assume the 'other' one has warned warningWithoutStack( @@ -107,9 +109,10 @@ function act(callback: () => Thenable) { if (called === false) { warningWithoutStack( null, - 'You called act(async () => ...) without await. ' + + 'You called act(async () => ...) without awaiting its result. ' + 'This could lead to unexpected testing behaviour, interleaving multiple act ' + - 'calls and mixing their scopes. You should - await act(async () => ...);', + 'calls and mixing their scopes. You should await asynchronous act() calls:\n' + + 'await act(async () => ...);\n', ); } }); @@ -145,7 +148,7 @@ function act(callback: () => Thenable) { warningWithoutStack( result === undefined, 'The callback passed to act(...) function ' + - 'must return undefined, or a Promise. You returned %s', + 'must return undefined or a Promise. You returned %s', result, ); } @@ -165,7 +168,8 @@ function act(callback: () => Thenable) { if (__DEV__) { warningWithoutStack( false, - 'Do not await the result of calling act(...) with sync logic, it is not a Promise.', + 'Do not await the result of calling a synchronous act(...), it is not a Promise. \n' + + 'Remove the `await` statement before this act() call.', ); } resolve(); diff --git a/packages/react/src/ReactActingRendererSigil.js b/packages/react/src/ReactActingRendererSigil.js new file mode 100644 index 0000000000000..a40ecca4c6604 --- /dev/null +++ b/packages/react/src/ReactActingRendererSigil.js @@ -0,0 +1,19 @@ +/** + * 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 + */ + +/** + * Used by act() to track whether you're outside an act() scope. + * We use a renderer's flushPassiveEffects as the sigil value + * so we can track identity of the renderer. + */ + +const ReactActingRendererSigil = { + current: (null: null | (() => boolean)), +}; +export default ReactActingRendererSigil; diff --git a/packages/react/src/ReactSharedInternals.js b/packages/react/src/ReactSharedInternals.js index 1bc95cffaad4e..de4a14adb3dbf 100644 --- a/packages/react/src/ReactSharedInternals.js +++ b/packages/react/src/ReactSharedInternals.js @@ -10,13 +10,13 @@ import ReactCurrentDispatcher from './ReactCurrentDispatcher'; import ReactCurrentBatchConfig from './ReactCurrentBatchConfig'; import ReactCurrentOwner from './ReactCurrentOwner'; import ReactDebugCurrentFrame from './ReactDebugCurrentFrame'; +import ReactActingRendererSigil from './ReactActingRendererSigil'; const ReactSharedInternals = { ReactCurrentDispatcher, ReactCurrentBatchConfig, ReactCurrentOwner, - // used by act() - ReactShouldWarnActingUpdates: {current: false}, + ReactActingRendererSigil, // Used by renderers to avoid bundling object-assign twice in UMD bundles: assign, }; diff --git a/scripts/circleci/test_entry_point.sh b/scripts/circleci/test_entry_point.sh index 5bf568028aeb2..a1343ce4cf983 100755 --- a/scripts/circleci/test_entry_point.sh +++ b/scripts/circleci/test_entry_point.sh @@ -30,6 +30,7 @@ if [ $((2 % CIRCLE_NODE_TOTAL)) -eq "$CIRCLE_NODE_INDEX" ]; then COMMANDS_TO_RUN+=('./scripts/circleci/build.sh') COMMANDS_TO_RUN+=('yarn test-build --maxWorkers=2') COMMANDS_TO_RUN+=('yarn test-build-prod --maxWorkers=2') + COMMANDS_TO_RUN+=('yarn test-dom-fixture') COMMANDS_TO_RUN+=('cp ./scripts/rollup/results.json ./build/bundle-sizes.json') COMMANDS_TO_RUN+=('node ./scripts/tasks/danger') COMMANDS_TO_RUN+=('./scripts/circleci/upload_build.sh')