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
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions .circleci/config.yml
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion fixtures/dom/package.json
Expand Up @@ -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"
Expand Down
65 changes: 32 additions & 33 deletions fixtures/dom/public/act-dom.html
@@ -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>
107 changes: 107 additions & 0 deletions fixtures/dom/src/index.test.js
@@ -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();
});
1 change: 1 addition & 0 deletions package.json
Expand Up @@ -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",
threepointone marked this conversation as resolved.
Show resolved Hide resolved
"flow": "node ./scripts/tasks/flow.js",
"flow-ci": "node ./scripts/tasks/flow-ci.js",
"prettier": "node ./scripts/prettier/index.js write-changed",
Expand Down
2 changes: 2 additions & 0 deletions packages/react-dom/src/client/ReactDOM.js
Expand Up @@ -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';
Expand Down Expand Up @@ -816,6 +817,7 @@ const ReactDOM: Object = {
dispatchEvent,
runEventsInBatch,
flushPassiveEffects,
ReactActingRendererSigil,
],
},
};
Expand Down
2 changes: 2 additions & 0 deletions packages/react-dom/src/fire/ReactFire.js
Expand Up @@ -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';
Expand Down Expand Up @@ -822,6 +823,7 @@ const ReactDOM: Object = {
dispatchEvent,
runEventsInBatch,
flushPassiveEffects,
ReactActingRendererSigil,
],
},
};
Expand Down
4 changes: 3 additions & 1 deletion packages/react-dom/src/test-utils/ReactTestUtils.js
Expand Up @@ -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) {}
Expand Down
11 changes: 6 additions & 5 deletions packages/react-dom/src/test-utils/ReactTestUtilsAct.js
Expand Up @@ -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
Expand Down Expand Up @@ -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) {
threepointone marked this conversation as resolved.
Show resolved Hide resolved
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(
Expand Down
16 changes: 10 additions & 6 deletions packages/react-noop-renderer/src/createReactNoop.js
Expand Up @@ -81,7 +81,7 @@ type TextInstance = {|
|};
type HostContext = Object;

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

const NO_CONTEXT = {};
const UPPERCASE_CONTEXT = {};
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(
Expand Down
6 changes: 3 additions & 3 deletions packages/react-reconciler/src/ReactFiberHooks.js
Expand Up @@ -35,6 +35,7 @@ import {
flushPassiveEffects,
requestCurrentTime,
warnIfNotCurrentlyActingUpdatesInDev,
warnIfNotScopedWithMatchingAct,
markRenderEventTimeAndConfig,
} from './ReactFiberWorkLoop';

Expand Down Expand Up @@ -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);
}
}
Expand Down
9 changes: 9 additions & 0 deletions packages/react-reconciler/src/ReactFiberReconciler.js
Expand Up @@ -56,6 +56,8 @@ import {
discreteUpdates,
flushDiscreteUpdates,
flushPassiveEffects,
warnIfNotScopedWithMatchingAct,
ReactActingRendererSigil,
} from './ReactFiberWorkLoop';
import {createUpdate, enqueueUpdate} from './ReactUpdateQueue';
import ReactFiberInstrumentation from './ReactFiberInstrumentation';
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -332,6 +340,7 @@ export {
flushControlled,
flushSync,
flushPassiveEffects,
ReactActingRendererSigil,
};

export function getPublicRootInstance(
Expand Down