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

Don't group Idle/Offscreen work with other work #17456

Merged
merged 1 commit into from Dec 3, 2019
Merged

Don't group Idle/Offscreen work with other work #17456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
19 changes: 15 additions & 4 deletions packages/react-reconciler/src/ReactFiberWorkLoop.js
View file
Open in desktop
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.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm a bit uneasy about this since if something goes wrong, it's not possible to unblock by pinging these levels. However, I don't think it ever happens now because these levels wouldn't ever be suspended if something else rendered at higher pri because that makes them unsuspended. I couldn't make a test that fails.

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
115 changes: 114 additions & 1 deletion packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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.www.js
View file
Open in desktop
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
View file
Open in desktop
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