Skip to content

Commit

Permalink
await act(async () => ...) (#14853)
Browse files Browse the repository at this point in the history 
This took a while, but I'm happy I went through it. Some key moments - recursively flushing effects, flushing microtasks on each async turn, and my team's uncompromising philosophy on code reuse. Really happy with this. I still want to expand test coverage, and I have some more small related todos, but this is good to land. On to the next one. 

Soundtrack to landing this - https://open.spotify.com/track/0MF8I8OUo8kytiOo8aSHYq?si=gSWqUheKQbiQDXzptCXHTg

* hacked up act(async () => {...})

* move stuff around

* merge changes

* abstract .act warnings and stuff. all renderers. pass all tests.

* move testutils.act back into testutils

* move into scheduler, rename some bits

* smaller bundle

* a comment for why we don't do typeof === 'function'

* fix test

* pass tests - fire, prod

* lose actContainerElement

* tighter

* write a test for TestRenderer

it's an odd one, because not only does sync act not flush effects correctly, but the async one does (wut). verified it's fine with the dom version.

* lint

* rewrote to move flushing logic closer to the renderer

the scheduler's `flushPassiveEffects` didn't work as expected for the test renderer, so I decided to go back to the hack (rendering a dumb container) This also makes reactdom not as heavy (by a few bytes, but still).

* move it around so the delta isn't too bad

* cleanups

fix promise chaining
propagate errors correctly
test for thenable the 'right' way
more tests!
tidier!
ponies!

* Stray comment

* recursively flush effects

* fixed tests

* lint, move noop.act into react-reconciler

* microtasks when checking if called, s/called/calledLog, cleanup

* pass fb lint

we could have globally changed our eslint config to assume Promise is available, but that means we expect a promise polyfill on the page, and we don't yet. this code is triggered only in jest anyway, and we're fairly certain Promise will be available there. hence, the once-off disable for the check

* shorter timers, fix a test, test for Promise

* use global.Promise for existence check

* flush microtasks

* a version that works in browsers (that support postMessage)

I also added a sanity fixture inside fixtures/dom/ mostly for me.

* hoist flushEffectsAndMicroTasks

* pull out tick logic from ReactFiberScheduler

* fix await act (...sync) hanging

- fix a hang when awaiting sync logic
- a better async/await test for test renderer

* feedback changes

- use node's setImmediate if available
- a warning if MessageChannel isn't available
- rename some functions

* pass lint/flow checks (without requiring a Promise polyfill/exclusion)

* prettier

the prettiest, even.

* use globalPromise for the missed await warning

* __DEV__ check for didWarnAboutMessageChannel

* thenables and callbacks instead of promises, pass flow/lint

* tinier. better.

- pulled most bits out of FiberScheduler
- actedUpdates uses callbacks now

* pass build validation

* augh prettier

* golfing 7 more chars

* Test that effects are not flushed without also flushing microtasks

* export doesHavePendingPassiveEffects, nits

* createAct()

* dead code

* missed in merge?

* lose the preflushing bits

* ugh prettier

* removed `actedUpdates()`, created shared/actingUpdatesScopeDepth

* rearrange imports so builds work, remove the hack versions of flushPassiveEffects

* represent actingUpdatesScopeDepth as a tuple [number]

* use a shared flag on React.__SECRET...

* remove createAct, setup act for all relevant renderers

* review feedback

shared/enqueueTask

import ReactSharedInternals from 'shared/ReactSharedInternals';

simpler act() internals

ReactSharedInternals.ReactShouldWarnActingUpdates

* move act() implementation into createReactNoop

* warnIfNotCurrentlyActingUpdatesInDev condition check order
  • Loading branch information
Sunil Pai authored Apr 2, 2019
1 parent 4c75881 commit aed0e1c
Show file tree
Hide file tree
Showing 25 changed files with 1,639 additions and 600 deletions.
2 changes: 2 additions & 0 deletions fixtures/dom/.gitignore
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ public/react-dom.development.js
public/react-dom.production.min.js
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

# misc
.DS_Store
Expand Down
2 changes: 1 addition & 1 deletion fixtures/dom/package.json
View file
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 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 public/",
"build": "react-scripts build && cp build/index.html build/200.html",
"test": "react-scripts test --env=jsdom",
"eject": "react-scripts eject"
Expand Down
41 changes: 41 additions & 0 deletions fixtures/dom/public/act-dom.html
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
<!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
function App() {
let [state, setState] = React.useState(0);
async function ticker() {
await null;
setState(x => x + 1);
}
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>
</html>
171 changes: 0 additions & 171 deletions packages/react-dom/src/__tests__/ReactTestUtils-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ let React;
let ReactDOM;
let ReactDOMServer;
let ReactTestUtils;
let act;

function getTestDocument(markup) {
const doc = document.implementation.createHTMLDocument('');
Expand All @@ -34,7 +33,6 @@ describe('ReactTestUtils', () => {
ReactDOM = require('react-dom');
ReactDOMServer = require('react-dom/server');
ReactTestUtils = require('react-dom/test-utils');
act = ReactTestUtils.act;
});

it('Simulate should have locally attached media events', () => {
Expand Down Expand Up @@ -517,173 +515,4 @@ describe('ReactTestUtils', () => {
ReactTestUtils.renderIntoDocument(<Component />);
expect(mockArgs.length).toEqual(0);
});

it('can use act to batch effects', () => {
function App(props) {
React.useEffect(props.callback);
return null;
}
const container = document.createElement('div');
document.body.appendChild(container);

try {
let called = false;
act(() => {
ReactDOM.render(
<App
callback={() => {
called = true;
}}
/>,
container,
);
});

expect(called).toBe(true);
} finally {
document.body.removeChild(container);
}
});

it('flushes effects on every call', () => {
function App(props) {
let [ctr, setCtr] = React.useState(0);
React.useEffect(() => {
props.callback(ctr);
});
return (
<button id="button" onClick={() => setCtr(x => x + 1)}>
click me!
</button>
);
}

const container = document.createElement('div');
document.body.appendChild(container);
let calledCtr = 0;
act(() => {
ReactDOM.render(
<App
callback={val => {
calledCtr = val;
}}
/>,
container,
);
});
const button = document.getElementById('button');
function click() {
button.dispatchEvent(new MouseEvent('click', {bubbles: true}));
}

act(() => {
click();
click();
click();
});
expect(calledCtr).toBe(3);
act(click);
expect(calledCtr).toBe(4);
act(click);
expect(calledCtr).toBe(5);

document.body.removeChild(container);
});

it('can use act to batch effects on updates too', () => {
function App() {
let [ctr, setCtr] = React.useState(0);
return (
<button id="button" onClick={() => setCtr(x => x + 1)}>
{ctr}
</button>
);
}
const container = document.createElement('div');
document.body.appendChild(container);
let button;
act(() => {
ReactDOM.render(<App />, container);
});
button = document.getElementById('button');
expect(button.innerHTML).toBe('0');
act(() => {
button.dispatchEvent(new MouseEvent('click', {bubbles: true}));
});
expect(button.innerHTML).toBe('1');
document.body.removeChild(container);
});

it('detects setState being called outside of act(...)', () => {
let setValueRef = null;
function App() {
let [value, setValue] = React.useState(0);
setValueRef = setValue;
return (
<button id="button" onClick={() => setValue(2)}>
{value}
</button>
);
}
const container = document.createElement('div');
document.body.appendChild(container);
let button;
act(() => {
ReactDOM.render(<App />, container);
button = container.querySelector('#button');
button.dispatchEvent(new MouseEvent('click', {bubbles: true}));
});
expect(button.innerHTML).toBe('2');
expect(() => setValueRef(1)).toWarnDev([
'An update to App inside a test was not wrapped in act(...).',
]);
document.body.removeChild(container);
});

it('lets a ticker update', () => {
function App() {
let [toggle, setToggle] = React.useState(0);
React.useEffect(() => {
let timeout = setTimeout(() => {
setToggle(1);
}, 200);
return () => clearTimeout(timeout);
});
return toggle;
}
const container = document.createElement('div');

act(() => {
act(() => {
ReactDOM.render(<App />, container);
});
jest.advanceTimersByTime(250);
});

expect(container.innerHTML).toBe('1');
});

it('warns if you return a value inside act', () => {
expect(() => act(() => null)).toWarnDev(
[
'The callback passed to ReactTestUtils.act(...) function must not return anything.',
],
{withoutStack: true},
);
expect(() => act(() => 123)).toWarnDev(
[
'The callback passed to ReactTestUtils.act(...) function must not return anything.',
],
{withoutStack: true},
);
});

it('warns if you try to await an .act call', () => {
expect(act(() => {}).then).toWarnDev(
[
'Do not await the result of calling ReactTestUtils.act(...), it is not a Promise.',
],
{withoutStack: true},
);
});
});
Loading

0 comments on commit aed0e1c

Please sign in to comment.