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

Clean up timeoutMs-related implementation details #19704

Merged
merged 3 commits into from
Aug 27, 2020
Merged

Clean up timeoutMs-related implementation details #19704

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ada4541
Disable busyDelayMs and busyMinDurationMs
acdlite Aug 26, 2020
a9e9d14
Remove unnecessary work loop variables
acdlite Aug 26, 2020
9391eb7
Remove unnecessary Update fields
acdlite Aug 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Disable busyDelayMs and busyMinDurationMs 
Refer to explanation in previous commit.
  • Loading branch information
@acdlite
acdlite committed Aug 26, 2020
commit ada4541e79e8a9a6a947911e5ed07d51296291a7
51 changes: 0 additions & 51 deletions packages/react-reconciler/src/ReactFiberWorkLoop.new.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,6 @@ let workInProgressRootExitStatus: RootExitStatus = RootIncomplete;
// A fatal error, if one is thrown
let workInProgressRootFatalError: mixed = null;
let workInProgressRootLatestSuspenseTimeout: number = NoTimestamp;
let workInProgressRootCanSuspendUsingConfig: null | SuspenseConfig = null;
// "Included" lanes refer to lanes that were worked on during this render. It's
// slightly different than `renderLanes` because `renderLanes` can change as you
// enter and exit an Offscreen tree. This value is the combination of all render
Expand Down Expand Up @@ -963,30 +962,6 @@ function finishConcurrentRender(root, exitStatus, lanes) {
}
case RootCompleted: {
// The work completed. Ready to commit.
const mostRecentEventTime = getMostRecentEventTime(root, lanes);
if (
// do not delay if we're inside an act() scope
!shouldForceFlushFallbacksInDEV() &&
mostRecentEventTime !== NoTimestamp &&
workInProgressRootCanSuspendUsingConfig !== null
) {
// If we have exceeded the minimum loading delay, which probably
// means we have shown a spinner already, we might have to suspend
// a bit longer to ensure that the spinner is shown for
// enough time.
const msUntilTimeout = computeMsUntilSuspenseLoadingDelay(
mostRecentEventTime,
workInProgressRootCanSuspendUsingConfig,
);
if (msUntilTimeout > 10) {
markRootSuspended(root, lanes);
root.timeoutHandle = scheduleTimeout(
commitRoot.bind(null, root),
msUntilTimeout,
);
break;
}
}
commitRoot(root);
break;
}
Expand Down Expand Up @@ -1370,7 +1345,6 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) {
workInProgressRootExitStatus = RootIncomplete;
workInProgressRootFatalError = null;
workInProgressRootLatestSuspenseTimeout = NoTimestamp;
workInProgressRootCanSuspendUsingConfig = null;
workInProgressRootSkippedLanes = NoLanes;
workInProgressRootUpdatedLanes = NoLanes;
workInProgressRootPingedLanes = NoLanes;
Expand Down Expand Up @@ -1501,7 +1475,6 @@ export function markRenderEventTimeAndConfig(
const timeoutTime = eventTime + timeoutMs;
if (timeoutTime > workInProgressRootLatestSuspenseTimeout) {
workInProgressRootLatestSuspenseTimeout = timeoutTime;
workInProgressRootCanSuspendUsingConfig = suspenseConfig;
}
}
}
Expand Down Expand Up @@ -3079,30 +3052,6 @@ function jnd(timeElapsed: number) {
: ceil(timeElapsed / 1960) * 1960;
}

function computeMsUntilSuspenseLoadingDelay(
mostRecentEventTime: number,
suspenseConfig: SuspenseConfig,
) {
const busyMinDurationMs = (suspenseConfig.busyMinDurationMs: any) | 0;
if (busyMinDurationMs <= 0) {
return 0;
}
const busyDelayMs = (suspenseConfig.busyDelayMs: any) | 0;

// Compute the time until this render pass would expire.
const currentTimeMs: number = now();
const eventTimeMs: number = mostRecentEventTime;
const timeElapsed = currentTimeMs - eventTimeMs;
if (timeElapsed <= busyDelayMs) {
// If we haven't yet waited longer than the initial delay, we don't
// have to wait any additional time.
return 0;
}
const msUntilTimeout = busyDelayMs + busyMinDurationMs - timeElapsed;
// This is the value that is passed to `setTimeout`.
return msUntilTimeout;
}

function checkForNestedUpdates() {
if (nestedUpdateCount > NESTED_UPDATE_LIMIT) {
nestedUpdateCount = 0;
Expand Down
51 changes: 0 additions & 51 deletions packages/react-reconciler/src/ReactFiberWorkLoop.old.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,6 @@ let workInProgressRootExitStatus: RootExitStatus = RootIncomplete;
// A fatal error, if one is thrown
let workInProgressRootFatalError: mixed = null;
let workInProgressRootLatestSuspenseTimeout: number = NoTimestamp;
let workInProgressRootCanSuspendUsingConfig: null | SuspenseConfig = null;
// "Included" lanes refer to lanes that were worked on during this render. It's
// slightly different than `renderLanes` because `renderLanes` can change as you
// enter and exit an Offscreen tree. This value is the combination of all render
Expand Down Expand Up @@ -951,30 +950,6 @@ function finishConcurrentRender(root, exitStatus, lanes) {
}
case RootCompleted: {
// The work completed. Ready to commit.
const mostRecentEventTime = getMostRecentEventTime(root, lanes);
if (
// do not delay if we're inside an act() scope
!shouldForceFlushFallbacksInDEV() &&
mostRecentEventTime !== NoTimestamp &&
workInProgressRootCanSuspendUsingConfig !== null
) {
// If we have exceeded the minimum loading delay, which probably
// means we have shown a spinner already, we might have to suspend
// a bit longer to ensure that the spinner is shown for
// enough time.
const msUntilTimeout = computeMsUntilSuspenseLoadingDelay(
mostRecentEventTime,
workInProgressRootCanSuspendUsingConfig,
);
if (msUntilTimeout > 10) {
markRootSuspended(root, lanes);
root.timeoutHandle = scheduleTimeout(
commitRoot.bind(null, root),
msUntilTimeout,
);
break;
}
}
commitRoot(root);
break;
}
Expand Down Expand Up @@ -1358,7 +1333,6 @@ function prepareFreshStack(root: FiberRoot, lanes: Lanes) {
workInProgressRootExitStatus = RootIncomplete;
workInProgressRootFatalError = null;
workInProgressRootLatestSuspenseTimeout = NoTimestamp;
workInProgressRootCanSuspendUsingConfig = null;
workInProgressRootSkippedLanes = NoLanes;
workInProgressRootUpdatedLanes = NoLanes;
workInProgressRootPingedLanes = NoLanes;
Expand Down Expand Up @@ -1489,7 +1463,6 @@ export function markRenderEventTimeAndConfig(
const timeoutTime = eventTime + timeoutMs;
if (timeoutTime > workInProgressRootLatestSuspenseTimeout) {
workInProgressRootLatestSuspenseTimeout = timeoutTime;
workInProgressRootCanSuspendUsingConfig = suspenseConfig;
}
}
}
Expand Down Expand Up @@ -3025,30 +2998,6 @@ function jnd(timeElapsed: number) {
: ceil(timeElapsed / 1960) * 1960;
}

function computeMsUntilSuspenseLoadingDelay(
mostRecentEventTime: number,
suspenseConfig: SuspenseConfig,
) {
const busyMinDurationMs = (suspenseConfig.busyMinDurationMs: any) | 0;
if (busyMinDurationMs <= 0) {
return 0;
}
const busyDelayMs = (suspenseConfig.busyDelayMs: any) | 0;

// Compute the time until this render pass would expire.
const currentTimeMs: number = now();
const eventTimeMs: number = mostRecentEventTime;
const timeElapsed = currentTimeMs - eventTimeMs;
if (timeElapsed <= busyDelayMs) {
// If we haven't yet waited longer than the initial delay, we don't
// have to wait any additional time.
return 0;
}
const msUntilTimeout = busyDelayMs + busyMinDurationMs - timeElapsed;
// This is the value that is passed to `setTimeout`.
return msUntilTimeout;
}

function checkForNestedUpdates() {
if (nestedUpdateCount > NESTED_UPDATE_LIMIT) {
nestedUpdateCount = 0;
Expand Down
79 changes: 0 additions & 79 deletions packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3318,85 +3318,6 @@ describe('ReactHooksWithNoopRenderer', () => {
]);
});
});

// @gate experimental
it('delays showing loading state until after busyDelayMs + busyMinDurationMs', async () => {
let transition;
function App() {
const [show, setShow] = useState(false);
const [startTransition, isPending] = useTransition({
busyDelayMs: 1000,
busyMinDurationMs: 2000,
});
transition = () => {
startTransition(() => {
setShow(true);
});
};
return (
<Suspense
fallback={<Text text={`Loading... Pending: ${isPending}`} />}>
{show ? (
<AsyncText text={`After... Pending: ${isPending}`} />
) : (
<Text text={`Before... Pending: ${isPending}`} />
)}
</Suspense>
);
}
ReactNoop.render(<App />);
expect(Scheduler).toFlushAndYield(['Before... Pending: false']);
expect(ReactNoop.getChildren()).toEqual([
span('Before... Pending: false'),
]);

await act(async () => {
Scheduler.unstable_runWithPriority(
Scheduler.unstable_UserBlockingPriority,
transition,
);

expect(Scheduler).toFlushAndYield([
'Before... Pending: true',
'Suspend! [After... Pending: false]',
'Loading... Pending: false',
]);
expect(ReactNoop.getChildren()).toEqual([
span('Before... Pending: true'),
]);

Scheduler.unstable_advanceTime(1000);
await advanceTimers(1000);

// Resolve the promise. The whole tree has now completed. However,
// because we exceeded the busy threshold, we won't commit the
// result yet.
Scheduler.unstable_advanceTime(1000);
await advanceTimers(1000);
await resolveText('After... Pending: false');
expect(Scheduler).toHaveYielded([
'Promise resolved [After... Pending: false]',
]);
expect(Scheduler).toFlushAndYield(['After... Pending: false']);
expect(ReactNoop.getChildren()).toEqual([
span('Before... Pending: true'),
]);

// Advance time until just before the `busyMinDuration` threshold.
Scheduler.unstable_advanceTime(999);
await advanceTimers(999);
expect(ReactNoop.getChildren()).toEqual([
span('Before... Pending: true'),
]);

// Advance time just a bit more. Now we complete the transition.
Scheduler.unstable_advanceTime(300);
await advanceTimers(300);
expect(ReactNoop.getChildren()).toEqual([
span('After... Pending: false'),
]);
});
});
});

describe('useDeferredValue', () => {
Expand Down
68 changes: 0 additions & 68 deletions packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2860,74 +2860,6 @@ describe('ReactSuspenseWithNoopRenderer', () => {
);
});

// @gate experimental
it('supports delaying a busy spinner from disappearing', async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure we don't want to support this use case?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the idea was that longer-term, the proper way to implement this is unmount animation (which we don't have an API for). That doesn't require you to precalculate the time and would let the spinner itself decide how much it wants to stay visible before being removed. E.g. a full spin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed out-of-band

const SUSPENSE_CONFIG = {
timeoutMs: 10000,
busyDelayMs: 500,
busyMinDurationMs: 400,
};

let transitionToPage;

function App() {
const [page, setPage] = React.useState('A');
const [startLoading, isLoading] = React.unstable_useTransition(
SUSPENSE_CONFIG,
);
transitionToPage = nextPage => startLoading(() => setPage(nextPage));
return (
<Fragment>
<Text text={page} />
{isLoading ? <Text text="L" /> : null}
</Fragment>
);
}

// Initial render.
ReactNoop.render(<App />);
expect(Scheduler).toFlushAndYield(['A']);
expect(ReactNoop.getChildren()).toEqual([span('A')]);

await ReactNoop.act(async () => {
transitionToPage('B');
// Rendering B is quick and we didn't have enough
// time to show the loading indicator.
Scheduler.unstable_advanceTime(200);
await advanceTimers(200);
expect(Scheduler).toFlushAndYield(['A', 'L', 'B']);
expect(ReactNoop.getChildren()).toEqual([span('B')]);
});

await ReactNoop.act(async () => {
transitionToPage('C');
// Rendering C is a bit slower so we've already showed
// the loading indicator.
Scheduler.unstable_advanceTime(600);
await advanceTimers(600);
expect(Scheduler).toFlushAndYield(['B', 'L', 'C']);
// We're technically done now but we haven't shown the
// loading indicator for long enough yet so we'll suspend
// while we keep it on the screen a bit longer.
expect(ReactNoop.getChildren()).toEqual([span('B'), span('L')]);
Scheduler.unstable_advanceTime(400);
await advanceTimers(400);
expect(ReactNoop.getChildren()).toEqual([span('C')]);
});

await ReactNoop.act(async () => {
transitionToPage('D');
// Rendering D is very slow so we've already showed
// the loading indicator.
Scheduler.unstable_advanceTime(1000);
await advanceTimers(1000);
expect(Scheduler).toFlushAndYield(['C', 'L', 'D']);
// However, since we exceeded the minimum time to show
// the loading indicator, we commit immediately.
expect(ReactNoop.getChildren()).toEqual([span('D')]);
});
});

it("suspended commit remains suspended even if there's another update at same expiration", async () => {
// Regression test
function App({text}) {
Expand Down