Skip to content

Commit

Permalink
flush fallbacks in tests
Browse files Browse the repository at this point in the history
In this PR, for tests (specifically, code inside an `act()` scope), we immediately trigger work that would have otherwise required a timeout. This makes it simpler to tests loading/spinner states, and makes tests resilient to changes in React.

For some of our tests(specifically, ReactSuspenseWithNoopRenderer-test.internal), we _don't_ want fallbacks to immediately trigger, because we're testing intermediate states and such. Added a feature flag `flushSuspenseFallbacksInTests` to disable this behaviour on a per case basis.
  • Loading branch information
threepointone committed Jul 30, 2019
1 parent 75ab53b commit e117bcb
Show file tree
Hide file tree
Showing 10 changed files with 80 additions and 4 deletions.
45 changes: 45 additions & 0 deletions packages/react-dom/src/__tests__/ReactTestUtilsAct-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -693,5 +693,50 @@ function runActTests(label, render, unmount) {
}
});
});

describe('suspense', () => {
it('triggers fallbacks if available', async () => {
const promise = new Promise(() => {});

function Suspends() {
throw promise;
}
function App(props) {
return (
<React.Suspense
fallback={<span data-test-id="spinner">loading...</span>}>
{props.suspend ? <Suspends /> : 'content'}
</React.Suspense>
);
}
// render something so there's content
act(() => {
render(<App suspend={false} />, container);
});

// trigger a suspendy update
act(() => {
React.unstable_withSuspenseConfig(
() => {
act(() => {
render(<App suspend={true} />, container);
});
},
{timeout: 5000},
);
});

expect(document.querySelector('[data-test-id=spinner]')).not.toBeNull();
// now render regular content again
act(() => {
render(<App suspend={false} />, container);
});

// this next assertion fails, investigating.
// in concurrent and batched modes, why is the spinner still visible?
// (inb4: fake timers and async act, they don't change this outcome)
expect(document.querySelector('[data-test-id=spinner]')).toBeNull();
});
});
});
}
27 changes: 23 additions & 4 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import {
enableProfilerTimer,
enableSchedulerTracing,
revertPassiveEffectsChange,
flushSuspenseFallbacksInTests,
} from 'shared/ReactFeatureFlags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import invariant from 'shared/invariant';
Expand Down Expand Up @@ -989,7 +990,7 @@ function renderRoot(
case RootIncomplete: {
invariant(false, 'Should have a work-in-progress.');
}
// Flow knows about invariant, so it compains if I add a break statement,
// Flow knows about invariant, so it complains if I add a break statement,
// but eslint doesn't know about invariant, so it complains if I do.
// eslint-disable-next-line no-fallthrough
case RootErrored: {
Expand Down Expand Up @@ -1023,7 +1024,14 @@ function renderRoot(
// possible.
const hasNotProcessedNewUpdates =
workInProgressRootLatestProcessedExpirationTime === Sync;
if (hasNotProcessedNewUpdates && !isSync) {
if (
hasNotProcessedNewUpdates &&
!(
isSync ||
// if we're inside an act() scope, we trigger a fallback immediately
(flushSuspenseFallbacksInTests && IsThisRendererActing.current)
)
) {
// If we have not processed any new updates during this pass, then this is
// either a retry of an existing fallback state or a hidden tree.
// Hidden trees shouldn't be batched with other work and after that's
Expand Down Expand Up @@ -1060,7 +1068,13 @@ function renderRoot(
return commitRoot.bind(null, root);
}
case RootSuspendedWithDelay: {
if (!isSync) {
if (
!(
isSync ||
// if we're inside an act() scope, we trigger a fallback immediately
(flushSuspenseFallbacksInTests && IsThisRendererActing.current)
)
) {
// We're suspended in a state that should be avoided. We'll try to avoid committing
// it for as long as the timeouts let us.
if (workInProgressRootHasPendingPing) {
Expand Down Expand Up @@ -1130,7 +1144,11 @@ function renderRoot(
case RootCompleted: {
// The work completed. Ready to commit.
if (
!isSync &&
!(
isSync ||
// if we're inside an act() scope, we trigger a fallback immediately
(flushSuspenseFallbacksInTests && IsThisRendererActing.current)
) &&
workInProgressRootLatestProcessedExpirationTime !== Sync &&
workInProgressRootCanSuspendUsingConfig !== null
) {
Expand Down Expand Up @@ -2435,6 +2453,7 @@ function warnAboutInvalidUpdatesOnClassComponentsInDEV(fiber) {
}
}

// a 'shared' variable that changes when act() opens/closes in tests.
export const IsThisRendererActing = {current: (false: boolean)};

export function warnIfNotScopedWithMatchingAct(fiber: Fiber): void {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ describe('ReactSuspenseWithNoopRenderer', () => {
ReactFeatureFlags = require('shared/ReactFeatureFlags');
ReactFeatureFlags.debugRenderPhaseSideEffectsForStrictMode = false;
ReactFeatureFlags.replayFailedUnitOfWorkWithInvokeGuardedCallback = false;
ReactFeatureFlags.flushSuspenseFallbacksInTests = false;
React = require('react');
Fragment = React.Fragment;
ReactNoop = require('react-noop-renderer');
Expand Down
4 changes: 4 additions & 0 deletions packages/shared/ReactFeatureFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,10 @@ export const warnAboutMissingMockScheduler = false;
// Temporary flag to revert the fix in #15650
export const revertPassiveEffectsChange = false;

// For tests, we flush suspense fallbacks in an act scope;
// *except* in some of our own tests, where we test incremental loading states.
export const flushSuspenseFallbacksInTests = true;

// Changes priority of some events like mousemove to user-blocking priority,
// but without making them discrete. The flag exists in case it causes
// starvation problems.
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-fb.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ export const enableFundamentalAPI = false;
export const enableJSXTransformAPI = false;
export const warnAboutMissingMockScheduler = true;
export const revertPassiveEffectsChange = false;
export const flushSuspenseFallbacksInTests = true;
export const enableUserBlockingEvents = false;
export const enableSuspenseCallback = false;
export const warnAboutDefaultPropsOnFunctionComponents = false;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.native-oss.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export const enableFundamentalAPI = false;
export const enableJSXTransformAPI = false;
export const warnAboutMissingMockScheduler = false;
export const revertPassiveEffectsChange = false;
export const flushSuspenseFallbacksInTests = true;
export const enableUserBlockingEvents = false;
export const enableSuspenseCallback = false;
export const warnAboutDefaultPropsOnFunctionComponents = false;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.persistent.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export const enableFundamentalAPI = false;
export const enableJSXTransformAPI = false;
export const warnAboutMissingMockScheduler = true;
export const revertPassiveEffectsChange = false;
export const flushSuspenseFallbacksInTests = true;
export const enableUserBlockingEvents = false;
export const enableSuspenseCallback = false;
export const warnAboutDefaultPropsOnFunctionComponents = false;
Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.test-renderer.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export const enableFundamentalAPI = false;
export const enableJSXTransformAPI = false;
export const warnAboutMissingMockScheduler = false;
export const revertPassiveEffectsChange = false;
export const flushSuspenseFallbacksInTests = true;
export const enableUserBlockingEvents = false;
export const enableSuspenseCallback = false;
export const warnAboutDefaultPropsOnFunctionComponents = false;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ export const enableFlareAPI = true;
export const enableFundamentalAPI = false;
export const enableJSXTransformAPI = true;
export const warnAboutMissingMockScheduler = true;
export const flushSuspenseFallbacksInTests = true;
export const enableUserBlockingEvents = false;
export const enableSuspenseCallback = true;
export const warnAboutDefaultPropsOnFunctionComponents = false;
Expand Down
2 changes: 2 additions & 0 deletions packages/shared/forks/ReactFeatureFlags.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ export const enableSuspenseCallback = true;

export const warnAboutDefaultPropsOnFunctionComponents = false;

export const flushSuspenseFallbacksInTests = true;

// Flow magic to verify the exports of this file match the original version.
// eslint-disable-next-line no-unused-vars
type Check<_X, Y: _X, X: Y = _X> = null;
Expand Down

0 comments on commit e117bcb

Please sign in to comment.