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 all 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",
@@ -38,6 +38,7 @@ import {
findHostInstance,
findHostInstanceWithWarning,
flushPassiveEffects,
ReactActingRendererSigil,
} from 'react-reconciler/inline.dom';
import {createPortal as createPortalImpl} from 'shared/ReactPortal';
import {canUseDOM} from 'shared/ExecutionEnvironment';
@@ -816,6 +817,7 @@ const ReactDOM: Object = {
dispatchEvent,
runEventsInBatch,
flushPassiveEffects,
ReactActingRendererSigil,
],
},
};
@@ -43,6 +43,7 @@ import {
findHostInstance,
findHostInstanceWithWarning,
flushPassiveEffects,
ReactActingRendererSigil,
} from 'react-reconciler/inline.fire';
import {createPortal as createPortalImpl} from 'shared/ReactPortal';
import {canUseDOM} from 'shared/ExecutionEnvironment';
@@ -822,6 +823,7 @@ const ReactDOM: Object = {
dispatchEvent,
runEventsInBatch,
flushPassiveEffects,
ReactActingRendererSigil,
],
},
};
@@ -42,8 +42,10 @@ const [
restoreStateIfNeeded,
dispatchEvent,
runEventsInBatch,
// eslint-disable-next-line no-unused-vars
/* eslint-disable no-unused-vars */
flushPassiveEffects,
ReactActingRendererSigil,
/* eslint-enable no-unused-vars */
] = ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Events;

function Event(suffix) {}
@@ -33,11 +33,12 @@ const [
runEventsInBatch,
/* eslint-enable no-unused-vars */
flushPassiveEffects,
ReactActingRendererSigil,
] = ReactDOM.__SECRET_INTERNALS_DO_NOT_USE_OR_YOU_WILL_BE_FIRED.Events;

const batchedUpdates = ReactDOM.unstable_batchedUpdates;

const {ReactShouldWarnActingUpdates} = ReactSharedInternals;
const {ReactCurrentActingRendererSigil} = ReactSharedInternals;

// this implementation should be exactly the same in
// ReactTestUtilsAct.js, ReactTestRendererAct.js, createReactNoop.js
@@ -85,17 +86,17 @@ let actingUpdatesScopeDepth = 0;

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

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;
}
ReactCurrentActingRendererSigil.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 {ReactCurrentActingRendererSigil} = ReactSharedInternals;

const NO_CONTEXT = {};
const UPPERCASE_CONTEXT = {};
@@ -650,7 +650,11 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
const roots = new Map();
const DEFAULT_ROOT_ID = '<default>';

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

// this act() implementation should be exactly the same in
// ReactTestUtilsAct.js, ReactTestRendererAct.js, createReactNoop.js
@@ -698,17 +702,17 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {

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

function onDone() {
actingUpdatesScopeDepth--;
if (__DEV__) {
if (actingUpdatesScopeDepth === 0) {
ReactShouldWarnActingUpdates.current = false;
}
ReactCurrentActingRendererSigil.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,8 @@ import {
discreteUpdates,
flushDiscreteUpdates,
flushPassiveEffects,
warnIfNotScopedWithMatchingAct,
ReactActingRendererSigil,
} from './ReactFiberWorkLoop';
import {createUpdate, enqueueUpdate} from './ReactUpdateQueue';
import ReactFiberInstrumentation from './ReactFiberInstrumentation';
@@ -303,6 +305,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,
@@ -332,6 +340,7 @@ export {
flushControlled,
flushSync,
flushPassiveEffects,
ReactActingRendererSigil,
};

export function getPublicRootInstance(
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.