Skip to content

Commit

Permalink
using the wrong renderer's act() should warn (#15756)
Browse files Browse the repository at this point in the history
* 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
  • Loading branch information
Sunil Pai authored May 29, 2019
1 parent 8ce8b9a commit 9aad17d
Show file tree
Hide file tree
Showing 16 changed files with 241 additions and 58 deletions.
1 change: 1 addition & 0 deletions .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,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
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/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
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,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",
"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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
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,
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
Original file line number Diff line number Diff line change
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) {
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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
Loading

0 comments on commit 9aad17d

Please sign in to comment.