Skip to content

Commit

Permalink
warn when using the wrong act() around create/updates
Browse files Browse the repository at this point in the history
via facebook#15319
This solves 2 specific problems -
- using the 'wrong' act() doesn't silence the warning
- using the wrong act logs a warning

It does this by using an empty object on the reconciler as the identity of ReactShouldWarnActingUpdates.current. We also add this check when calling createContainer() to catch the common failure of this happening right in the beginning.
  • Loading branch information
Sunil Pai committed Apr 12, 2019
1 parent a09bd0f commit 6aac3a2
Show file tree
Hide file tree
Showing 17 changed files with 182 additions and 53 deletions.
4 changes: 4 additions & 0 deletions fixtures/dom/.gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ public/react-dom-server.browser.development.js
public/react-dom-server.browser.production.min.js
public/react-dom-test-utils.development.js
public/react-dom-test-utils.production.min.js
public/react-test-renderer.development.js
public/react-test-renderer.production.min.js
public/scheduler.development.js
public/scheduler.production.min.js

# misc
.DS_Store
Expand Down
2 changes: 1 addition & 1 deletion fixtures/dom/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
},
"scripts": {
"start": "react-scripts start",
"prestart": "cp ../../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/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 ../../build/node_modules/react-test-renderer/umd/react-test-renderer.development.js ../../build/node_modules/react-test-renderer/umd/react-test-renderer.production.min.js ../../build/node_modules/scheduler/umd/scheduler.development.js ../../build/node_modules/scheduler/umd/scheduler.production.min.js public/",
"build": "react-scripts build && cp build/index.html build/200.html",
"test": "react-scripts test --env=jsdom",
"eject": "react-scripts eject"
Expand Down
77 changes: 47 additions & 30 deletions fixtures/dom/public/act-dom.html
Original file line number Diff line number Diff line change
@@ -1,41 +1,58 @@
<!DOCTYPE html>
<html>
<head>
<title>sanity test for ReactTestUtils.act</title>
</head>
<body>
this page tests whether act runs properly in a browser.
<br/>
your console should say "5"
<script src='react.development.js'></script>
<script src='react-dom.development.js'></script>
<script src='react-dom-test-utils.development.js'></script>
<script>
async function run(){
// from ReactTestUtilsAct-test.js
<head>
<title>sanity test for ReactTestUtils.act</title>
</head>
<body>
this page tests whether act runs properly in a browser.
<br />
your console should say "5", and a warning about not using the right act()
<script src="react.development.js"></script>
<script src="react-dom.development.js"></script>
<script src="react-dom-test-utils.development.js"></script>
<script src="scheduler.development.js"></script>
<script src="react-test-renderer.development.js"></script>
<script>
window.jest = {}; // to enable warnings
function App() {
let [state, setState] = React.useState(0);
async function ticker() {
await null;
setState(x => x + 1);
}
React.useEffect(
() => {
ticker();
},
[Math.min(state, 4)],
);
React.useEffect(() => {
ticker();
}, [Math.min(state, 4)]);
return state;
}
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);
}
run();

</script>
</body>

function sleep(period){
return new Promise(resolve => setTimeout(resolve, period))
}

async function testDOMAsynAct() {
// from ReactTestUtilsAct-test.js

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);
}

async function testMixRenderers() {
await ReactTestUtils.act(async () => {
ReactTestRenderer.create(React.createElement(App));
});
}

async function run() {
await testDOMAsynAct();
await testMixRenderers();
}

run();
</script>
</body>
</html>
2 changes: 2 additions & 0 deletions packages/react-dom/src/client/ReactDOM.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ import {
findHostInstance,
findHostInstanceWithWarning,
flushPassiveEffects,
ReactActingUpdatesSigil,
} from 'react-reconciler/inline.dom';
import {createPortal as createPortalImpl} from 'shared/ReactPortal';
import {canUseDOM} from 'shared/ExecutionEnvironment';
Expand Down Expand Up @@ -822,6 +823,7 @@ const ReactDOM: Object = {
dispatchEvent,
runEventsInBatch,
flushPassiveEffects,
ReactActingUpdatesSigil,
],
},
};
Expand Down
2 changes: 2 additions & 0 deletions packages/react-dom/src/fire/ReactFire.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ import {
findHostInstance,
findHostInstanceWithWarning,
flushPassiveEffects,
ReactActingUpdatesSigil,
} from 'react-reconciler/inline.fire';
import {createPortal as createPortalImpl} from 'shared/ReactPortal';
import {canUseDOM} from 'shared/ExecutionEnvironment';
Expand Down Expand Up @@ -828,6 +829,7 @@ const ReactDOM: Object = {
dispatchEvent,
runEventsInBatch,
flushPassiveEffects,
ReactActingUpdatesSigil,
],
},
};
Expand Down
4 changes: 3 additions & 1 deletion packages/react-dom/src/test-utils/ReactTestUtils.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,10 @@ const [
restoreStateIfNeeded,
dispatchEvent,
runEventsInBatch,
// eslint-disable-next-line no-unused-vars
/* eslint-disable no-unused-vars */
flushPassiveEffects,
ReactActingUpdatesSigil,
/* eslint-enable no-unused-vars */
] = ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Events;

function Event(suffix) {}
Expand Down
9 changes: 5 additions & 4 deletions packages/react-dom/src/test-utils/ReactTestUtilsAct.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const [
runEventsInBatch,
/* eslint-enable no-unused-vars */
flushPassiveEffects,
ReactActingUpdatesSigil,
] = ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Events;

const batchedUpdates = ReactDOM.unstable_batchedUpdates;
Expand Down Expand Up @@ -61,18 +62,18 @@ function flushEffectsAndMicroTasks(onDone: (err: ?Error) => void) {

function act(callback: () => Thenable) {
let previousActingUpdatesScopeDepth;
let previousActingUpdatesSigil;
if (__DEV__) {
previousActingUpdatesScopeDepth = actingUpdatesScopeDepth;
previousActingUpdatesSigil = ReactShouldWarnActingUpdates.current;
actingUpdatesScopeDepth++;
ReactShouldWarnActingUpdates.current = true;
ReactShouldWarnActingUpdates.current = ReactActingUpdatesSigil;
}

function onDone() {
if (__DEV__) {
actingUpdatesScopeDepth--;
if (actingUpdatesScopeDepth === 0) {
ReactShouldWarnActingUpdates.current = false;
}
ReactShouldWarnActingUpdates.current = previousActingUpdatesSigil;
if (actingUpdatesScopeDepth > previousActingUpdatesScopeDepth) {
// if it's _less than_ previousActingUpdatesScopeDepth, then we can assume the 'other' one has warned
warningWithoutStack(
Expand Down
16 changes: 11 additions & 5 deletions packages/react-noop-renderer/src/createReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -597,11 +597,17 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
const roots = new Map();
const DEFAULT_ROOT_ID = '<default>';

const {flushPassiveEffects, batchedUpdates} = NoopRenderer;
const {
flushPassiveEffects,
batchedUpdates,
ReactActingUpdatesSigil,
} = NoopRenderer;

// this act() implementation should be exactly the same in
// ReactTestUtilsAct.js, ReactTestRendererAct.js, createReactNoop.js

// we track the 'depth' of the act() calls with this counter,
// so we can tell if any async act() calls try to run in parallel.
let actingUpdatesScopeDepth = 0;

function flushEffectsAndMicroTasks(onDone: (err: ?Error) => void) {
Expand All @@ -621,18 +627,18 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {

function act(callback: () => Thenable) {
let previousActingUpdatesScopeDepth;
let previousActingUpdatesSigil;
if (__DEV__) {
previousActingUpdatesScopeDepth = actingUpdatesScopeDepth;
previousActingUpdatesSigil = ReactShouldWarnActingUpdates.current;
actingUpdatesScopeDepth++;
ReactShouldWarnActingUpdates.current = true;
ReactShouldWarnActingUpdates.current = ReactActingUpdatesSigil;
}

function onDone() {
if (__DEV__) {
actingUpdatesScopeDepth--;
if (actingUpdatesScopeDepth === 0) {
ReactShouldWarnActingUpdates.current = false;
}
ReactShouldWarnActingUpdates.current = previousActingUpdatesSigil;
if (actingUpdatesScopeDepth > previousActingUpdatesScopeDepth) {
// if it's _less than_ previousActingUpdatesScopeDepth, then we can assume the 'other' one has warned
warningWithoutStack(
Expand Down
10 changes: 10 additions & 0 deletions packages/react-reconciler/src/ReactActingUpdatesSigil.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
/**
* 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
*/

export default {};
2 changes: 2 additions & 0 deletions packages/react-reconciler/src/ReactFiberHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import {
flushPassiveEffects,
requestCurrentTime,
warnIfNotCurrentlyActingUpdatesInDev,
warnIfNotScopedWithMatchingAct,
} from './ReactFiberScheduler';

import invariant from 'shared/invariant';
Expand Down Expand Up @@ -1169,6 +1170,7 @@ function dispatchAction<S, A>(
// 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);
}
}
Expand Down
16 changes: 14 additions & 2 deletions packages/react-reconciler/src/ReactFiberReconciler.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import type {
} from './ReactFiberHostConfig';
import type {ReactNodeList} from 'shared/ReactTypes';
import type {ExpirationTime} from './ReactFiberExpirationTime';
import ReactActingUpdatesSigil from './ReactActingUpdatesSigil';

import {
findCurrentHostFiber,
Expand Down Expand Up @@ -53,6 +54,7 @@ import {
interactiveUpdates,
flushInteractiveUpdates,
flushPassiveEffects,
warnIfNotScopedWithMatchingAct,
} from './ReactFiberScheduler';
import {createUpdate, enqueueUpdate} from './ReactUpdateQueue';
import ReactFiberInstrumentation from './ReactFiberInstrumentation';
Expand Down Expand Up @@ -275,8 +277,15 @@ export function createContainer(
containerInfo: Container,
isConcurrent: boolean,
hydrate: boolean,
): OpaqueRoot {
return createFiberRoot(containerInfo, isConcurrent, hydrate);
): OpaqueRoot {
const fiberRoot = createFiberRoot(containerInfo, isConcurrent, hydrate);
// 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(fiberRoot.current);
}
return fiberRoot;
}

export function updateContainer(
Expand Down Expand Up @@ -309,6 +318,7 @@ export {
flushControlled,
flushSync,
flushPassiveEffects,
warnIfNotScopedWithMatchingAct,
};

export function getPublicRootInstance(
Expand Down Expand Up @@ -436,3 +446,5 @@ export function injectIntoDevTools(devToolsConfig: DevToolsConfig): boolean {
},
});
}

export {ReactActingUpdatesSigil};
5 changes: 5 additions & 0 deletions packages/react-reconciler/src/ReactFiberScheduler.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
flushInteractiveUpdates as flushInteractiveUpdates_old,
computeUniqueAsyncExpiration as computeUniqueAsyncExpiration_old,
flushPassiveEffects as flushPassiveEffects_old,
warnIfNotScopedWithMatchingAct as warnIfNotScopedWithMatchingAct_old,
warnIfNotCurrentlyActingUpdatesInDev as warnIfNotCurrentlyActingUpdatesInDev_old,
inferStartTimeFromExpirationTime as inferStartTimeFromExpirationTime_old,
} from './ReactFiberScheduler.old';
Expand Down Expand Up @@ -61,6 +62,7 @@ import {
flushInteractiveUpdates as flushInteractiveUpdates_new,
computeUniqueAsyncExpiration as computeUniqueAsyncExpiration_new,
flushPassiveEffects as flushPassiveEffects_new,
warnIfNotScopedWithMatchingAct as warnIfNotScopedWithMatchingAct_new,
warnIfNotCurrentlyActingUpdatesInDev as warnIfNotCurrentlyActingUpdatesInDev_new,
inferStartTimeFromExpirationTime as inferStartTimeFromExpirationTime_new,
} from './ReactFiberScheduler.new';
Expand Down Expand Up @@ -130,6 +132,9 @@ export const computeUniqueAsyncExpiration = enableNewScheduler
export const flushPassiveEffects = enableNewScheduler
? flushPassiveEffects_new
: flushPassiveEffects_old;
export const warnIfNotScopedWithMatchingAct = enableNewScheduler
? warnIfNotScopedWithMatchingAct_new
: warnIfNotScopedWithMatchingAct_old;
export const warnIfNotCurrentlyActingUpdatesInDev = enableNewScheduler
? warnIfNotCurrentlyActingUpdatesInDev_new
: warnIfNotCurrentlyActingUpdatesInDev_old;
Expand Down
30 changes: 29 additions & 1 deletion packages/react-reconciler/src/ReactFiberScheduler.new.js
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ import {
clearCaughtError,
} from 'shared/ReactErrorUtils';
import {onCommitRoot} from './ReactFiberDevToolsHook';
import ReactActingUpdatesSigil from './ReactActingUpdatesSigil';

const {
ReactCurrentDispatcher,
Expand Down Expand Up @@ -2029,11 +2030,38 @@ function warnAboutInvalidUpdatesOnClassComponentsInDEV(fiber) {
}
}

export function warnIfNotScopedWithMatchingAct(fiber: Fiber): void {
if (__DEV__) {
if (
ReactShouldWarnActingUpdates.current !== null &&
ReactShouldWarnActingUpdates.current !== ReactActingUpdatesSigil
) {
// 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 interactions.\n" +
'Be sure to use the matching version of act() corresponding to your renderer. e.g. -\n' +
"for react-dom, import {act} from 'react-test-utils';\n" +
'for react-test-renderer, const {act} = TestRenderer.' +
'%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
ReactShouldWarnActingUpdates.current !== ReactActingUpdatesSigil
) {
warningWithoutStack(
false,
Expand Down
Loading

0 comments on commit 6aac3a2

Please sign in to comment.