@samitouri / QOS-React-2 / commits / 9aad17d60c

using the wrong renderer's act() should warn (#15756)

* warn when using the wrong renderer's act around another renderer's updates like it says. it uses a real object as the sigil (instead of just a boolean). specifically, it uses a renderer's flushPassiveEffects as the sigil. We also run tests for this separate from our main suite (which doesn't allow loading multiple renderers in a suite), but makes sure to run this in CI as well. * unneeded (and wrong) comment * run the dom fixture on CI * update the sigil only in __DEV__ * remove the obnoxious comment * use an explicit export for the sigil

Sunil Pai committed May 29, 2019 at 22:56 UTC 9aad17d60cd8453357ec36e8180b61aaaf313b7e
16 files changed +241 -58
.circleci/config.yml
+1
@@ -178,6 +178,7 @@ jobs:
178 - *restore_yarn_cache
179 - *run_yarn
180 - run: yarn test-build --maxWorkers=2
181 + - run: yarn test-dom-fixture
182
183 test_fuzz:
184 docker: *docker
fixtures/dom/package.json
+1 -1
@@ -18,7 +18,7 @@
18 },
19 "scripts": {
20 "start": "react-scripts start",
21 - "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/",
21 + "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",
22 "build": "react-scripts build && cp build/index.html build/200.html",
23 "test": "react-scripts test --env=jsdom",
24 "eject": "react-scripts eject"
fixtures/dom/public/act-dom.html
+32 -33
@@ -1,21 +1,21 @@
1 <!DOCTYPE html>
2 <html>
3 -<head>
4 - <title>sanity test for ReactTestUtils.act</title>
5 -</head>
6 -<body>
7 - this page tests whether act runs properly in a browser.
8 - <br/>
9 - your console should say "5"
10 - <script src='scheduler-unstable_mock.development.js'></script>
11 - <script src='react.development.js'></script>
12 - <script type="text/javascript">
13 - window.React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Scheduler = window.SchedulerMock
14 - </script>
15 - <script src='react-dom.development.js'></script>
16 - <script src='react-dom-test-utils.development.js'></script>
17 - <script>
18 - async function run(){
3 + <head>
4 + <title>sanity test for ReactTestUtils.act</title>
5 + </head>
6 + <body>
7 + this page tests whether act runs properly in a browser.
8 + <br />
9 + your console should say "5"
10 + <script src="scheduler-unstable_mock.development.js"></script>
11 + <script src="react.development.js"></script>
12 + <script type="text/javascript">
13 + window.React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Scheduler =
14 + window.SchedulerMock;
15 + </script>
16 + <script src="react-dom.development.js"></script>
17 + <script src="react-dom-test-utils.development.js"></script>
18 + <script>
19 // from ReactTestUtilsAct-test.js
20 function App() {
21 let [state, setState] = React.useState(0);
@@ -23,23 +23,22 @@
23 await null;
24 setState(x => x + 1);
25 }
26 - React.useEffect(
27 - () => {
28 - ticker();
29 - },
30 - [Math.min(state, 4)],
31 - );
26 + React.useEffect(() => {
27 + ticker();
28 + }, [Math.min(state, 4)]);
29 return state;
30 }
34 - const el = document.createElement('div');
35 - await ReactTestUtils.act(async () => {
36 - ReactDOM.render(React.createElement(App), el);
37 - });
38 - // all 5 ticks present and accounted for
39 - console.log(el.innerHTML);
40 - }
41 - run();
42 -
43 - </script>
44 -</body>
31 +
32 + async function testAsyncAct() {
33 + const el = document.createElement("div");
34 + await ReactTestUtils.act(async () => {
35 + ReactDOM.render(React.createElement(App), el);
36 + });
37 + // all 5 ticks present and accounted for
38 + console.log(el.innerHTML);
39 + }
40 +
41 + testAsyncAct();
42 + </script>
43 + </body>
44 </html>
fixtures/dom/src/index.test.js new
+107
@@ -0,0 +1,107 @@
1 +/**
2 + * Copyright (c) Facebook, Inc. and its affiliates.
3 + *
4 + * This source code is licensed under the MIT license found in the
5 + * LICENSE file in the root directory of this source tree.
6 + *
7 + * @emails react-core
8 + */
9 +
10 +import React from 'react';
11 +import ReactDOM from 'react-dom';
12 +import TestUtils from 'react-dom/test-utils';
13 +import TestRenderer from 'react-test-renderer';
14 +
15 +let spy;
16 +beforeEach(() => {
17 + spy = jest.spyOn(console, 'error').mockImplementation(() => {});
18 +});
19 +
20 +function confirmWarning() {
21 + expect(spy).toHaveBeenCalledWith(
22 + expect.stringContaining(
23 + "It looks like you're using the wrong act() around your test interactions."
24 + ),
25 + ''
26 + );
27 +}
28 +
29 +function App(props) {
30 + return 'hello world';
31 +}
32 +
33 +it("doesn't warn when you use the right act + renderer: dom", () => {
34 + TestUtils.act(() => {
35 + TestUtils.renderIntoDocument(<App />);
36 + });
37 + expect(spy).not.toHaveBeenCalled();
38 +});
39 +
40 +it("doesn't warn when you use the right act + renderer: test", () => {
41 + TestRenderer.act(() => {
42 + TestRenderer.create(<App />);
43 + });
44 + expect(spy).not.toHaveBeenCalled();
45 +});
46 +
47 +it('works with createRoot().render combo', () => {
48 + const root = ReactDOM.unstable_createRoot(document.createElement('div'));
49 + TestRenderer.act(() => {
50 + root.render(<App />);
51 + });
52 + confirmWarning();
53 +});
54 +
55 +it('warns when using the wrong act version - test + dom: render', () => {
56 + TestRenderer.act(() => {
57 + TestUtils.renderIntoDocument(<App />);
58 + });
59 + confirmWarning();
60 +});
61 +
62 +it('warns when using the wrong act version - test + dom: updates', () => {
63 + let setCtr;
64 + function Counter(props) {
65 + const [ctr, _setCtr] = React.useState(0);
66 + setCtr = _setCtr;
67 + return ctr;
68 + }
69 + TestUtils.renderIntoDocument(<Counter />);
70 + TestRenderer.act(() => {
71 + setCtr(1);
72 + });
73 + confirmWarning();
74 +});
75 +
76 +it('warns when using the wrong act version - dom + test: .create()', () => {
77 + TestUtils.act(() => {
78 + TestRenderer.create(<App />);
79 + });
80 + confirmWarning();
81 +});
82 +
83 +it('warns when using the wrong act version - dom + test: .update()', () => {
84 + let root;
85 + // use the right one here so we don't get the first warning
86 + TestRenderer.act(() => {
87 + root = TestRenderer.create(<App key="one" />);
88 + });
89 + TestUtils.act(() => {
90 + root.update(<App key="two" />);
91 + });
92 + confirmWarning();
93 +});
94 +
95 +it('warns when using the wrong act version - dom + test: updates', () => {
96 + let setCtr;
97 + function Counter(props) {
98 + const [ctr, _setCtr] = React.useState(0);
99 + setCtr = _setCtr;
100 + return ctr;
101 + }
102 + const root = TestRenderer.create(<Counter />);
103 + TestUtils.act(() => {
104 + setCtr(1);
105 + });
106 + confirmWarning();
107 +});
package.json
+1
@@ -108,6 +108,7 @@
108 "test-prod-build": "yarn test-build-prod",
109 "test-build": "cross-env NODE_ENV=development jest --config ./scripts/jest/config.build.js",
110 "test-build-prod": "cross-env NODE_ENV=production jest --config ./scripts/jest/config.build.js",
111 + "test-dom-fixture": "cd fixtures/dom && yarn && yarn prestart && yarn test",
112 "flow": "node ./scripts/tasks/flow.js",
113 "flow-ci": "node ./scripts/tasks/flow-ci.js",
114 "prettier": "node ./scripts/prettier/index.js write-changed",
packages/react-dom/src/client/ReactDOM.js
+2
@@ -38,6 +38,7 @@ import {
38 findHostInstance,
39 findHostInstanceWithWarning,
40 flushPassiveEffects,
41 + ReactActingRendererSigil,
42 } from 'react-reconciler/inline.dom';
43 import {createPortal as createPortalImpl} from 'shared/ReactPortal';
44 import {canUseDOM} from 'shared/ExecutionEnvironment';
@@ -816,6 +817,7 @@ const ReactDOM: Object = {
817 dispatchEvent,
818 runEventsInBatch,
819 flushPassiveEffects,
820 + ReactActingRendererSigil,
821 ],
822 },
823 };
packages/react-dom/src/fire/ReactFire.js
+2
@@ -43,6 +43,7 @@ import {
43 findHostInstance,
44 findHostInstanceWithWarning,
45 flushPassiveEffects,
46 + ReactActingRendererSigil,
47 } from 'react-reconciler/inline.fire';
48 import {createPortal as createPortalImpl} from 'shared/ReactPortal';
49 import {canUseDOM} from 'shared/ExecutionEnvironment';
@@ -822,6 +823,7 @@ const ReactDOM: Object = {
823 dispatchEvent,
824 runEventsInBatch,
825 flushPassiveEffects,
826 + ReactActingRendererSigil,
827 ],
828 },
829 };
packages/react-dom/src/test-utils/ReactTestUtils.js
+3 -1
@@ -42,8 +42,10 @@ const [
42 restoreStateIfNeeded,
43 dispatchEvent,
44 runEventsInBatch,
45 - // eslint-disable-next-line no-unused-vars
45 + /* eslint-disable no-unused-vars */
46 flushPassiveEffects,
47 + ReactActingRendererSigil,
48 + /* eslint-enable no-unused-vars */
49 ] = ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Events;
50
51 function Event(suffix) {}
packages/react-dom/src/test-utils/ReactTestUtilsAct.js
+6 -5
@@ -33,11 +33,12 @@ const [
33 runEventsInBatch,
34 /* eslint-enable no-unused-vars */
35 flushPassiveEffects,
36 + ReactActingRendererSigil,
37 ] = ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Events;
38
39 const batchedUpdates = ReactDOM.unstable_batchedUpdates;
40
40 -const {ReactShouldWarnActingUpdates} = ReactSharedInternals;
41 +const {ReactCurrentActingRendererSigil} = ReactSharedInternals;
42
43 // this implementation should be exactly the same in
44 // ReactTestUtilsAct.js, ReactTestRendererAct.js, createReactNoop.js
@@ -85,17 +86,17 @@ let actingUpdatesScopeDepth = 0;
86
87 function act(callback: () => Thenable) {
88 let previousActingUpdatesScopeDepth = actingUpdatesScopeDepth;
89 + let previousActingUpdatesSigil;
90 actingUpdatesScopeDepth++;
91 if (__DEV__) {
90 - ReactShouldWarnActingUpdates.current = true;
92 + previousActingUpdatesSigil = ReactCurrentActingRendererSigil.current;
93 + ReactCurrentActingRendererSigil.current = ReactActingRendererSigil;
94 }
95
96 function onDone() {
97 actingUpdatesScopeDepth--;
98 if (__DEV__) {
96 - if (actingUpdatesScopeDepth === 0) {
97 - ReactShouldWarnActingUpdates.current = false;
98 - }
99 + ReactCurrentActingRendererSigil.current = previousActingUpdatesSigil;
100 if (actingUpdatesScopeDepth > previousActingUpdatesScopeDepth) {
101 // if it's _less than_ previousActingUpdatesScopeDepth, then we can assume the 'other' one has warned
102 warningWithoutStack(
packages/react-noop-renderer/src/createReactNoop.js
+10 -6
@@ -81,7 +81,7 @@ type TextInstance = {|
81 |};
82 type HostContext = Object;
83
84 -const {ReactShouldWarnActingUpdates} = ReactSharedInternals;
84 +const {ReactCurrentActingRendererSigil} = ReactSharedInternals;
85
86 const NO_CONTEXT = {};
87 const UPPERCASE_CONTEXT = {};
@@ -650,7 +650,11 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
650 const roots = new Map();
651 const DEFAULT_ROOT_ID = '<default>';
652
653 - const {flushPassiveEffects, batchedUpdates} = NoopRenderer;
653 + const {
654 + flushPassiveEffects,
655 + batchedUpdates,
656 + ReactActingRendererSigil,
657 + } = NoopRenderer;
658
659 // this act() implementation should be exactly the same in
660 // ReactTestUtilsAct.js, ReactTestRendererAct.js, createReactNoop.js
@@ -698,17 +702,17 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
702
703 function act(callback: () => Thenable) {
704 let previousActingUpdatesScopeDepth = actingUpdatesScopeDepth;
705 + let previousActingUpdatesSigil;
706 actingUpdatesScopeDepth++;
707 if (__DEV__) {
703 - ReactShouldWarnActingUpdates.current = true;
708 + previousActingUpdatesSigil = ReactCurrentActingRendererSigil.current;
709 + ReactCurrentActingRendererSigil.current = ReactActingRendererSigil;
710 }
711
712 function onDone() {
713 actingUpdatesScopeDepth--;
714 if (__DEV__) {
709 - if (actingUpdatesScopeDepth === 0) {
710 - ReactShouldWarnActingUpdates.current = false;
711 - }
715 + ReactCurrentActingRendererSigil.current = previousActingUpdatesSigil;
716 if (actingUpdatesScopeDepth > previousActingUpdatesScopeDepth) {
717 // if it's _less than_ previousActingUpdatesScopeDepth, then we can assume the 'other' one has warned
718 warningWithoutStack(
packages/react-reconciler/src/ReactFiberHooks.js
+3 -3
@@ -35,6 +35,7 @@ import {
35 flushPassiveEffects,
36 requestCurrentTime,
37 warnIfNotCurrentlyActingUpdatesInDev,
38 + warnIfNotScopedWithMatchingAct,
39 markRenderEventTimeAndConfig,
40 } from './ReactFiberWorkLoop';
41
@@ -1207,10 +1208,9 @@ function dispatchAction<S, A>(
1208 }
1209 }
1210 if (__DEV__) {
1210 - // jest isn't a 'global', it's just exposed to tests via a wrapped function
1211 - // further, this isn't a test file, so flow doesn't recognize the symbol. So...
1212 - // $FlowExpectedError - because requirements don't give a damn about your type sigs.
1211 + // $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests
1212 if ('undefined' !== typeof jest) {
1213 + warnIfNotScopedWithMatchingAct(fiber);
1214 warnIfNotCurrentlyActingUpdatesInDev(fiber);
1215 }
1216 }
packages/react-reconciler/src/ReactFiberReconciler.js
+9
@@ -56,6 +56,8 @@ import {
56 discreteUpdates,
57 flushDiscreteUpdates,
58 flushPassiveEffects,
59 + warnIfNotScopedWithMatchingAct,
60 + ReactActingRendererSigil,
61 } from './ReactFiberWorkLoop';
62 import {createUpdate, enqueueUpdate} from './ReactUpdateQueue';
63 import ReactFiberInstrumentation from './ReactFiberInstrumentation';
@@ -303,6 +305,12 @@ export function updateContainer(
305 ): ExpirationTime {
306 const current = container.current;
307 const currentTime = requestCurrentTime();
308 + if (__DEV__) {
309 + // $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests
310 + if ('undefined' !== typeof jest) {
311 + warnIfNotScopedWithMatchingAct(current);
312 + }
313 + }
314 const suspenseConfig = requestCurrentSuspenseConfig();
315 const expirationTime = computeExpirationForFiber(
316 currentTime,
@@ -332,6 +340,7 @@ export {
340 flushControlled,
341 flushSync,
342 flushPassiveEffects,
343 + ReactActingRendererSigil,
344 };
345
346 export function getPublicRootInstance(
packages/react-reconciler/src/ReactFiberWorkLoop.js
+37 -2
@@ -173,7 +173,7 @@ const ceil = Math.ceil;
173 const {
174 ReactCurrentDispatcher,
175 ReactCurrentOwner,
176 - ReactShouldWarnActingUpdates,
176 + ReactCurrentActingRendererSigil,
177 } = ReactSharedInternals;
178
179 type WorkPhase = 0 | 1 | 2 | 3 | 4 | 5 | 6;
@@ -2271,11 +2271,46 @@ function warnAboutInvalidUpdatesOnClassComponentsInDEV(fiber) {
2271 }
2272 }
2273
2274 +// We export a simple object here to be used by a renderer/test-utils
2275 +// as the value of ReactCurrentActingRendererSigil.current
2276 +// This identity lets us identify (ha!) when the wrong renderer's act()
2277 +// wraps anothers' updates/effects
2278 +export const ReactActingRendererSigil = {};
2279 +
2280 +export function warnIfNotScopedWithMatchingAct(fiber: Fiber): void {
2281 + if (__DEV__) {
2282 + if (
2283 + ReactCurrentActingRendererSigil.current !== null &&
2284 + // use the function flushPassiveEffects directly as the sigil
2285 + // so this comparison is expected here
2286 + ReactCurrentActingRendererSigil.current !== ReactActingRendererSigil
2287 + ) {
2288 + // it looks like we're using the wrong matching act(), so log a warning
2289 + warningWithoutStack(
2290 + false,
2291 + "It looks like you're using the wrong act() around your test interactions.\n" +
2292 + 'Be sure to use the matching version of act() corresponding to your renderer:\n\n' +
2293 + '// for react-dom:\n' +
2294 + "import {act} from 'react-test-utils';\n" +
2295 + '//...\n' +
2296 + 'act(() => ...);\n\n' +
2297 + '// for react-test-renderer:\n' +
2298 + "import TestRenderer from 'react-test-renderer';\n" +
2299 + 'const {act} = TestRenderer;\n' +
2300 + '//...\n' +
2301 + 'act(() => ...);' +
2302 + '%s',
2303 + getStackByFiberInDevAndProd(fiber),
2304 + );
2305 + }
2306 + }
2307 +}
2308 +
2309 function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void {
2310 if (__DEV__) {
2311 if (
2312 workPhase === NotWorking &&
2278 - ReactShouldWarnActingUpdates.current === false
2313 + ReactCurrentActingRendererSigil.current !== ReactActingRendererSigil
2314 ) {
2315 warningWithoutStack(
2316 false,
packages/react-test-renderer/src/ReactTestRendererAct.js
+6 -5
@@ -11,6 +11,7 @@ import type {Thenable} from 'react-reconciler/src/ReactFiberWorkLoop';
11 import {
12 batchedUpdates,
13 flushPassiveEffects,
14 + ReactActingRendererSigil,
15 } from 'react-reconciler/inline.test';
16 import ReactSharedInternals from 'shared/ReactSharedInternals';
17 import warningWithoutStack from 'shared/warningWithoutStack';
@@ -18,7 +19,7 @@ import {warnAboutMissingMockScheduler} from 'shared/ReactFeatureFlags';
19 import enqueueTask from 'shared/enqueueTask';
20 import * as Scheduler from 'scheduler';
21
21 -const {ReactShouldWarnActingUpdates} = ReactSharedInternals;
22 +const {ReactCurrentActingRendererSigil} = ReactSharedInternals;
23
24 // this implementation should be exactly the same in
25 // ReactTestUtilsAct.js, ReactTestRendererAct.js, createReactNoop.js
@@ -66,17 +67,17 @@ let actingUpdatesScopeDepth = 0;
67
68 function act(callback: () => Thenable) {
69 let previousActingUpdatesScopeDepth = actingUpdatesScopeDepth;
70 + let previousActingUpdatesSigil;
71 actingUpdatesScopeDepth++;
72 if (__DEV__) {
71 - ReactShouldWarnActingUpdates.current = true;
73 + previousActingUpdatesSigil = ReactCurrentActingRendererSigil.current;
74 + ReactCurrentActingRendererSigil.current = ReactActingRendererSigil;
75 }
76
77 function onDone() {
78 actingUpdatesScopeDepth--;
79 if (__DEV__) {
77 - if (actingUpdatesScopeDepth === 0) {
78 - ReactShouldWarnActingUpdates.current = false;
79 - }
80 + ReactCurrentActingRendererSigil.current = previousActingUpdatesSigil;
81 if (actingUpdatesScopeDepth > previousActingUpdatesScopeDepth) {
82 // if it's _less than_ previousActingUpdatesScopeDepth, then we can assume the 'other' one has warned
83 warningWithoutStack(
packages/react/src/ReactCurrentActingRendererSigil.js new
+19
@@ -0,0 +1,19 @@
1 +/**
2 + * Copyright (c) Facebook, Inc. and its affiliates.
3 + *
4 + * This source code is licensed under the MIT license found in the
5 + * LICENSE file in the root directory of this source tree.
6 + *
7 + * @flow
8 + */
9 +
10 +/**
11 + * Used by act() to track whether you're outside an act() scope.
12 + * We use a renderer's flushPassiveEffects as the sigil value
13 + * so we can track identity of the renderer.
14 + */
15 +
16 +const ReactCurrentActingRendererSigil = {
17 + current: (null: null | (() => boolean)),
18 +};
19 +export default ReactCurrentActingRendererSigil;
packages/react/src/ReactSharedInternals.js
+2 -2
@@ -10,13 +10,13 @@ import ReactCurrentDispatcher from './ReactCurrentDispatcher';
10 import ReactCurrentBatchConfig from './ReactCurrentBatchConfig';
11 import ReactCurrentOwner from './ReactCurrentOwner';
12 import ReactDebugCurrentFrame from './ReactDebugCurrentFrame';
13 +import ReactCurrentActingRendererSigil from './ReactCurrentActingRendererSigil';
14
15 const ReactSharedInternals = {
16 ReactCurrentDispatcher,
17 ReactCurrentBatchConfig,
18 ReactCurrentOwner,
18 - // used by act()
19 - ReactShouldWarnActingUpdates: {current: false},
19 + ReactCurrentActingRendererSigil,
20 // Used by renderers to avoid bundling object-assign twice in UMD bundles:
21 assign,
22 };