From 0803d2247960c6535cd89cb24cd29dd04dd1e0cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Fri, 10 May 2019 10:53:20 -0700 Subject: [PATCH] Don't consider "Never" expiration as part of most recent event time (#15606) * Don't consider "Never" expiration as part of most recent event time This doesn't happen with deprioritization since those are not "updates" by themselves so they don't go into this accounting. However, they are real updates if they were scheduled as Idle pri using the scheduler explicitly. It's unclear what suspense should do for these updates. For offscreen work, we probably want them to commit immediately. No point in delay them since they're offscreen anyway. However if this is an explicit but very low priority update that might not make sense. So maybe this means that these should have different expiration times? In this PR I just set the suspense to the lowest JND. However, we don't want is for these things to commit earlier in case they got batched in with other work so I also ensured that they're not accounted for in in the workInProgressRootMostRecentEventTime calculation at all. This makes them commit immediately if they're by themselves, or after the JND of whatever they were batched in with. Ultimately, I think that we should probably never schedule anything at Never that isn't truly offscreen so this should never happen. However, that begs the question what happens with very low pri work that suspends. Do we always work at that level first? * Adjust test to account for the new shorter suspense time --- .../src/ReactFiberScheduler.js | 11 ++++- .../ReactSuspensePlaceholder-test.internal.js | 6 +-- ...tSuspenseWithNoopRenderer-test.internal.js | 45 +++++++++++++++++++ 3 files changed, 57 insertions(+), 5 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberScheduler.js b/packages/react-reconciler/src/ReactFiberScheduler.js index f748228f8e..7864aa0f9a 100644 --- a/packages/react-reconciler/src/ReactFiberScheduler.js +++ b/packages/react-reconciler/src/ReactFiberScheduler.js @@ -936,7 +936,10 @@ function renderRoot( } export function markRenderEventTime(expirationTime: ExpirationTime): void { - if (expirationTime < workInProgressRootMostRecentEventTime) { + if ( + expirationTime < workInProgressRootMostRecentEventTime && + expirationTime > Never + ) { workInProgressRootMostRecentEventTime = expirationTime; } } @@ -1866,7 +1869,11 @@ function computeMsUntilTimeout( const eventTimeMs: number = inferTimeFromExpirationTime(mostRecentEventTime); const currentTimeMs: number = now(); - const timeElapsed = currentTimeMs - eventTimeMs; + let timeElapsed = currentTimeMs - eventTimeMs; + if (timeElapsed < 0) { + // We get this wrong some time since we estimate the time. + timeElapsed = 0; + } let msUntilTimeout = jnd(timeElapsed) - timeElapsed; diff --git a/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js index 7060f42730..40af1ba4c2 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js @@ -459,7 +459,7 @@ describe('ReactSuspensePlaceholder', () => { expect(ReactNoop).toMatchRenderedOutput('Text'); // Show the fallback UI. - jest.advanceTimersByTime(750); + jest.advanceTimersByTime(900); expect(ReactNoop).toMatchRenderedOutput('Loading...'); expect(onRender).toHaveBeenCalledTimes(2); @@ -479,7 +479,7 @@ describe('ReactSuspensePlaceholder', () => { - + , ); @@ -495,7 +495,7 @@ describe('ReactSuspensePlaceholder', () => { expect(onRender).toHaveBeenCalledTimes(2); // Resolve the pending promise. - jest.advanceTimersByTime(250); + jest.advanceTimersByTime(100); expect(Scheduler).toHaveYielded([ 'Promise resolved [Loaded]', 'Promise resolved [Sibling]', diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js index 5e0e9cac07..87e86b9495 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js @@ -1841,4 +1841,49 @@ describe('ReactSuspenseWithNoopRenderer', () => { expect(Scheduler).toFlushAndYield(['A', 'C']); expect(ReactNoop.getChildren()).toEqual([span('A'), span('C'), span('B')]); }); + + it('commits a suspended idle pri render within a reasonable time', async () => { + function Foo({something}) { + return ( + + }> + + + + ); + } + + ReactNoop.render(); + + // Took a long time to render. This is to ensure we get a long suspense time. + // Could also use something like suspendIfNeeded to simulate this. + Scheduler.advanceTime(1500); + await advanceTimers(1500); + + expect(Scheduler).toFlushAndYield(['Suspend! [A]', 'Loading A...']); + // We're still suspended. + expect(ReactNoop.getChildren()).toEqual([]); + + // Schedule an update at idle pri. + Scheduler.unstable_runWithPriority(Scheduler.unstable_IdlePriority, () => + ReactNoop.render(), + ); + expect(Scheduler).toFlushAndYield(['Suspend! [A]', 'Loading A...']); + + // We're still suspended. + expect(ReactNoop.getChildren()).toEqual([]); + + // Advance time a little bit. + Scheduler.advanceTime(150); + await advanceTimers(150); + + // We should not have committed yet because we had a long suspense time. + expect(ReactNoop.getChildren()).toEqual([]); + + // Flush to skip suspended time. + Scheduler.advanceTime(600); + await advanceTimers(600); + + expect(ReactNoop.getChildren()).toEqual([span('Loading A...')]); + }); });