Skip to content

Commit

Permalink
Internal act: Flush timers at end of scope (#19788)
Browse files Browse the repository at this point in the history
If there are any suspended fallbacks at the end of the `act` scope,
force them to display by running the pending timers (i.e. `setTimeout`).

The public implementation of `act` achieves the same behavior with an
extra check in the work loop (`shouldForceFlushFallbacks`). Since our
internal `act` needs to work in both development and production, without
additional runtime checks, we instead rely on Jest's mock timers.

This doesn't not affect refresh transitions, which are meant to delay
indefinitely, because in that case we exit the work loop without
posting a timer.
  • Loading branch information
acdlite authored Sep 9, 2020
1 parent d17086c commit e7b2553
Show file tree
Hide file tree
Showing 10 changed files with 48 additions and 35 deletions.
5 changes: 3 additions & 2 deletions packages/react-dom/src/test-utils/ReactTestUtilsAct.js
Original file line number Diff line number Diff line change
Expand Up @@ -126,8 +126,9 @@ export function unstable_concurrentAct(scope: () => Thenable<mixed> | void) {
}

function flushActWork(resolve, reject) {
// TODO: Run timers to flush suspended fallbacks
// jest.runOnlyPendingTimers();
// Flush suspended fallbacks
// $FlowFixMe: Flow doesn't know about global Jest object
jest.runOnlyPendingTimers();
enqueueTask(() => {
try {
const didFlushWork = Scheduler.unstable_flushAllWithoutAsserting();
Expand Down
5 changes: 3 additions & 2 deletions packages/react-noop-renderer/src/createReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -1175,8 +1175,9 @@ function createReactNoop(reconciler: Function, useMutation: boolean) {
}

function flushActWork(resolve, reject) {
// TODO: Run timers to flush suspended fallbacks
// jest.runOnlyPendingTimers();
// Flush suspended fallbacks
// $FlowFixMe: Flow doesn't know about global Jest object
jest.runOnlyPendingTimers();
enqueueTask(() => {
try {
const didFlushWork = Scheduler.unstable_flushAllWithoutAsserting();
Expand Down
18 changes: 13 additions & 5 deletions packages/react-reconciler/src/__tests__/ReactBlocks-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ let useState;
let Suspense;
let block;
let readString;
let resolvePromises;
let Scheduler;

describe('ReactBlocks', () => {
Expand All @@ -28,15 +29,16 @@ describe('ReactBlocks', () => {
useState = React.useState;
Suspense = React.Suspense;
const cache = new Map();
let unresolved = [];
readString = function(text) {
let entry = cache.get(text);
if (!entry) {
entry = {
promise: new Promise(resolve => {
setTimeout(() => {
unresolved.push(() => {
entry.resolved = true;
resolve();
}, 100);
});
}),
resolved: false,
};
Expand All @@ -47,6 +49,12 @@ describe('ReactBlocks', () => {
}
return text;
};

resolvePromises = () => {
const res = unresolved;
unresolved = [];
res.forEach(r => r());
};
});

// @gate experimental
Expand Down Expand Up @@ -144,7 +152,7 @@ describe('ReactBlocks', () => {
expect(ReactNoop).toMatchRenderedOutput('Loading...');

await ReactNoop.act(async () => {
jest.advanceTimersByTime(1000);
resolvePromises();
});

expect(ReactNoop).toMatchRenderedOutput(<span>Name: Sebastian</span>);
Expand Down Expand Up @@ -291,7 +299,7 @@ describe('ReactBlocks', () => {
ReactNoop.render(<App Page={loadParent('Sebastian')} />);
});
await ReactNoop.act(async () => {
jest.advanceTimersByTime(1000);
resolvePromises();
});
expect(ReactNoop).toMatchRenderedOutput(<span>Name: Sebastian</span>);
});
Expand Down Expand Up @@ -336,7 +344,7 @@ describe('ReactBlocks', () => {
});
await ReactNoop.act(async () => {
_setSuspend(false);
jest.advanceTimersByTime(1000);
resolvePromises();
});
expect(ReactNoop).toMatchRenderedOutput(<span>Sebastian</span>);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -575,7 +575,7 @@ describe('ReactSuspenseWithNoopRenderer', () => {
<Suspense fallback="Loading...">
<Text text="Sibling" />
{shouldSuspend ? (
<AsyncText ms={10000} text={'Step ' + step} />
<AsyncText text={'Step ' + step} />
) : (
<Text text={'Step ' + step} />
)}
Expand Down Expand Up @@ -2595,7 +2595,7 @@ describe('ReactSuspenseWithNoopRenderer', () => {
}
return (
<Suspense fallback={<Text text="Loading..." />}>
<AsyncText text={page} ms={5000} />
<AsyncText text={page} />
</Suspense>
);
}
Expand All @@ -2616,8 +2616,7 @@ describe('ReactSuspenseWithNoopRenderer', () => {
});

// Later we load the data.
Scheduler.unstable_advanceTime(5000);
await advanceTimers(5000);
await resolveText('A');
expect(Scheduler).toHaveYielded(['Promise resolved [A]']);
expect(Scheduler).toFlushAndYield(['A']);
expect(ReactNoop.getChildren()).toEqual([span('A')]);
Expand All @@ -2635,8 +2634,7 @@ describe('ReactSuspenseWithNoopRenderer', () => {
});

// Later we load the data.
Scheduler.unstable_advanceTime(3000);
await advanceTimers(3000);
await resolveText('B');
expect(Scheduler).toHaveYielded(['Promise resolved [B]']);
expect(Scheduler).toFlushAndYield(['B']);
expect(ReactNoop.getChildren()).toEqual([span('B')]);
Expand Down Expand Up @@ -2754,12 +2752,14 @@ describe('ReactSuspenseWithNoopRenderer', () => {
);
});

// TODO: This test is specifically about avoided commits that suspend for a
// JND. We may remove this behavior.
it("suspended commit remains suspended even if there's another update at same expiration", async () => {
// Regression test
function App({text}) {
return (
<Suspense fallback="Loading...">
<AsyncText ms={2000} text={text} />
<AsyncText text={text} />
</Suspense>
);
}
Expand All @@ -2768,34 +2768,28 @@ describe('ReactSuspenseWithNoopRenderer', () => {
await ReactNoop.act(async () => {
root.render(<App text="Initial" />);
});
expect(Scheduler).toHaveYielded(['Suspend! [Initial]']);

// Resolve initial render
await ReactNoop.act(async () => {
Scheduler.unstable_advanceTime(2000);
await advanceTimers(2000);
await resolveText('Initial');
});
expect(Scheduler).toHaveYielded([
'Suspend! [Initial]',
'Promise resolved [Initial]',
'Initial',
]);
expect(Scheduler).toHaveYielded(['Promise resolved [Initial]', 'Initial']);
expect(root).toMatchRenderedOutput(<span prop="Initial" />);

// Update. Since showing a fallback would hide content that's already
// visible, it should suspend for a bit without committing.
await ReactNoop.act(async () => {
// Update. Since showing a fallback would hide content that's already
// visible, it should suspend for a JND without committing.
root.render(<App text="First update" />);

expect(Scheduler).toFlushAndYield(['Suspend! [First update]']);

// Should not display a fallback
expect(root).toMatchRenderedOutput(<span prop="Initial" />);
});

// Update again. This should also suspend for a bit.
await ReactNoop.act(async () => {
// Update again. This should also suspend for a JND.
root.render(<App text="Second update" />);

expect(Scheduler).toFlushAndYield(['Suspend! [Second update]']);

// Should not display a fallback
expect(root).toMatchRenderedOutput(<span prop="Initial" />);
});
Expand Down Expand Up @@ -3989,9 +3983,6 @@ describe('ReactSuspenseWithNoopRenderer', () => {
await ReactNoop.act(async () => {
root.render(<App show={true} />);
});
// TODO: `act` should have already flushed the placeholder, so this
// runAllTimers call should be unnecessary.
jest.runAllTimers();
expect(Scheduler).toHaveYielded(['Suspend! [Async]', 'Loading...']);
expect(root).toMatchRenderedOutput(
<>
Expand Down
5 changes: 3 additions & 2 deletions packages/react-test-renderer/src/ReactTestRenderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -684,8 +684,9 @@ function unstable_concurrentAct(scope: () => Thenable<mixed> | void) {
}

function flushActWork(resolve, reject) {
// TODO: Run timers to flush suspended fallbacks
// jest.runOnlyPendingTimers();
// Flush suspended fallbacks
// $FlowFixMe: Flow doesn't know about global Jest object
jest.runOnlyPendingTimers();
enqueueTask(() => {
try {
const didFlushWork = Scheduler.unstable_flushAllWithoutAsserting();
Expand Down
1 change: 1 addition & 0 deletions scripts/rollup/validate/eslintrc.cjs.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ module.exports = {

// jest
expect: true,
jest: true,
},
parserOptions: {
ecmaVersion: 5,
Expand Down
1 change: 1 addition & 0 deletions scripts/rollup/validate/eslintrc.cjs2015.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ module.exports = {

// jest
expect: true,
jest: true,
},
parserOptions: {
ecmaVersion: 2015,
Expand Down
3 changes: 3 additions & 0 deletions scripts/rollup/validate/eslintrc.fb.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,9 @@ module.exports = {
// Flight
Uint8Array: true,
Promise: true,

// jest
jest: true,
},
parserOptions: {
ecmaVersion: 5,
Expand Down
3 changes: 3 additions & 0 deletions scripts/rollup/validate/eslintrc.rn.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ module.exports = {
ArrayBuffer: true,

TaskController: true,

// jest
jest: true,
},
parserOptions: {
ecmaVersion: 5,
Expand Down
3 changes: 3 additions & 0 deletions scripts/rollup/validate/eslintrc.umd.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ module.exports = {
// Flight Webpack
__webpack_chunk_load__: true,
__webpack_require__: true,

// jest
jest: true,
},
parserOptions: {
ecmaVersion: 5,
Expand Down

0 comments on commit e7b2553

Please sign in to comment.