Skip to content

Commit 8b41858

Browse files
kassenskassens
committed
Revert "Remove JND delay for non-transition updates (facebook#26597)"
This reverts commit 0b931f9.
1 parent 110f762 commit 8b41858

10 files changed

+743
-114
lines changed

‎packages/react-cache/src/__tests__/ReactCacheOld-test.internal.js

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -236,7 +236,7 @@ describe('ReactCache', () => {
236236

237237
jest.advanceTimersByTime(100);
238238
assertLog(['Promise resolved [4]']);
239-
await waitForAll([1, 4, 'Suspend! [5]']);
239+
await waitForAll([1, 4, 'Suspend! [5]', 'Loading...']);
240240

241241
jest.advanceTimersByTime(100);
242242
assertLog(['Promise resolved [5]']);
@@ -264,7 +264,7 @@ describe('ReactCache', () => {
264264
]);
265265
jest.advanceTimersByTime(100);
266266
assertLog(['Promise resolved [2]']);
267-
await waitForAll([1, 2, 'Suspend! [3]']);
267+
await waitForAll([1, 2, 'Suspend! [3]', 'Loading...']);
268268

269269
jest.advanceTimersByTime(100);
270270
assertLog(['Promise resolved [3]']);

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

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -145,6 +145,7 @@ import {
145145
includesExpiredLane,
146146
getNextLanes,
147147
getLanesToRetrySynchronouslyOnError,
148+
getMostRecentEventTime,
148149
markRootUpdated,
149150
markRootSuspended as markRootSuspended_dontCallThisOneDirectly,
150151
markRootPinged,
@@ -283,6 +284,8 @@ import {
283284
} from './ReactFiberRootScheduler';
284285
import {getMaskedContext, getUnmaskedContext} from './ReactFiberContext';
285286

287+
const ceil = Math.ceil;
288+
286289
const PossiblyWeakMap = typeof WeakMap === 'function' ? WeakMap : Map;
287290

288291
const {
@@ -1190,6 +1193,38 @@ function finishConcurrentRender(
11901193
break;
11911194
}
11921195

1196+
if (!shouldForceFlushFallbacksInDEV()) {
1197+
// This is not a transition, but we did trigger an avoided state.
1198+
// Schedule a placeholder to display after a short delay, using the Just
1199+
// Noticeable Difference.
1200+
// TODO: Is the JND optimization worth the added complexity? If this is
1201+
// the only reason we track the event time, then probably not.
1202+
// Consider removing.
1203+
1204+
const mostRecentEventTime = getMostRecentEventTime(root, lanes);
1205+
const eventTimeMs = mostRecentEventTime;
1206+
const timeElapsedMs = now() - eventTimeMs;
1207+
const msUntilTimeout = jnd(timeElapsedMs) - timeElapsedMs;
1208+
1209+
// Don't bother with a very short suspense time.
1210+
if (msUntilTimeout > 10) {
1211+
// Instead of committing the fallback immediately, wait for more data
1212+
// to arrive.
1213+
root.timeoutHandle = scheduleTimeout(
1214+
commitRootWhenReady.bind(
1215+
null,
1216+
root,
1217+
finishedWork,
1218+
workInProgressRootRecoverableErrors,
1219+
workInProgressTransitions,
1220+
lanes,
1221+
),
1222+
msUntilTimeout,
1223+
);
1224+
break;
1225+
}
1226+
}
1227+
11931228
// Commit the placeholder.
11941229
commitRootWhenReady(
11951230
root,
@@ -3545,6 +3580,31 @@ export function resolveRetryWakeable(boundaryFiber: Fiber, wakeable: Wakeable) {
35453580
retryTimedOutBoundary(boundaryFiber, retryLane);
35463581
}
35473582

3583+
// Computes the next Just Noticeable Difference (JND) boundary.
3584+
// The theory is that a person can't tell the difference between small differences in time.
3585+
// Therefore, if we wait a bit longer than necessary that won't translate to a noticeable
3586+
// difference in the experience. However, waiting for longer might mean that we can avoid
3587+
// showing an intermediate loading state. The longer we have already waited, the harder it
3588+
// is to tell small differences in time. Therefore, the longer we've already waited,
3589+
// the longer we can wait additionally. At some point we have to give up though.
3590+
// We pick a train model where the next boundary commits at a consistent schedule.
3591+
// These particular numbers are vague estimates. We expect to adjust them based on research.
3592+
function jnd(timeElapsed: number) {
3593+
return timeElapsed < 120
3594+
? 120
3595+
: timeElapsed < 480
3596+
? 480
3597+
: timeElapsed < 1080
3598+
? 1080
3599+
: timeElapsed < 1920
3600+
? 1920
3601+
: timeElapsed < 3000
3602+
? 3000
3603+
: timeElapsed < 4320
3604+
? 4320
3605+
: ceil(timeElapsed / 1960) * 1960;
3606+
}
3607+
35483608
export function throwIfInfiniteUpdateLoopDetected() {
35493609
if (nestedUpdateCount > NESTED_UPDATE_LIMIT) {
35503610
nestedUpdateCount = 0;

‎packages/react-reconciler/src/__tests__/ReactExpiration-test.js

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -732,9 +732,13 @@ describe('ReactExpiration', () => {
732732
expect(root).toMatchRenderedOutput('A0BC');
733733

734734
await act(async () => {
735-
React.startTransition(() => {
735+
if (gate(flags => flags.enableSyncDefaultUpdates)) {
736+
React.startTransition(() => {
737+
root.render(<App step={1} />);
738+
});
739+
} else {
736740
root.render(<App step={1} />);
737-
});
741+
}
738742
await waitForAll(['Suspend! [A1]', 'Loading...']);
739743

740744
// Lots of time elapses before the promise resolves

‎packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -692,16 +692,24 @@ describe('ReactHooksWithNoopRenderer', () => {
692692
await waitForAll([0]);
693693
expect(root).toMatchRenderedOutput(<span prop={0} />);
694694

695-
React.startTransition(() => {
695+
if (gate(flags => flags.enableSyncDefaultUpdates)) {
696+
React.startTransition(() => {
697+
root.render(<Foo signal={false} />);
698+
});
699+
} else {
696700
root.render(<Foo signal={false} />);
697-
});
701+
}
698702
await waitForAll(['Suspend!']);
699703
expect(root).toMatchRenderedOutput(<span prop={0} />);
700704

701705
// Rendering again should suspend again.
702-
React.startTransition(() => {
706+
if (gate(flags => flags.enableSyncDefaultUpdates)) {
707+
React.startTransition(() => {
708+
root.render(<Foo signal={false} />);
709+
});
710+
} else {
703711
root.render(<Foo signal={false} />);
704-
});
712+
}
705713
await waitForAll(['Suspend!']);
706714
});
707715

@@ -747,25 +755,38 @@ describe('ReactHooksWithNoopRenderer', () => {
747755
expect(root).toMatchRenderedOutput(<span prop="A:0" />);
748756

749757
await act(async () => {
750-
React.startTransition(() => {
758+
if (gate(flags => flags.enableSyncDefaultUpdates)) {
759+
React.startTransition(() => {
760+
root.render(<Foo signal={false} />);
761+
setLabel('B');
762+
});
763+
} else {
751764
root.render(<Foo signal={false} />);
752765
setLabel('B');
753-
});
766+
}
754767

755768
await waitForAll(['Suspend!']);
756769
expect(root).toMatchRenderedOutput(<span prop="A:0" />);
757770

758771
// Rendering again should suspend again.
759-
React.startTransition(() => {
772+
if (gate(flags => flags.enableSyncDefaultUpdates)) {
773+
React.startTransition(() => {
774+
root.render(<Foo signal={false} />);
775+
});
776+
} else {
760777
root.render(<Foo signal={false} />);
761-
});
778+
}
762779
await waitForAll(['Suspend!']);
763780

764781
// Flip the signal back to "cancel" the update. However, the update to
765782
// label should still proceed. It shouldn't have been dropped.
766-
React.startTransition(() => {
783+
if (gate(flags => flags.enableSyncDefaultUpdates)) {
784+
React.startTransition(() => {
785+
root.render(<Foo signal={true} />);
786+
});
787+
} else {
767788
root.render(<Foo signal={true} />);
768-
});
789+
}
769790
await waitForAll(['B:0']);
770791
expect(root).toMatchRenderedOutput(<span prop="B:0" />);
771792
});

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

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1414,12 +1414,10 @@ describe('ReactLazy', () => {
14141414

14151415
// Swap the position of A and B
14161416
root.update(<Parent swap={true} />);
1417-
await waitForAll([
1418-
'Init B2',
1419-
'Loading...',
1420-
'Did unmount: A',
1421-
'Did unmount: B',
1422-
]);
1417+
await waitForAll(['Init B2', 'Loading...']);
1418+
jest.runAllTimers();
1419+
1420+
assertLog(['Did unmount: A', 'Did unmount: B']);
14231421

14241422
// The suspense boundary should've triggered now.
14251423
expect(root).toMatchRenderedOutput('Loading...');
@@ -1561,9 +1559,13 @@ describe('ReactLazy', () => {
15611559
expect(root).toMatchRenderedOutput('AB');
15621560

15631561
// Swap the position of A and B
1564-
React.startTransition(() => {
1562+
if (gate(flags => flags.enableSyncDefaultUpdates)) {
1563+
React.startTransition(() => {
1564+
root.update(<Parent swap={true} />);
1565+
});
1566+
} else {
15651567
root.update(<Parent swap={true} />);
1566-
});
1568+
}
15671569
await waitForAll(['Init B2', 'Loading...']);
15681570
await resolveFakeImport(ChildB2);
15691571
// We need to flush to trigger the second one to load.

‎packages/react-reconciler/src/__tests__/ReactOffscreenSuspense-test.js

Lines changed: 43 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,6 @@ let useState;
99
let useEffect;
1010
let startTransition;
1111
let textCache;
12-
let waitFor;
1312
let waitForPaint;
1413
let assertLog;
1514

@@ -29,7 +28,6 @@ describe('ReactOffscreen', () => {
2928
startTransition = React.startTransition;
3029

3130
const InternalTestUtils = require('internal-test-utils');
32-
waitFor = InternalTestUtils.waitFor;
3331
waitForPaint = InternalTestUtils.waitForPaint;
3432
assertLog = InternalTestUtils.assertLog;
3533

@@ -409,6 +407,7 @@ describe('ReactOffscreen', () => {
409407
expect(root).toMatchRenderedOutput(<span hidden={true}>B1</span>);
410408
});
411409

410+
// Only works in new reconciler
412411
// @gate enableOffscreen
413412
test('detect updates to a hidden tree during a concurrent event', async () => {
414413
// This is a pretty complex test case. It relates to how we detect if an
@@ -443,17 +442,17 @@ describe('ReactOffscreen', () => {
443442
setOuter = _setOuter;
444443
return (
445444
<>
445+
<span>
446+
<Text text={'Outer: ' + outer} />
447+
</span>
446448
<Offscreen mode={show ? 'visible' : 'hidden'}>
447449
<span>
448450
<Child outer={outer} />
449451
</span>
450452
</Offscreen>
451-
<span>
452-
<Text text={'Outer: ' + outer} />
453-
</span>
454453
<Suspense fallback={<Text text="Loading..." />}>
455454
<span>
456-
<Text text={'Sibling: ' + outer} />
455+
<AsyncText text={'Async: ' + outer} />
457456
</span>
458457
</Suspense>
459458
</>
@@ -467,41 +466,50 @@ describe('ReactOffscreen', () => {
467466
root.render(<App show={true} />);
468467
});
469468
assertLog([
470-
'Inner: 0',
471469
'Outer: 0',
472-
'Sibling: 0',
470+
'Inner: 0',
471+
'Async: 0',
473472
'Inner and outer are consistent',
474473
]);
475474
expect(root).toMatchRenderedOutput(
476475
<>
477-
<span>Inner: 0</span>
478476
<span>Outer: 0</span>
479-
<span>Sibling: 0</span>
477+
<span>Inner: 0</span>
478+
<span>Async: 0</span>
480479
</>,
481480
);
482481

483482
await act(async () => {
484483
// Update a value both inside and outside the hidden tree. These values
485484
// must always be consistent.
486-
startTransition(() => {
487-
setOuter(1);
488-
setInner(1);
489-
// In the same render, also hide the offscreen tree.
490-
root.render(<App show={false} />);
491-
});
485+
setOuter(1);
486+
setInner(1);
487+
// In the same render, also hide the offscreen tree.
488+
root.render(<App show={false} />);
492489

493-
await waitFor([
490+
await waitForPaint([
494491
// The outer update will commit, but the inner update is deferred until
495492
// a later render.
496493
'Outer: 1',
494+
495+
// Something suspended. This means we won't commit immediately; there
496+
// will be an async gap between render and commit. In this test, we will
497+
// use this property to schedule a concurrent update. The fact that
498+
// we're using Suspense to schedule a concurrent update is not directly
499+
// relevant to the test — we could also use time slicing, but I've
500+
// chosen to use Suspense the because implementation details of time
501+
// slicing are more volatile.
502+
'Suspend! [Async: 1]',
503+
504+
'Loading...',
497505
]);
498506

499507
// Assert that we haven't committed quite yet
500508
expect(root).toMatchRenderedOutput(
501509
<>
502-
<span>Inner: 0</span>
503510
<span>Outer: 0</span>
504-
<span>Sibling: 0</span>
511+
<span>Inner: 0</span>
512+
<span>Async: 0</span>
505513
</>,
506514
);
507515

@@ -512,13 +520,14 @@ describe('ReactOffscreen', () => {
512520
setInner(2);
513521
});
514522

515-
// Finish rendering and commit the in-progress render.
516-
await waitForPaint(['Sibling: 1']);
523+
// Commit the previous render.
524+
jest.runAllTimers();
517525
expect(root).toMatchRenderedOutput(
518526
<>
519-
<span hidden={true}>Inner: 0</span>
520527
<span>Outer: 1</span>
521-
<span>Sibling: 1</span>
528+
<span hidden={true}>Inner: 0</span>
529+
<span hidden={true}>Async: 0</span>
530+
Loading...
522531
</>,
523532
);
524533

@@ -527,27 +536,32 @@ describe('ReactOffscreen', () => {
527536
root.render(<App show={true} />);
528537
});
529538
assertLog([
539+
'Outer: 1',
540+
530541
// There are two pending updates on Inner, but only the first one
531542
// is processed, even though they share the same lane. If the second
532543
// update were erroneously processed, then Inner would be inconsistent
533544
// with Outer.
534545
'Inner: 1',
535-
'Outer: 1',
536-
'Sibling: 1',
546+
547+
'Suspend! [Async: 1]',
548+
'Loading...',
537549
'Inner and outer are consistent',
538550
]);
539551
});
540552
assertLog([
541-
'Inner: 2',
542553
'Outer: 2',
543-
'Sibling: 2',
554+
'Inner: 2',
555+
'Suspend! [Async: 2]',
556+
'Loading...',
544557
'Inner and outer are consistent',
545558
]);
546559
expect(root).toMatchRenderedOutput(
547560
<>
548-
<span>Inner: 2</span>
549561
<span>Outer: 2</span>
550-
<span>Sibling: 2</span>
562+
<span>Inner: 2</span>
563+
<span hidden={true}>Async: 0</span>
564+
Loading...
551565
</>,
552566
);
553567
});

0 commit comments

Comments
 (0)