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.

Sign up for GitHub

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

Jump to bottom

Flush act() only on exiting outermost call #15519

Closed
wants to merge 10 commits into from
Closed

Flush act() only on exiting outermost call #15519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c35e37a
mark react-events as private so we publish script skips it for now
Apr 3, 2019
dc605c1
Merge remote-tracking branch 'upstream/master'
Apr 3, 2019
a09bd0f
Merge remote-tracking branch 'upstream/master'
Apr 3, 2019
48ffcee
Merge remote-tracking branch 'upstream/master'
Apr 12, 2019
7cc1379
Merge remote-tracking branch 'upstream/master'
Apr 15, 2019
e86d051
Merge remote-tracking branch 'upstream/master'
Apr 16, 2019
fda3a14
Merge remote-tracking branch 'upstream/master'
Apr 25, 2019
d2c26ee
don't flush effects/tasks till the outermost one exits
Apr 27, 2019
f7aa6ca
do scope depth mutations in prod too
Apr 27, 2019
8f88e96
early return
Apr 27, 2019
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
30 changes: 17 additions & 13 deletions packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,14 +285,13 @@ describe('ReactTestUtils.act()', () => {
return ctr;
}
const el = document.createElement('div');
act(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you move this to a separate act? I would expect moving the assertion outside act is sufficient.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the effect wouldn't flush effects until the act exits, only after which the timeout in the component would start, failing the assertion. hence, 2 act()s.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove the await then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't?

ReactDOM.render(<App />, el);
});
await act(async () => {
act(() => {
ReactDOM.render(<App />, el);
});

await sleep(100);
expect(el.innerHTML).toBe('1');
});
expect(el.innerHTML).toBe('1');
});

it('can handle async/await', async () => {
Expand Down Expand Up @@ -371,18 +370,23 @@ describe('ReactTestUtils.act()', () => {
let ctr = 0;
const div = document.createElement('div');

await act(async () => {
act(() => {
ReactDOM.render(<App callback={() => ctr++} />, div);
});
expect(div.innerHTML).toBe('0');
expect(ctr).toBe(1);
act(() => {
ReactDOM.render(<App callback={() => ctr++} />, div);
});
// this may seem odd, but it matches user behaviour -
// a flash of "0" followed by "1"
expect(div.innerHTML).toBe('0');
expect(ctr).toBe(1);

await act(async () => {}); // #just-react-things
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you remove the await from the earlier act?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to specifically test the intermediate sync state. this test is named badly (and frankly on looking at it now, it doesn't seem super useful).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not clear to me at all what behavior you're actually trying to test. More comments could help.


expect(div.innerHTML).toBe('1');
expect(ctr).toBe(2);

// this may seem odd, but it matches browser behaviour -
// a flash of "0" followed by "1"

// now you probably wouldn't write a test like this for your app
// you'd await act(async () => { ReactDOM.render(<App/> })
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So then why did you write it this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

like mentioned, just wanted to make sure the intermediate state was what I'd expected.

// and assert on final render
});

it('propagates errors', async () => {
Expand Down
21 changes: 15 additions & 6 deletions packages/react-dom/src/test-utils/ReactTestUtilsAct.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,15 @@ function flushEffectsAndMicroTasks(onDone: (err: ?Error) => void) {
}

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

function onDone() {
actingUpdatesScopeDepth--;
if (__DEV__) {
actingUpdatesScopeDepth--;
if (actingUpdatesScopeDepth === 0) {
ReactShouldWarnActingUpdates.current = false;
}
Expand Down Expand Up @@ -119,6 +118,13 @@ function act(callback: () => Thenable) {
called = true;
result.then(
() => {
if (actingUpdatesScopeDepth !== 1) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any scenario where actingUpdatesScopeDepth would decrement twice, and this would be less than 1? Maybe should change to actingUpdatesScopeDepth < 1 just in case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rather, actingUpdatesScopeDepth > 1

onDone();
resolve();
return;
}
// we're about to exit the act() scope,
// now's the time to flush tasks/effects
flushEffectsAndMicroTasks((err: ?Error) => {
onDone();
if (err) {
Expand All @@ -145,9 +151,12 @@ function act(callback: () => Thenable) {
);
}

// flush effects until none remain, and cleanup
try {
while (flushPassiveEffects()) {}
if (actingUpdatesScopeDepth === 1) {
// we're about to exit the act() scope,
// now's the time to flush effects
while (flushPassiveEffects()) {}
}
onDone();
} catch (err) {
onDone();
Expand Down
23 changes: 17 additions & 6 deletions packages/react-noop-renderer/src/createReactNoop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,8 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
// this act() implementation should be exactly the same in
// ReactTestUtilsAct.js, ReactTestRendererAct.js, createReactNoop.js

// we track the 'depth' of the act() calls with this counter,
// so we can tell if any async act() calls try to run in parallel.
let actingUpdatesScopeDepth = 0;

function flushEffectsAndMicroTasks(onDone: (err: ?Error) => void) {
Expand All @@ -668,16 +670,15 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
}

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

function onDone() {
actingUpdatesScopeDepth--;
if (__DEV__) {
actingUpdatesScopeDepth--;
if (actingUpdatesScopeDepth === 0) {
ReactShouldWarnActingUpdates.current = false;
}
Expand Down Expand Up @@ -727,6 +728,13 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
called = true;
result.then(
() => {
if (actingUpdatesScopeDepth !== 1) {
onDone();
resolve();
return;
}
// we're about to exit the act() scope,
// now's the time to flush tasks/effects
flushEffectsAndMicroTasks((err: ?Error) => {
onDone();
if (err) {
Expand All @@ -753,9 +761,12 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
);
}

// flush effects until none remain, and cleanup
try {
while (flushPassiveEffects()) {}
if (actingUpdatesScopeDepth === 1) {
// we're about to exit the act() scope,
// now's the time to flush effects
while (flushPassiveEffects()) {}
}
onDone();
} catch (err) {
onDone();
Expand Down
21 changes: 15 additions & 6 deletions packages/react-test-renderer/src/ReactTestRendererAct.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,16 +41,15 @@ function flushEffectsAndMicroTasks(onDone: (err: ?Error) => void) {
}

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

function onDone() {
actingUpdatesScopeDepth--;
if (__DEV__) {
actingUpdatesScopeDepth--;
if (actingUpdatesScopeDepth === 0) {
ReactShouldWarnActingUpdates.current = false;
}
Expand Down Expand Up @@ -100,6 +99,13 @@ function act(callback: () => Thenable) {
called = true;
result.then(
() => {
if (actingUpdatesScopeDepth !== 1) {
onDone();
resolve();
return;
}
// we're about to exit the act() scope,
// now's the time to flush tasks/effects
flushEffectsAndMicroTasks((err: ?Error) => {
onDone();
if (err) {
Expand All @@ -126,9 +132,12 @@ function act(callback: () => Thenable) {
);
}

// flush effects until none remain, and cleanup
try {
while (flushPassiveEffects()) {}
if (actingUpdatesScopeDepth === 1) {
// we're about to exit the act() scope,
// now's the time to flush effects
while (flushPassiveEffects()) {}
}
onDone();
} catch (err) {
onDone();
Expand Down