From 8b418589d274b240d7dd3d822a40a4e650026e88 Mon Sep 17 00:00:00 2001 From: Jan Kassens Date: Mon, 17 Apr 2023 16:24:23 -0400 Subject: [PATCH] Revert "Remove JND delay for non-transition updates (#26597)" This reverts commit 0b931f90e8964183f08ac328e7350d847abb08f9. --- .../__tests__/ReactCacheOld-test.internal.js | 4 +- .../src/ReactFiberWorkLoop.js | 60 +++ .../src/__tests__/ReactExpiration-test.js | 8 +- .../ReactHooksWithNoopRenderer-test.js | 41 +- .../src/__tests__/ReactLazy-test.internal.js | 18 +- .../__tests__/ReactOffscreenSuspense-test.js | 72 ++- .../__tests__/ReactSuspense-test.internal.js | 8 +- .../ReactSuspenseEffectsSemantics-test.js | 129 ++++- .../ReactSuspensePlaceholder-test.internal.js | 15 +- .../ReactSuspenseWithNoopRenderer-test.js | 502 ++++++++++++++++-- 10 files changed, 743 insertions(+), 114 deletions(-) diff --git a/packages/react-cache/src/__tests__/ReactCacheOld-test.internal.js b/packages/react-cache/src/__tests__/ReactCacheOld-test.internal.js index 579c835878eb8..117c0b488b178 100644 --- a/packages/react-cache/src/__tests__/ReactCacheOld-test.internal.js +++ b/packages/react-cache/src/__tests__/ReactCacheOld-test.internal.js @@ -236,7 +236,7 @@ describe('ReactCache', () => { jest.advanceTimersByTime(100); assertLog(['Promise resolved [4]']); - await waitForAll([1, 4, 'Suspend! [5]']); + await waitForAll([1, 4, 'Suspend! [5]', 'Loading...']); jest.advanceTimersByTime(100); assertLog(['Promise resolved [5]']); @@ -264,7 +264,7 @@ describe('ReactCache', () => { ]); jest.advanceTimersByTime(100); assertLog(['Promise resolved [2]']); - await waitForAll([1, 2, 'Suspend! [3]']); + await waitForAll([1, 2, 'Suspend! [3]', 'Loading...']); jest.advanceTimersByTime(100); assertLog(['Promise resolved [3]']); diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index d75cc2c47ff15..94af7e44d59e1 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -145,6 +145,7 @@ import { includesExpiredLane, getNextLanes, getLanesToRetrySynchronouslyOnError, + getMostRecentEventTime, markRootUpdated, markRootSuspended as markRootSuspended_dontCallThisOneDirectly, markRootPinged, @@ -283,6 +284,8 @@ import { } from './ReactFiberRootScheduler'; import {getMaskedContext, getUnmaskedContext} from './ReactFiberContext'; +const ceil = Math.ceil; + const PossiblyWeakMap = typeof WeakMap === 'function' ? WeakMap : Map; const { @@ -1190,6 +1193,38 @@ function finishConcurrentRender( break; } + if (!shouldForceFlushFallbacksInDEV()) { + // This is not a transition, but we did trigger an avoided state. + // Schedule a placeholder to display after a short delay, using the Just + // Noticeable Difference. + // TODO: Is the JND optimization worth the added complexity? If this is + // the only reason we track the event time, then probably not. + // Consider removing. + + const mostRecentEventTime = getMostRecentEventTime(root, lanes); + const eventTimeMs = mostRecentEventTime; + const timeElapsedMs = now() - eventTimeMs; + const msUntilTimeout = jnd(timeElapsedMs) - timeElapsedMs; + + // Don't bother with a very short suspense time. + if (msUntilTimeout > 10) { + // Instead of committing the fallback immediately, wait for more data + // to arrive. + root.timeoutHandle = scheduleTimeout( + commitRootWhenReady.bind( + null, + root, + finishedWork, + workInProgressRootRecoverableErrors, + workInProgressTransitions, + lanes, + ), + msUntilTimeout, + ); + break; + } + } + // Commit the placeholder. commitRootWhenReady( root, @@ -3545,6 +3580,31 @@ export function resolveRetryWakeable(boundaryFiber: Fiber, wakeable: Wakeable) { retryTimedOutBoundary(boundaryFiber, retryLane); } +// Computes the next Just Noticeable Difference (JND) boundary. +// The theory is that a person can't tell the difference between small differences in time. +// Therefore, if we wait a bit longer than necessary that won't translate to a noticeable +// difference in the experience. However, waiting for longer might mean that we can avoid +// showing an intermediate loading state. The longer we have already waited, the harder it +// is to tell small differences in time. Therefore, the longer we've already waited, +// the longer we can wait additionally. At some point we have to give up though. +// We pick a train model where the next boundary commits at a consistent schedule. +// These particular numbers are vague estimates. We expect to adjust them based on research. +function jnd(timeElapsed: number) { + return timeElapsed < 120 + ? 120 + : timeElapsed < 480 + ? 480 + : timeElapsed < 1080 + ? 1080 + : timeElapsed < 1920 + ? 1920 + : timeElapsed < 3000 + ? 3000 + : timeElapsed < 4320 + ? 4320 + : ceil(timeElapsed / 1960) * 1960; +} + export function throwIfInfiniteUpdateLoopDetected() { if (nestedUpdateCount > NESTED_UPDATE_LIMIT) { nestedUpdateCount = 0; diff --git a/packages/react-reconciler/src/__tests__/ReactExpiration-test.js b/packages/react-reconciler/src/__tests__/ReactExpiration-test.js index 653bc304600bb..2c38a05168a6c 100644 --- a/packages/react-reconciler/src/__tests__/ReactExpiration-test.js +++ b/packages/react-reconciler/src/__tests__/ReactExpiration-test.js @@ -732,9 +732,13 @@ describe('ReactExpiration', () => { expect(root).toMatchRenderedOutput('A0BC'); await act(async () => { - React.startTransition(() => { + if (gate(flags => flags.enableSyncDefaultUpdates)) { + React.startTransition(() => { + root.render(); + }); + } else { root.render(); - }); + } await waitForAll(['Suspend! [A1]', 'Loading...']); // Lots of time elapses before the promise resolves diff --git a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js index 1c7b25a2c9175..627dc4e856216 100644 --- a/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js +++ b/packages/react-reconciler/src/__tests__/ReactHooksWithNoopRenderer-test.js @@ -692,16 +692,24 @@ describe('ReactHooksWithNoopRenderer', () => { await waitForAll([0]); expect(root).toMatchRenderedOutput(); - React.startTransition(() => { + if (gate(flags => flags.enableSyncDefaultUpdates)) { + React.startTransition(() => { + root.render(); + }); + } else { root.render(); - }); + } await waitForAll(['Suspend!']); expect(root).toMatchRenderedOutput(); // Rendering again should suspend again. - React.startTransition(() => { + if (gate(flags => flags.enableSyncDefaultUpdates)) { + React.startTransition(() => { + root.render(); + }); + } else { root.render(); - }); + } await waitForAll(['Suspend!']); }); @@ -747,25 +755,38 @@ describe('ReactHooksWithNoopRenderer', () => { expect(root).toMatchRenderedOutput(); await act(async () => { - React.startTransition(() => { + if (gate(flags => flags.enableSyncDefaultUpdates)) { + React.startTransition(() => { + root.render(); + setLabel('B'); + }); + } else { root.render(); setLabel('B'); - }); + } await waitForAll(['Suspend!']); expect(root).toMatchRenderedOutput(); // Rendering again should suspend again. - React.startTransition(() => { + if (gate(flags => flags.enableSyncDefaultUpdates)) { + React.startTransition(() => { + root.render(); + }); + } else { root.render(); - }); + } await waitForAll(['Suspend!']); // Flip the signal back to "cancel" the update. However, the update to // label should still proceed. It shouldn't have been dropped. - React.startTransition(() => { + if (gate(flags => flags.enableSyncDefaultUpdates)) { + React.startTransition(() => { + root.render(); + }); + } else { root.render(); - }); + } await waitForAll(['B:0']); expect(root).toMatchRenderedOutput(); }); diff --git a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js index bd4c534a781b8..0ebf8f2d53293 100644 --- a/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactLazy-test.internal.js @@ -1414,12 +1414,10 @@ describe('ReactLazy', () => { // Swap the position of A and B root.update(); - await waitForAll([ - 'Init B2', - 'Loading...', - 'Did unmount: A', - 'Did unmount: B', - ]); + await waitForAll(['Init B2', 'Loading...']); + jest.runAllTimers(); + + assertLog(['Did unmount: A', 'Did unmount: B']); // The suspense boundary should've triggered now. expect(root).toMatchRenderedOutput('Loading...'); @@ -1561,9 +1559,13 @@ describe('ReactLazy', () => { expect(root).toMatchRenderedOutput('AB'); // Swap the position of A and B - React.startTransition(() => { + if (gate(flags => flags.enableSyncDefaultUpdates)) { + React.startTransition(() => { + root.update(); + }); + } else { root.update(); - }); + } await waitForAll(['Init B2', 'Loading...']); await resolveFakeImport(ChildB2); // We need to flush to trigger the second one to load. diff --git a/packages/react-reconciler/src/__tests__/ReactOffscreenSuspense-test.js b/packages/react-reconciler/src/__tests__/ReactOffscreenSuspense-test.js index c73e3caf62c64..3a09abd9b10b7 100644 --- a/packages/react-reconciler/src/__tests__/ReactOffscreenSuspense-test.js +++ b/packages/react-reconciler/src/__tests__/ReactOffscreenSuspense-test.js @@ -9,7 +9,6 @@ let useState; let useEffect; let startTransition; let textCache; -let waitFor; let waitForPaint; let assertLog; @@ -29,7 +28,6 @@ describe('ReactOffscreen', () => { startTransition = React.startTransition; const InternalTestUtils = require('internal-test-utils'); - waitFor = InternalTestUtils.waitFor; waitForPaint = InternalTestUtils.waitForPaint; assertLog = InternalTestUtils.assertLog; @@ -409,6 +407,7 @@ describe('ReactOffscreen', () => { expect(root).toMatchRenderedOutput(); }); + // Only works in new reconciler // @gate enableOffscreen test('detect updates to a hidden tree during a concurrent event', async () => { // This is a pretty complex test case. It relates to how we detect if an @@ -443,17 +442,17 @@ describe('ReactOffscreen', () => { setOuter = _setOuter; return ( <> + + + - - - }> - + @@ -467,41 +466,50 @@ describe('ReactOffscreen', () => { root.render(); }); assertLog([ - 'Inner: 0', 'Outer: 0', - 'Sibling: 0', + 'Inner: 0', + 'Async: 0', 'Inner and outer are consistent', ]); expect(root).toMatchRenderedOutput( <> - Inner: 0 Outer: 0 - Sibling: 0 + Inner: 0 + Async: 0 , ); await act(async () => { // Update a value both inside and outside the hidden tree. These values // must always be consistent. - startTransition(() => { - setOuter(1); - setInner(1); - // In the same render, also hide the offscreen tree. - root.render(); - }); + setOuter(1); + setInner(1); + // In the same render, also hide the offscreen tree. + root.render(); - await waitFor([ + await waitForPaint([ // The outer update will commit, but the inner update is deferred until // a later render. 'Outer: 1', + + // Something suspended. This means we won't commit immediately; there + // will be an async gap between render and commit. In this test, we will + // use this property to schedule a concurrent update. The fact that + // we're using Suspense to schedule a concurrent update is not directly + // relevant to the test — we could also use time slicing, but I've + // chosen to use Suspense the because implementation details of time + // slicing are more volatile. + 'Suspend! [Async: 1]', + + 'Loading...', ]); // Assert that we haven't committed quite yet expect(root).toMatchRenderedOutput( <> - Inner: 0 Outer: 0 - Sibling: 0 + Inner: 0 + Async: 0 , ); @@ -512,13 +520,14 @@ describe('ReactOffscreen', () => { setInner(2); }); - // Finish rendering and commit the in-progress render. - await waitForPaint(['Sibling: 1']); + // Commit the previous render. + jest.runAllTimers(); expect(root).toMatchRenderedOutput( <> - Outer: 1 - Sibling: 1 + + + Loading... , ); @@ -527,27 +536,32 @@ describe('ReactOffscreen', () => { root.render(); }); assertLog([ + 'Outer: 1', + // There are two pending updates on Inner, but only the first one // is processed, even though they share the same lane. If the second // update were erroneously processed, then Inner would be inconsistent // with Outer. 'Inner: 1', - 'Outer: 1', - 'Sibling: 1', + + 'Suspend! [Async: 1]', + 'Loading...', 'Inner and outer are consistent', ]); }); assertLog([ - 'Inner: 2', 'Outer: 2', - 'Sibling: 2', + 'Inner: 2', + 'Suspend! [Async: 2]', + 'Loading...', 'Inner and outer are consistent', ]); expect(root).toMatchRenderedOutput( <> - Inner: 2 Outer: 2 - Sibling: 2 + Inner: 2 + + Loading... , ); }); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js index 75827aa409368..5723e0039e2eb 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js @@ -125,9 +125,13 @@ describe('ReactSuspense', () => { // Navigate the shell to now render the child content. // This should suspend. - React.startTransition(() => { + if (gate(flags => flags.enableSyncDefaultUpdates)) { + React.startTransition(() => { + root.update(); + }); + } else { root.update(); - }); + } await waitForAll([ 'Foo', diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js index 5200635b6fca4..a684f6ab4d245 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseEffectsSemantics-test.js @@ -576,7 +576,7 @@ describe('ReactSuspenseEffectsSemantics', () => { ]); }); - // @gate enableLegacyCache + // @gate enableLegacyCache && enableSyncDefaultUpdates it('should be destroyed and recreated for function components', async () => { function App({children = null}) { Scheduler.log('App render'); @@ -642,6 +642,19 @@ describe('ReactSuspenseEffectsSemantics', () => { 'Suspend:Async', 'Text:Fallback render', 'Text:Outside render', + ]); + expect(ReactNoop).toMatchRenderedOutput( + <> + + + + , + ); + + await jest.runAllTimers(); + + // Timing out should commit the fallback and destroy inner layout effects. + assertLog([ 'Text:Inside:Before destroy layout', 'Text:Inside:After destroy layout', 'Text:Fallback create layout', @@ -698,7 +711,7 @@ describe('ReactSuspenseEffectsSemantics', () => { ]); }); - // @gate enableLegacyCache + // @gate enableLegacyCache && enableSyncDefaultUpdates it('should be destroyed and recreated for class components', async () => { class ClassText extends React.Component { componentDidMount() { @@ -783,6 +796,19 @@ describe('ReactSuspenseEffectsSemantics', () => { 'Suspend:Async', 'ClassText:Fallback render', 'ClassText:Outside render', + ]); + expect(ReactNoop).toMatchRenderedOutput( + <> + + + + , + ); + + await jest.runAllTimers(); + + // Timing out should commit the fallback and destroy inner layout effects. + assertLog([ 'ClassText:Inside:Before componentWillUnmount', 'ClassText:Inside:After componentWillUnmount', 'ClassText:Fallback componentDidMount', @@ -834,7 +860,7 @@ describe('ReactSuspenseEffectsSemantics', () => { ]); }); - // @gate enableLegacyCache + // @gate enableLegacyCache && enableSyncDefaultUpdates it('should be destroyed and recreated when nested below host components', async () => { function App({children = null}) { Scheduler.log('App render'); @@ -888,10 +914,17 @@ describe('ReactSuspenseEffectsSemantics', () => { , ); - await waitFor([ - 'App render', - 'Suspend:Async', - 'Text:Fallback render', + await waitFor(['App render', 'Suspend:Async', 'Text:Fallback render']); + expect(ReactNoop).toMatchRenderedOutput( + + + , + ); + + await jest.runAllTimers(); + + // Timing out should commit the fallback and destroy inner layout effects. + assertLog([ 'Text:Outer destroy layout', 'Text:Inner destroy layout', 'Text:Fallback create layout', @@ -946,7 +979,7 @@ describe('ReactSuspenseEffectsSemantics', () => { ]); }); - // @gate enableLegacyCache + // @gate enableLegacyCache && enableSyncDefaultUpdates it('should be destroyed and recreated even if there is a bailout because of memoization', async () => { const MemoizedText = React.memo(Text, () => true); @@ -1007,6 +1040,18 @@ describe('ReactSuspenseEffectsSemantics', () => { 'Suspend:Async', // Text:MemoizedInner is memoized 'Text:Fallback render', + ]); + expect(ReactNoop).toMatchRenderedOutput( + + + , + ); + + await jest.runAllTimers(); + + // Timing out should commit the fallback and destroy inner layout effects. + // Even though the innermost layout effects are beneath a hidden HostComponent. + assertLog([ 'Text:Outer destroy layout', 'Text:MemoizedInner destroy layout', 'Text:Fallback create layout', @@ -1403,7 +1448,7 @@ describe('ReactSuspenseEffectsSemantics', () => { ); }); - // @gate enableLegacyCache + // @gate enableLegacyCache && enableSyncDefaultUpdates it('should be cleaned up inside of a fallback that suspends', async () => { function App({fallbackChildren = null, outerChildren = null}) { return ( @@ -1456,6 +1501,17 @@ describe('ReactSuspenseEffectsSemantics', () => { 'Text:Fallback:Inside render', 'Text:Fallback:Outside render', 'Text:Outside render', + ]); + expect(ReactNoop).toMatchRenderedOutput( + <> + + + , + ); + + // Timing out should commit the fallback and destroy inner layout effects. + await jest.runAllTimers(); + assertLog([ 'Text:Inside destroy layout', 'Text:Fallback:Inside create layout', 'Text:Fallback:Outside create layout', @@ -1490,6 +1546,19 @@ describe('ReactSuspenseEffectsSemantics', () => { 'Text:Fallback:Fallback render', 'Text:Fallback:Outside render', 'Text:Outside render', + ]); + expect(ReactNoop).toMatchRenderedOutput( + <> +