Skip to content

Commit c9bb461

Browse files
acdliteacdlite
committed
Revert change to expiration time rounding
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.
1 parent 73c1643 commit c9bb461

File tree

4 files changed

+37
-49
lines changed

4 files changed

+37
-49
lines changed

‎packages/react-reconciler/src/ReactFiberExpirationTime.js

+3-3
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,8 @@ export function expirationTimeToMs(expirationTime: ExpirationTime): number {
2929
return (expirationTime - MAGIC_NUMBER_OFFSET) * UNIT_SIZE;
3030
}
3131

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

3636
export function computeExpirationBucket(
@@ -40,7 +40,7 @@ export function computeExpirationBucket(
4040
): ExpirationTime {
4141
return (
4242
MAGIC_NUMBER_OFFSET +
43-
round(
43+
ceiling(
4444
currentTime - MAGIC_NUMBER_OFFSET + expirationInMs / UNIT_SIZE,
4545
bucketSizeMs / UNIT_SIZE,
4646
)

‎packages/react-reconciler/src/ReactFiberPendingPriority.js

+1-16
Original file line numberDiff line numberDiff line change
@@ -167,22 +167,7 @@ export function markPingedPriorityLevel(
167167
if (latestSuspendedTime !== NoWork && latestSuspendedTime <= pingedTime) {
168168
const latestPingedTime = root.latestPingedTime;
169169
if (latestPingedTime === NoWork || latestPingedTime < pingedTime) {
170-
// TODO: At one point, we decided we'd always work on the lowest priority
171-
// suspended level. Part of the reasoning was to avoid displaying
172-
// intermediate suspended states, e.g. if you click on two tabs in quick
173-
// succession, only the final tab should render. But we later realized
174-
// that the correct solution to this problem is in user space, e.g. by
175-
// using a setState updater for the lower priority update that refers
176-
// to the most recent high priority value.
177-
//
178-
// The only reason we track the lowest suspended level is so we don't have
179-
// to track *every* suspended level. It's good enough to work on the last
180-
// one. But in case of a ping, we know exactly what level we received, so
181-
// we can go ahead and work on that one.
182-
//
183-
// Consider using the commented-out line instead:
184-
// root.latestPingedTime = pingedTime;
185-
root.latestPingedTime = latestSuspendedTime;
170+
root.latestPingedTime = pingedTime;
186171
}
187172
}
188173
}

‎packages/react-reconciler/src/ReactFiberUnwindWork.js

+6-3
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ import {
4949
enableSuspense,
5050
} from 'shared/ReactFeatureFlags';
5151

52-
import {Sync, expirationTimeToMs} from './ReactFiberExpirationTime';
52+
import {Never, Sync, expirationTimeToMs} from './ReactFiberExpirationTime';
5353

5454
export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
5555
config: HostConfig<T, P, I, TI, HI, PI, C, CC, CX, PL>,
@@ -182,7 +182,10 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
182182

183183
const expirationTimeMs = expirationTimeToMs(renderExpirationTime);
184184
const startTimeMs = expirationTimeMs - 5000;
185-
const elapsedMs = currentTimeMs - startTimeMs;
185+
let elapsedMs = currentTimeMs - startTimeMs;
186+
if (elapsedMs < 0) {
187+
elapsedMs = 0;
188+
}
186189
const remainingTimeMs = expirationTimeMs - currentTimeMs;
187190

188191
// Find the earliest timeout of all the timeouts in the ancestor path.
@@ -221,7 +224,7 @@ export default function<T, P, I, TI, HI, PI, C, CC, CX, PL>(
221224
// Compute the remaining time until the timeout.
222225
const msUntilTimeout = earliestTimeoutMs - elapsedMs;
223226

224-
if (msUntilTimeout > 0) {
227+
if (renderExpirationTime === Never || msUntilTimeout > 0) {
225228
// There's still time remaining.
226229
suspendRoot(root, thenable, msUntilTimeout, renderExpirationTime);
227230
const onResolveOrReject = () => {

‎packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js

+27-27
Original file line numberDiff line numberDiff line change
@@ -193,7 +193,7 @@ describe('ReactSuspense', () => {
193193
return (
194194
<Fallback>
195195
<ErrorBoundary ref={errorBoundary}>
196-
<AsyncText text="Result" ms={100} />
196+
<AsyncText text="Result" ms={1000} />
197197
</ErrorBoundary>
198198
</Fallback>
199199
);
@@ -204,8 +204,8 @@ describe('ReactSuspense', () => {
204204
expect(ReactNoop.getChildren()).toEqual([]);
205205

206206
textResourceShouldFail = true;
207-
ReactNoop.expire(100);
208-
await advanceTimers(100);
207+
ReactNoop.expire(1000);
208+
await advanceTimers(1000);
209209
textResourceShouldFail = false;
210210

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

224224
expect(ReactNoop.flush()).toEqual(['Suspend! [Result]']);
225-
ReactNoop.expire(100);
226-
await advanceTimers(100);
225+
ReactNoop.expire(1000);
226+
await advanceTimers(1000);
227227
expect(ReactNoop.flush()).toEqual(['Promise resolved [Result]', 'Result']);
228228
expect(ReactNoop.getChildren()).toEqual([span('Result')]);
229229
});
@@ -248,9 +248,9 @@ describe('ReactSuspense', () => {
248248
const errorBoundary = React.createRef();
249249
function App() {
250250
return (
251-
<Fallback timeout={50} placeholder={<Text text="Loading..." />}>
251+
<Fallback timeout={1000} placeholder={<Text text="Loading..." />}>
252252
<ErrorBoundary ref={errorBoundary}>
253-
<AsyncText text="Result" ms={100} />
253+
<AsyncText text="Result" ms={3000} />
254254
</ErrorBoundary>
255255
</Fallback>
256256
);
@@ -260,14 +260,14 @@ describe('ReactSuspense', () => {
260260
expect(ReactNoop.flush()).toEqual(['Suspend! [Result]']);
261261
expect(ReactNoop.getChildren()).toEqual([]);
262262

263-
ReactNoop.expire(50);
264-
await advanceTimers(50);
263+
ReactNoop.expire(2000);
264+
await advanceTimers(2000);
265265
expect(ReactNoop.flush()).toEqual(['Suspend! [Result]', 'Loading...']);
266266
expect(ReactNoop.getChildren()).toEqual([span('Loading...')]);
267267

268268
textResourceShouldFail = true;
269-
ReactNoop.expire(50);
270-
await advanceTimers(50);
269+
ReactNoop.expire(1000);
270+
await advanceTimers(1000);
271271
textResourceShouldFail = false;
272272

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

286-
expect(ReactNoop.flush()).toEqual(['Suspend! [Result]', 'Loading...']);
287-
ReactNoop.expire(100);
288-
await advanceTimers(100);
286+
expect(ReactNoop.flush()).toEqual(['Suspend! [Result]']);
287+
ReactNoop.expire(3000);
288+
await advanceTimers(3000);
289289
expect(ReactNoop.flush()).toEqual(['Promise resolved [Result]', 'Result']);
290290
expect(ReactNoop.getChildren()).toEqual([span('Result')]);
291291
});
@@ -495,8 +495,8 @@ describe('ReactSuspense', () => {
495495

496496
// Expire the outer timeout, but don't expire the inner one.
497497
// We should see the outer loading placeholder.
498-
ReactNoop.expire(1000);
499-
await advanceTimers(1000);
498+
ReactNoop.expire(1500);
499+
await advanceTimers(1500);
500500
expect(ReactNoop.flush()).toEqual([
501501
'Sync',
502502
// Still suspended.
@@ -600,8 +600,8 @@ describe('ReactSuspense', () => {
600600
it('expires early with a `timeout` option', async () => {
601601
ReactNoop.render(
602602
<Fragment>
603-
<Fallback timeout={100} placeholder={<Text text="Loading..." />}>
604-
<AsyncText text="Async" ms={1000} />
603+
<Fallback timeout={1000} placeholder={<Text text="Loading..." />}>
604+
<AsyncText text="Async" ms={3000} />
605605
</Fallback>
606606
<Text text="Sync" />
607607
</Fragment>,
@@ -619,8 +619,8 @@ describe('ReactSuspense', () => {
619619
// Advance both React's virtual time and Jest's timers by enough to trigger
620620
// the timeout, but not by enough to flush the promise or reach the true
621621
// expiration time.
622-
ReactNoop.expire(120);
623-
await advanceTimers(120);
622+
ReactNoop.expire(2000);
623+
await advanceTimers(2000);
624624
expect(ReactNoop.flush()).toEqual([
625625
// Still suspended.
626626
'Suspend! [Async]',
@@ -763,7 +763,7 @@ describe('ReactSuspense', () => {
763763
{didTimeout => (
764764
<Fragment>
765765
<div hidden={didTimeout}>
766-
<AsyncText text="Async" ms={2000} />
766+
<AsyncText text="Async" ms={3000} />
767767
</div>
768768
{didTimeout ? <Text text="Loading..." /> : null}
769769
</Fragment>
@@ -776,8 +776,8 @@ describe('ReactSuspense', () => {
776776
expect(ReactNoop.flush()).toEqual(['Suspend! [Async]']);
777777
expect(ReactNoop.getChildren()).toEqual([]);
778778

779-
ReactNoop.expire(1000);
780-
await advanceTimers(1000);
779+
ReactNoop.expire(2000);
780+
await advanceTimers(2000);
781781
expect(ReactNoop.flush()).toEqual([
782782
'Suspend! [Async]',
783783
'Loading...',
@@ -932,13 +932,13 @@ describe('ReactSuspense', () => {
932932
ReactNoop.flush();
933933
expect(ReactNoop.getChildren()).toEqual([]);
934934

935-
await advanceTimers(999);
936-
ReactNoop.expire(999);
935+
await advanceTimers(800);
936+
ReactNoop.expire(800);
937937
ReactNoop.flush();
938938
expect(ReactNoop.getChildren()).toEqual([]);
939939

940-
await advanceTimers(1);
941-
ReactNoop.expire(1);
940+
await advanceTimers(1000);
941+
ReactNoop.expire(1000);
942942
ReactNoop.flush();
943943
expect(ReactNoop.getChildren()).toEqual([span('A')]);
944944
});

0 commit comments

Comments
 (0)