Skip to content

Commit

Permalink
Revert change to expiration time rounding
Browse files Browse the repository at this point in the history 
This means you have to account for the start time approximation
heuristic when writing Suspense tests, but that's going to be
true regardless.

When updating the tests, I also made a fix related to offscreen
priority. We should never timeout inside a hidden tree.
  • Loading branch information
@acdlite
acdlite committed May 10, 2018
1 parent 1b835af commit 86a49da
Show file tree
Hide file tree
Showing 4 changed files with 37 additions and 49 deletions.
6 changes: 3 additions & 3 deletions packages/react-reconciler/src/ReactFiberExpirationTime.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,8 @@ export function expirationTimeToMs(expirationTime: ExpirationTime): number {
return (expirationTime - MAGIC_NUMBER_OFFSET) * UNIT_SIZE;
}

function round(num: number, precision: number): number {
return ((num / precision) | 0) * precision;
function ceiling(num: number, precision: number): number {
return (((num / precision) | 0) + 1) * precision;
}

export function computeExpirationBucket(
Expand All @@ -40,7 +40,7 @@ export function computeExpirationBucket(
): ExpirationTime {
return (
MAGIC_NUMBER_OFFSET +
round(
ceiling(
currentTime - MAGIC_NUMBER_OFFSET + expirationInMs / UNIT_SIZE,
bucketSizeMs / UNIT_SIZE,
)
Expand Down
17 changes: 1 addition & 16 deletions packages/react-reconciler/src/ReactFiberPendingPriority.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,22 +167,7 @@ export function markPingedPriorityLevel(
if (latestSuspendedTime !== NoWork && latestSuspendedTime <= pingedTime) {
const latestPingedTime = root.latestPingedTime;
if (latestPingedTime === NoWork || latestPingedTime < pingedTime) {
// TODO: At one point, we decided we'd always work on the lowest priority
// suspended level. Part of the reasoning was to avoid displaying
// intermediate suspended states, e.g. if you click on two tabs in quick
// succession, only the final tab should render. But we later realized
// that the correct solution to this problem is in user space, e.g. by
// using a setState updater for the lower priority update that refers
// to the most recent high priority value.
//
// The only reason we track the lowest suspended level is so we don't have
// to track *every* suspended level. It's good enough to work on the last
// one. But in case of a ping, we know exactly what level we received, so
// we can go ahead and work on that one.
//
// Consider using the commented-out line instead:
// root.latestPingedTime = pingedTime;
root.latestPingedTime = latestSuspendedTime;
root.latestPingedTime = pingedTime;
}
}
}
Expand Down
9 changes: 6 additions & 3 deletions packages/react-reconciler/src/ReactFiberUnwindWork.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ import {
ShouldCapture,
} from 'shared/ReactTypeOfSideEffect';

import {Sync, expirationTimeToMs} from './ReactFiberExpirationTime';
import {Never, Sync, expirationTimeToMs} from './ReactFiberExpirationTime';

import {
enableGetDerivedStateFromCatch,
Expand Down Expand Up @@ -173,7 +173,10 @@ export default function<C, CX>(

const expirationTimeMs = expirationTimeToMs(renderExpirationTime);
const startTimeMs = expirationTimeMs - 5000;
const elapsedMs = currentTimeMs - startTimeMs;
let elapsedMs = currentTimeMs - startTimeMs;
if (elapsedMs < 0) {
elapsedMs = 0;
}
const remainingTimeMs = expirationTimeMs - currentTimeMs;

// Find the earliest timeout of all the timeouts in the ancestor path.
Expand Down Expand Up @@ -212,7 +215,7 @@ export default function<C, CX>(
// Compute the remaining time until the timeout.
const msUntilTimeout = earliestTimeoutMs - elapsedMs;

if (msUntilTimeout > 0) {
if (renderExpirationTime === Never || msUntilTimeout > 0) {
// There's still time remaining.
suspendRoot(root, thenable, msUntilTimeout, renderExpirationTime);
const onResolveOrReject = () => {
Expand Down
54 changes: 27 additions & 27 deletions packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ describe('ReactSuspense', () => {
return (
<Fallback>
<ErrorBoundary ref={errorBoundary}>
<AsyncText text="Result" ms={100} />
<AsyncText text="Result" ms={1000} />
</ErrorBoundary>
</Fallback>
);
Expand All @@ -204,8 +204,8 @@ describe('ReactSuspense', () => {
expect(ReactNoop.getChildren()).toEqual([]);

textResourceShouldFail = true;
ReactNoop.expire(100);
await advanceTimers(100);
ReactNoop.expire(1000);
await advanceTimers(1000);
textResourceShouldFail = false;

expect(ReactNoop.flush()).toEqual([
Expand All @@ -222,8 +222,8 @@ describe('ReactSuspense', () => {
cache.invalidate();

expect(ReactNoop.flush()).toEqual(['Suspend! [Result]']);
ReactNoop.expire(100);
await advanceTimers(100);
ReactNoop.expire(1000);
await advanceTimers(1000);
expect(ReactNoop.flush()).toEqual(['Promise resolved [Result]', 'Result']);
expect(ReactNoop.getChildren()).toEqual([span('Result')]);
});
Expand All @@ -248,9 +248,9 @@ describe('ReactSuspense', () => {
const errorBoundary = React.createRef();
function App() {
return (
<Fallback timeout={50} placeholder={<Text text="Loading..." />}>
<Fallback timeout={1000} placeholder={<Text text="Loading..." />}>
<ErrorBoundary ref={errorBoundary}>
<AsyncText text="Result" ms={100} />
<AsyncText text="Result" ms={3000} />
</ErrorBoundary>
</Fallback>
);
Expand All @@ -260,14 +260,14 @@ describe('ReactSuspense', () => {
expect(ReactNoop.flush()).toEqual(['Suspend! [Result]']);
expect(ReactNoop.getChildren()).toEqual([]);

ReactNoop.expire(50);
await advanceTimers(50);
ReactNoop.expire(2000);
await advanceTimers(2000);
expect(ReactNoop.flush()).toEqual(['Suspend! [Result]', 'Loading...']);
expect(ReactNoop.getChildren()).toEqual([span('Loading...')]);

textResourceShouldFail = true;
ReactNoop.expire(50);
await advanceTimers(50);
ReactNoop.expire(1000);
await advanceTimers(1000);
textResourceShouldFail = false;

expect(ReactNoop.flush()).toEqual([
Expand All @@ -283,9 +283,9 @@ describe('ReactSuspense', () => {
errorBoundary.current.reset();
cache.invalidate();

expect(ReactNoop.flush()).toEqual(['Suspend! [Result]', 'Loading...']);
ReactNoop.expire(100);
await advanceTimers(100);
expect(ReactNoop.flush()).toEqual(['Suspend! [Result]']);
ReactNoop.expire(3000);
await advanceTimers(3000);
expect(ReactNoop.flush()).toEqual(['Promise resolved [Result]', 'Result']);
expect(ReactNoop.getChildren()).toEqual([span('Result')]);
});
Expand Down Expand Up @@ -495,8 +495,8 @@ describe('ReactSuspense', () => {

// Expire the outer timeout, but don't expire the inner one.
// We should see the outer loading placeholder.
ReactNoop.expire(1000);
await advanceTimers(1000);
ReactNoop.expire(1500);
await advanceTimers(1500);
expect(ReactNoop.flush()).toEqual([
'Sync',
// Still suspended.
Expand Down Expand Up @@ -600,8 +600,8 @@ describe('ReactSuspense', () => {
it('expires early with a `timeout` option', async () => {
ReactNoop.render(
<Fragment>
<Fallback timeout={100} placeholder={<Text text="Loading..." />}>
<AsyncText text="Async" ms={1000} />
<Fallback timeout={1000} placeholder={<Text text="Loading..." />}>
<AsyncText text="Async" ms={3000} />
</Fallback>
<Text text="Sync" />
</Fragment>,
Expand All @@ -619,8 +619,8 @@ describe('ReactSuspense', () => {
// Advance both React's virtual time and Jest's timers by enough to trigger
// the timeout, but not by enough to flush the promise or reach the true
// expiration time.
ReactNoop.expire(120);
await advanceTimers(120);
ReactNoop.expire(2000);
await advanceTimers(2000);
expect(ReactNoop.flush()).toEqual([
// Still suspended.
'Suspend! [Async]',
Expand Down Expand Up @@ -763,7 +763,7 @@ describe('ReactSuspense', () => {
{didTimeout => (
<Fragment>
<div hidden={didTimeout}>
<AsyncText text="Async" ms={2000} />
<AsyncText text="Async" ms={3000} />
</div>
{didTimeout ? <Text text="Loading..." /> : null}
</Fragment>
Expand All @@ -776,8 +776,8 @@ describe('ReactSuspense', () => {
expect(ReactNoop.flush()).toEqual(['Suspend! [Async]']);
expect(ReactNoop.getChildren()).toEqual([]);

ReactNoop.expire(1000);
await advanceTimers(1000);
ReactNoop.expire(2000);
await advanceTimers(2000);
expect(ReactNoop.flush()).toEqual([
'Suspend! [Async]',
'Loading...',
Expand Down Expand Up @@ -932,13 +932,13 @@ describe('ReactSuspense', () => {
ReactNoop.flush();
expect(ReactNoop.getChildren()).toEqual([]);

await advanceTimers(999);
ReactNoop.expire(999);
await advanceTimers(800);
ReactNoop.expire(800);
ReactNoop.flush();
expect(ReactNoop.getChildren()).toEqual([]);

await advanceTimers(1);
ReactNoop.expire(1);
await advanceTimers(1000);
ReactNoop.expire(1000);
ReactNoop.flush();
expect(ReactNoop.getChildren()).toEqual([span('A')]);
});
Expand Down

0 comments on commit 86a49da

Please sign in to comment.