Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
Merged
Changes from 5 commits
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -164,6 +164,7 @@ jobs:
- *restore_yarn_cache
- *run_yarn
- run: yarn test-build --maxWorkers=2
- run: yarn test-dom-fixture

test_fuzz:
docker: *docker
@@ -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"
@@ -1,45 +1,44 @@
<!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='scheduler-unstable_mock.development.js'></script>
<script src='react.development.js'></script>
<script type="text/javascript">
window.React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Scheduler = window.SchedulerMock
</script>
<script src='react-dom.development.js'></script>
<script src='react-dom-test-utils.development.js'></script>
<script>
async function run(){
<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="scheduler-unstable_mock.development.js"></script>
<script src="react.development.js"></script>
<script type="text/javascript">
window.React.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Scheduler =
window.SchedulerMock;
</script>
<script src="react-dom.development.js"></script>
<script src="react-dom-test-utils.development.js"></script>
<script>
// from ReactTestUtilsAct-test.js
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>
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();
</script>
</body>
</html>
@@ -0,0 +1,107 @@
/**
* 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
*/

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(<App />);
});
expect(spy).not.toHaveBeenCalled();
});

it("doesn't warn when you use the right act + renderer: test", () => {
TestRenderer.act(() => {
TestRenderer.create(<App />);
});
expect(spy).not.toHaveBeenCalled();
});

it('works with createRoot().render combo', () => {
const root = ReactDOM.unstable_createRoot(document.createElement('div'));
TestRenderer.act(() => {
root.render(<App />);
});
confirmWarning();
});

it('warns when using the wrong act version - test + dom: render', () => {
TestRenderer.act(() => {
TestUtils.renderIntoDocument(<App />);
});
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(<Counter />);
TestRenderer.act(() => {
setCtr(1);
});
confirmWarning();
});

it('warns when using the wrong act version - dom + test: .create()', () => {
TestUtils.act(() => {
TestRenderer.create(<App />);
});
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(<App key="one" />);
});
TestUtils.act(() => {
root.update(<App key="two" />);
});
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(<Counter />);
TestUtils.act(() => {
setCtr(1);
});
confirmWarning();
});
@@ -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",
This conversation was marked as resolved by threepointone

This comment has been minimized.

Copy link
@acdlite

acdlite May 29, 2019

Member

Create a CI job for this please. Most people will not remember to run it.

This comment has been minimized.

This comment has been minimized.

Copy link
@acdlite

acdlite May 29, 2019

Member

Oh you're right. Missed it!

"flow": "node ./scripts/tasks/flow.js",
"flow-ci": "node ./scripts/tasks/flow-ci.js",
"prettier": "node ./scripts/prettier/index.js write-changed",
@@ -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,17 +85,19 @@ let actingUpdatesScopeDepth = 0;

function act(callback: () => Thenable) {
let previousActingUpdatesScopeDepth = actingUpdatesScopeDepth;
let previousActingUpdatesSigil;
actingUpdatesScopeDepth++;
// we use the function flushPassiveEffects directly as the sigil,
// since it's unique to a renderer
if (__DEV__) {
ReactShouldWarnActingUpdates.current = true;
previousActingUpdatesSigil = ReactActingRendererSigil.current;
ReactActingRendererSigil.current = flushPassiveEffects;
This conversation was marked as resolved by threepointone

This comment has been minimized.

Copy link
@acdlite

acdlite May 29, 2019

Member

Could you use a separate export instead? I find this unnecessarily confusing for no benefit, given that it's dev-only.

}

function onDone() {
actingUpdatesScopeDepth--;
if (__DEV__) {
if (actingUpdatesScopeDepth === 0) {
This conversation was marked as resolved by threepointone

This comment has been minimized.

Copy link
@acdlite

acdlite May 29, 2019

Member

I don't get how you were able to remove this depth check. Can you explain?

This comment has been minimized.

Copy link
@threepointone

threepointone May 29, 2019

Author Contributor

ReactActingRendererSigil.current starts with a .current value of null. Inside warnIfNotCurrentlyActingUpdatesInDev, we check whether .current === flushPassiveEffects)
As the act() scope opens, it sets the value to flushPassiveEffects which isn't null, and while exiting scopes sets it to whatever the previous one was, until the last scope, when previousActingUpdatesSigil gets set as null, triggering the warning.

We could have had the same pattern when the sigil was just a boolean, by saving .current as a previousActingSigilBoolean or something.

This comment has been minimized.

Copy link
@acdlite

acdlite May 29, 2019

Member

Makes sense

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(
@@ -81,7 +81,7 @@ type TextInstance = {|
|};
type HostContext = Object;

const {ReactShouldWarnActingUpdates} = ReactSharedInternals;
const {ReactActingRendererSigil} = ReactSharedInternals;

const NO_CONTEXT = {};
const UPPERCASE_CONTEXT = {};
@@ -698,17 +698,19 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {

function act(callback: () => Thenable) {
let previousActingUpdatesScopeDepth = actingUpdatesScopeDepth;
let previousActingUpdatesSigil;
actingUpdatesScopeDepth++;
// we use the function flushPassiveEffects directly as the sigil,
// since it's unique to a renderer
if (__DEV__) {
ReactShouldWarnActingUpdates.current = true;
previousActingUpdatesSigil = ReactActingRendererSigil.current;
ReactActingRendererSigil.current = flushPassiveEffects;
}

function onDone() {
actingUpdatesScopeDepth--;
if (__DEV__) {
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(
@@ -35,6 +35,7 @@ import {
flushPassiveEffects,
requestCurrentTime,
warnIfNotCurrentlyActingUpdatesInDev,
warnIfNotScopedWithMatchingAct,
markRenderEventTimeAndConfig,
} from './ReactFiberWorkLoop';

@@ -1207,10 +1208,9 @@ function dispatchAction<S, A>(
}
}
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.
// $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests
if ('undefined' !== typeof jest) {
warnIfNotScopedWithMatchingAct(fiber);
warnIfNotCurrentlyActingUpdatesInDev(fiber);
}
}
@@ -56,6 +56,7 @@ import {
discreteUpdates,
flushDiscreteUpdates,
flushPassiveEffects,
warnIfNotScopedWithMatchingAct,
} from './ReactFiberWorkLoop';
import {createUpdate, enqueueUpdate} from './ReactUpdateQueue';
import ReactFiberInstrumentation from './ReactFiberInstrumentation';
@@ -303,6 +304,12 @@ export function updateContainer(
): ExpirationTime {
const current = container.current;
const currentTime = requestCurrentTime();
if (__DEV__) {
// $FlowExpectedError - jest isn't a global, and isn't recognized outside of tests
if ('undefined' !== typeof jest) {
warnIfNotScopedWithMatchingAct(current);
}
}
const suspenseConfig = requestCurrentSuspenseConfig();
const expirationTime = computeExpirationForFiber(
currentTime,
@@ -173,7 +173,7 @@ const ceil = Math.ceil;
const {
ReactCurrentDispatcher,
ReactCurrentOwner,
ReactShouldWarnActingUpdates,
ReactActingRendererSigil,
} = ReactSharedInternals;

type WorkPhase = 0 | 1 | 2 | 3 | 4 | 5 | 6;
@@ -2276,11 +2276,40 @@ function warnAboutInvalidUpdatesOnClassComponentsInDEV(fiber) {
}
}

export function warnIfNotScopedWithMatchingAct(fiber: Fiber): void {
if (__DEV__) {
if (
ReactActingRendererSigil.current !== null &&
// use the function flushPassiveEffects directly as the sigil

This comment has been minimized.

Copy link
@gaearon

gaearon May 29, 2019

Member

Outdated comment

// so this comparison is expected here
ReactActingRendererSigil.current !== flushPassiveEffects
) {
// it looks like we're using the wrong matching act(), so log a warning

This comment has been minimized.

Copy link
@gaearon

gaearon May 29, 2019

Member

This comment kinda repeats the 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" +

This comment has been minimized.

Copy link
@threepointone

threepointone May 29, 2019

Author Contributor

there's no such package either 🤦‍♂️

'//...\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),
);
}
}
}

function warnIfNotCurrentlyActingUpdatesInDEV(fiber: Fiber): void {
if (__DEV__) {
if (
workPhase === NotWorking &&
ReactShouldWarnActingUpdates.current === false
ReactActingRendererSigil.current !== flushPassiveEffects
) {
warningWithoutStack(
false,
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.