Skip to content

Commit

Permalink
Don't group Idle/Offscreen work with other work (#17456)
Browse files Browse the repository at this point in the history
When we suspend we always try a lower level but we shouldn't try offscreen.
  • Loading branch information
sebmarkbage authored Dec 3, 2019
1 parent f523b2e commit dc18b8b
Show file tree
Hide file tree
Showing 9 changed files with 137 additions and 5 deletions.
19 changes: 15 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 {
warnAboutUnmockedScheduler,
flushSuspenseFallbacksInTests,
disableSchedulerTimeoutBasedOnReactExpirationTime,
enableTrainModelFix,
} from 'shared/ReactFeatureFlags';
import ReactSharedInternals from 'shared/ReactSharedInternals';
import invariant from 'shared/invariant';
Expand Down Expand Up @@ -539,9 +540,19 @@ function getNextRootExpirationTimeToWorkOn(root: FiberRoot): ExpirationTime {
// on whichever is higher priority.
const lastPingedTime = root.lastPingedTime;
const nextKnownPendingLevel = root.nextKnownPendingLevel;
return lastPingedTime > nextKnownPendingLevel
? lastPingedTime
: nextKnownPendingLevel;
const nextLevel =
lastPingedTime > nextKnownPendingLevel
? lastPingedTime
: nextKnownPendingLevel;
if (
enableTrainModelFix &&
nextLevel <= Idle &&
firstPendingTime !== nextLevel
) {
// Don't work on Idle/Never priority unless everything else is committed.
return NoWork;
}
return nextLevel;
}

// Use this function to schedule a task for a root. There's only one task per
Expand Down Expand Up @@ -2362,7 +2373,7 @@ export function pingSuspendedRoot(
// Mark the time at which this ping was scheduled.
root.lastPingedTime = suspendedTime;

if (root.finishedExpirationTime === suspendedTime) {
if (!enableTrainModelFix && root.finishedExpirationTime === suspendedTime) {
// If there's a pending fallback waiting to commit, throw it away.
root.finishedExpirationTime = NoWork;
root.finishedWork = null;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2222,7 +2222,8 @@ describe('ReactSuspenseWithNoopRenderer', () => {
Scheduler.unstable_runWithPriority(Scheduler.unstable_IdlePriority, () =>
ReactNoop.render(<Foo renderContent={2} />),
);
expect(Scheduler).toFlushAndYield(['Suspend! [A]', 'Loading A...']);
// We won't even work on Idle priority.
expect(Scheduler).toFlushAndYield([]);

// We're still suspended.
expect(ReactNoop.getChildren()).toEqual([]);
Expand Down Expand Up @@ -2789,4 +2790,116 @@ describe('ReactSuspenseWithNoopRenderer', () => {

expect(root).toMatchRenderedOutput(<span prop="Foo" />);
});

it('should not render hidden content while suspended on higher pri', async () => {
function Offscreen() {
Scheduler.unstable_yieldValue('Offscreen');
return 'Offscreen';
}
function App({showContent}) {
React.useLayoutEffect(() => {
Scheduler.unstable_yieldValue('Commit');
});
return (
<>
<div hidden={true}>
<Offscreen />
</div>
<Suspense fallback={<Text text="Loading..." />}>
{showContent ? <AsyncText text="A" ms={2000} /> : null}
</Suspense>
</>
);
}

// Initial render.
ReactNoop.render(<App showContent={false} />);
expect(Scheduler).toFlushAndYieldThrough(['Commit']);
expect(ReactNoop).toMatchRenderedOutput(<div hidden={true} />);

// Start transition.
React.unstable_withSuspenseConfig(
() => {
ReactNoop.render(<App showContent={true} />);
},
{timeoutMs: 2000},
);

expect(Scheduler).toFlushAndYield(['Suspend! [A]', 'Loading...']);
Scheduler.unstable_advanceTime(2000);
await advanceTimers(2000);
expect(Scheduler).toHaveYielded(['Promise resolved [A]']);
expect(Scheduler).toFlushAndYieldThrough(['A', 'Commit']);
expect(ReactNoop).toMatchRenderedOutput(
<>
<div hidden={true} />
<span prop="A" />
</>,
);
expect(Scheduler).toFlushAndYield(['Offscreen']);
expect(ReactNoop).toMatchRenderedOutput(
<>
<div hidden={true}>Offscreen</div>
<span prop="A" />
</>,
);
});

it('should be able to unblock higher pri content before suspended hidden', async () => {
function Offscreen() {
Scheduler.unstable_yieldValue('Offscreen');
return 'Offscreen';
}
function App({showContent}) {
React.useLayoutEffect(() => {
Scheduler.unstable_yieldValue('Commit');
});
return (
<Suspense fallback={<Text text="Loading..." />}>
<div hidden={true}>
<AsyncText text="A" ms={2000} />
<Offscreen />
</div>
{showContent ? <AsyncText text="A" ms={2000} /> : null}
</Suspense>
);
}

// Initial render.
ReactNoop.render(<App showContent={false} />);
expect(Scheduler).toFlushAndYieldThrough(['Commit']);
expect(ReactNoop).toMatchRenderedOutput(<div hidden={true} />);

// Partially render through the hidden content.
expect(Scheduler).toFlushAndYieldThrough(['Suspend! [A]']);

// Start transition.
React.unstable_withSuspenseConfig(
() => {
ReactNoop.render(<App showContent={true} />);
},
{timeoutMs: 5000},
);

expect(Scheduler).toFlushAndYield(['Suspend! [A]', 'Loading...']);
Scheduler.unstable_advanceTime(2000);
await advanceTimers(2000);
expect(Scheduler).toHaveYielded(['Promise resolved [A]']);
expect(Scheduler).toFlushAndYieldThrough(['A', 'Commit']);
expect(ReactNoop).toMatchRenderedOutput(
<>
<div hidden={true} />
<span prop="A" />
</>,
);
expect(Scheduler).toFlushAndYield(['A', 'Offscreen']);
expect(ReactNoop).toMatchRenderedOutput(
<>
<div hidden={true}>
<span prop="A" />Offscreen
</div>
<span prop="A" />
</>,
);
});
});
2 changes: 2 additions & 0 deletions packages/shared/ReactFeatureFlags.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ export const disableLegacyContext = false;

export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;

export const enableTrainModelFix = __EXPERIMENTAL__;

export const enableTrustedTypesIntegration = false;

// Flag to turn event.target and event.currentTarget in ReactNative from a reactTag to a component instance
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 @@ -42,6 +42,7 @@ export const warnAboutDefaultPropsOnFunctionComponents = false;
export const warnAboutStringRefs = false;
export const disableLegacyContext = false;
export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
export const enableTrainModelFix = false;
export const enableTrustedTypesIntegration = false;

// Only used in www builds.
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 @@ -36,6 +36,7 @@ export const warnAboutDefaultPropsOnFunctionComponents = false;
export const warnAboutStringRefs = false;
export const disableLegacyContext = false;
export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
export const enableTrainModelFix = false;
export const enableTrustedTypesIntegration = false;
export const enableNativeTargetAsInstance = 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 @@ -36,6 +36,7 @@ export const warnAboutDefaultPropsOnFunctionComponents = false;
export const warnAboutStringRefs = false;
export const disableLegacyContext = false;
export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
export const enableTrainModelFix = false;
export const enableTrustedTypesIntegration = false;
export const enableNativeTargetAsInstance = 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 @@ -36,6 +36,7 @@ export const warnAboutDefaultPropsOnFunctionComponents = false;
export const warnAboutStringRefs = false;
export const disableLegacyContext = false;
export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
export const enableTrainModelFix = false;
export const enableTrustedTypesIntegration = false;
export const enableNativeTargetAsInstance = false;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export const warnAboutDefaultPropsOnFunctionComponents = false;
export const warnAboutStringRefs = false;
export const disableLegacyContext = false;
export const disableSchedulerTimeoutBasedOnReactExpirationTime = false;
export const enableTrainModelFix = false;
export const enableTrustedTypesIntegration = false;
export const enableNativeTargetAsInstance = false;

Expand Down
1 change: 1 addition & 0 deletions packages/shared/forks/ReactFeatureFlags.www.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export const {
disableInputAttributeSyncing,
enableTrustedTypesIntegration,
enableSelectiveHydration,
enableTrainModelFix,
} = require('ReactFeatureFlags');

// In www, we have experimental support for gathering data
Expand Down

0 comments on commit dc18b8b

Please sign in to comment.