@samitouri / QOS-React-2 / commits / 0803d22479

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

Sebastian Markbåge committed May 10, 2019 at 10:53 UTC 0803d2247960c6535cd89cb24cd29dd04dd1e0cc

3 files changed +57 -5

packages/react-reconciler/src/ReactFiberScheduler.js

+9 -2

@@ -936,7 +936,10 @@ function renderRoot(

936 }

937

938 export function markRenderEventTime(expirationTime: ExpirationTime): void {

939 - if (expirationTime < workInProgressRootMostRecentEventTime) {

939 + if (

940 + expirationTime < workInProgressRootMostRecentEventTime &&

941 + expirationTime > Never

942 + ) {

943 workInProgressRootMostRecentEventTime = expirationTime;

944 }

945 }

@@ -1866,7 +1869,11 @@ function computeMsUntilTimeout(

1869

1870 const eventTimeMs: number = inferTimeFromExpirationTime(mostRecentEventTime);

1871 const currentTimeMs: number = now();

1869 - const timeElapsed = currentTimeMs - eventTimeMs;

1872 + let timeElapsed = currentTimeMs - eventTimeMs;

1873 + if (timeElapsed < 0) {

1874 + // We get this wrong some time since we estimate the time.

1875 + timeElapsed = 0;

1876 + }

1877

1878 let msUntilTimeout = jnd(timeElapsed) - timeElapsed;

1879

packages/react-reconciler/src/__tests__/ReactSuspensePlaceholder-test.internal.js

+3 -3

@@ -459,7 +459,7 @@ describe('ReactSuspensePlaceholder', () => {

459 expect(ReactNoop).toMatchRenderedOutput('Text');

460

461 // Show the fallback UI.

462 - jest.advanceTimersByTime(750);

462 + jest.advanceTimersByTime(900);

463 expect(ReactNoop).toMatchRenderedOutput('Loading...');

464 expect(onRender).toHaveBeenCalledTimes(2);

465

@@ -479,7 +479,7 @@ describe('ReactSuspensePlaceholder', () => {

479 <React.Fragment>

480 <App shouldSuspend={true} text="New" textRenderDuration={6} />

481 <Suspense fallback={null}>

482 - <AsyncText ms={250} text="Sibling" fakeRenderDuration={1} />

482 + <AsyncText ms={100} text="Sibling" fakeRenderDuration={1} />

483 </Suspense>

484 </React.Fragment>,

485 );

@@ -495,7 +495,7 @@ describe('ReactSuspensePlaceholder', () => {

495 expect(onRender).toHaveBeenCalledTimes(2);

496

497 // Resolve the pending promise.

498 - jest.advanceTimersByTime(250);

498 + jest.advanceTimersByTime(100);

499 expect(Scheduler).toHaveYielded([

500 'Promise resolved [Loaded]',

501 'Promise resolved [Sibling]',

packages/react-reconciler/src/__tests__/ReactSuspenseWithNoopRenderer-test.internal.js

+45

@@ -1841,4 +1841,49 @@ describe('ReactSuspenseWithNoopRenderer', () => {

1841 expect(Scheduler).toFlushAndYield(['A', 'C']);

1842 expect(ReactNoop.getChildren()).toEqual([span('A'), span('C'), span('B')]);

1843 });

1844 +

1845 + it('commits a suspended idle pri render within a reasonable time', async () => {

1846 + function Foo({something}) {

1847 + return (

1848 + <Fragment>

1849 + <Suspense fallback={<Text text="Loading A..." />}>

1850 + <AsyncText text="A" ms={10000} />

1851 + </Suspense>

1852 + </Fragment>

1853 + );

1854 + }

1855 +

1856 + ReactNoop.render(<Foo />);

1857 +

1858 + // Took a long time to render. This is to ensure we get a long suspense time.

1859 + // Could also use something like suspendIfNeeded to simulate this.

1860 + Scheduler.advanceTime(1500);

1861 + await advanceTimers(1500);

1862 +

1863 + expect(Scheduler).toFlushAndYield(['Suspend! [A]', 'Loading A...']);

1864 + // We're still suspended.

1865 + expect(ReactNoop.getChildren()).toEqual([]);

1866 +

1867 + // Schedule an update at idle pri.

1868 + Scheduler.unstable_runWithPriority(Scheduler.unstable_IdlePriority, () =>

1869 + ReactNoop.render(<Foo something={true} />),

1870 + );

1871 + expect(Scheduler).toFlushAndYield(['Suspend! [A]', 'Loading A...']);

1872 +

1873 + // We're still suspended.

1874 + expect(ReactNoop.getChildren()).toEqual([]);

1875 +

1876 + // Advance time a little bit.

1877 + Scheduler.advanceTime(150);

1878 + await advanceTimers(150);

1879 +

1880 + // We should not have committed yet because we had a long suspense time.

1881 + expect(ReactNoop.getChildren()).toEqual([]);

1882 +

1883 + // Flush to skip suspended time.

1884 + Scheduler.advanceTime(600);

1885 + await advanceTimers(600);

1886 +

1887 + expect(ReactNoop.getChildren()).toEqual([span('Loading A...')]);

1888 + });

1889 });