@samitouri / QOS-React-2 / commits / 9dfbd9fa90

use: Don't suspend if there are pending updates

Before suspending, check if there are other pending updates that might possibly unblock the suspended component. If so, interrupt the current render and switch to working on that. This logic was already implemented for the old "throw a Promise" Suspense but has to be replicated for `use` because it suspends the work loop much earlier. I'm getting a little anxious about the divergence between the two Suspense patterns. I'm going to look into enabling the new behavior for the old pattern so that we can unify the implementations.

Andrew Clark committed Nov 3, 2022 at 16:15 UTC 9dfbd9fa90a6f7c1182e2c6dc85956cf7ea3010c
3 files changed +116 -2
packages/react-reconciler/src/ReactFiberWorkLoop.new.js
+13 -1
@@ -1849,7 +1849,7 @@ function handleThrow(root, thrownValue): void {
1849
1850 function shouldAttemptToSuspendUntilDataResolves() {
1851 // TODO: We should be able to move the
1852 - // renderDidSuspend/renderDidSuspendWithDelay logic into this function,
1852 + // renderDidSuspend/renderDidSuspendDelayIfPossible logic into this function,
1853 // instead of repeating it in the complete phase. Or something to that effect.
1854
1855 if (includesOnlyRetries(workInProgressRootRenderLanes)) {
@@ -1857,6 +1857,18 @@ function shouldAttemptToSuspendUntilDataResolves() {
1857 return true;
1858 }
1859
1860 + // Check if there are other pending updates that might possibly unblock this
1861 + // component from suspending. This mirrors the check in
1862 + // renderDidSuspendDelayIfPossible. We should attempt to unify them somehow.
1863 + if (
1864 + includesNonIdleWork(workInProgressRootSkippedLanes) ||
1865 + includesNonIdleWork(workInProgressRootInterleavedUpdatedLanes)
1866 + ) {
1867 + // Suspend normally. renderDidSuspendDelayIfPossible will handle
1868 + // interrupting the work loop.
1869 + return false;
1870 + }
1871 +
1872 // TODO: We should be able to remove the equivalent check in
1873 // finishConcurrentRender, and rely just on this one.
1874 if (includesOnlyTransitions(workInProgressRootRenderLanes)) {
packages/react-reconciler/src/ReactFiberWorkLoop.old.js
+13 -1
@@ -1849,7 +1849,7 @@ function handleThrow(root, thrownValue): void {
1849
1850 function shouldAttemptToSuspendUntilDataResolves() {
1851 // TODO: We should be able to move the
1852 - // renderDidSuspend/renderDidSuspendWithDelay logic into this function,
1852 + // renderDidSuspend/renderDidSuspendDelayIfPossible logic into this function,
1853 // instead of repeating it in the complete phase. Or something to that effect.
1854
1855 if (includesOnlyRetries(workInProgressRootRenderLanes)) {
@@ -1857,6 +1857,18 @@ function shouldAttemptToSuspendUntilDataResolves() {
1857 return true;
1858 }
1859
1860 + // Check if there are other pending updates that might possibly unblock this
1861 + // component from suspending. This mirrors the check in
1862 + // renderDidSuspendDelayIfPossible. We should attempt to unify them somehow.
1863 + if (
1864 + includesNonIdleWork(workInProgressRootSkippedLanes) ||
1865 + includesNonIdleWork(workInProgressRootInterleavedUpdatedLanes)
1866 + ) {
1867 + // Suspend normally. renderDidSuspendDelayIfPossible will handle
1868 + // interrupting the work loop.
1869 + return false;
1870 + }
1871 +
1872 // TODO: We should be able to remove the equivalent check in
1873 // finishConcurrentRender, and rely just on this one.
1874 if (includesOnlyTransitions(workInProgressRootRenderLanes)) {
packages/react-reconciler/src/__tests__/ReactThenable-test.js
+90
@@ -5,6 +5,7 @@ let ReactNoop;
5 let Scheduler;
6 let act;
7 let use;
8 +let useState;
9 let Suspense;
10 let startTransition;
11 let pendingTextRequests;
@@ -18,6 +19,7 @@ describe('ReactThenable', () => {
19 Scheduler = require('scheduler');
20 act = require('jest-react').act;
21 use = React.use;
22 + useState = React.useState;
23 Suspense = React.Suspense;
24 startTransition = React.startTransition;
25
@@ -668,4 +670,92 @@ describe('ReactThenable', () => {
670 expect(Scheduler).toHaveYielded(['Hi']);
671 expect(root).toMatchRenderedOutput('Hi');
672 });
673 +
674 + // @gate enableUseHook
675 + test('does not suspend indefinitely if an interleaved update was skipped', async () => {
676 + function Child({childShouldSuspend}) {
677 + return (
678 + <Text
679 + text={
680 + childShouldSuspend
681 + ? use(getAsyncText('Will never resolve'))
682 + : 'Child'
683 + }
684 + />
685 + );
686 + }
687 +
688 + let setChildShouldSuspend;
689 + let setShowChild;
690 + function Parent() {
691 + const [showChild, _setShowChild] = useState(true);
692 + setShowChild = _setShowChild;
693 +
694 + const [childShouldSuspend, _setChildShouldSuspend] = useState(false);
695 + setChildShouldSuspend = _setChildShouldSuspend;
696 +
697 + Scheduler.unstable_yieldValue(
698 + `childShouldSuspend: ${childShouldSuspend}, showChild: ${showChild}`,
699 + );
700 + return showChild ? (
701 + <Child childShouldSuspend={childShouldSuspend} />
702 + ) : (
703 + <Text text="(empty)" />
704 + );
705 + }
706 +
707 + const root = ReactNoop.createRoot();
708 + await act(() => {
709 + root.render(<Parent />);
710 + });
711 + expect(Scheduler).toHaveYielded([
712 + 'childShouldSuspend: false, showChild: true',
713 + 'Child',
714 + ]);
715 + expect(root).toMatchRenderedOutput('Child');
716 +
717 + await act(() => {
718 + // Perform an update that causes the app to suspend
719 + startTransition(() => {
720 + setChildShouldSuspend(true);
721 + });
722 + expect(Scheduler).toFlushAndYieldThrough([
723 + 'childShouldSuspend: true, showChild: true',
724 + ]);
725 + // While the update is in progress, schedule another update.
726 + startTransition(() => {
727 + setShowChild(false);
728 + });
729 + });
730 + expect(Scheduler).toHaveYielded([
731 + // Because the interleaved update is not higher priority than what we were
732 + // already working on, it won't interrupt. The first update will continue,
733 + // and will suspend.
734 + 'Async text requested [Will never resolve]',
735 +
736 + // Instead of waiting for the promise to resolve, React notices there's
737 + // another pending update that it hasn't tried yet. It will switch to
738 + // rendering that instead.
739 + //
740 + // This time, the update hides the component that previous was suspending,
741 + // so it finishes successfully.
742 + 'childShouldSuspend: false, showChild: false',
743 + '(empty)',
744 +
745 + // Finally, React attempts to render the first update again. It also
746 + // finishes successfully, because it was rebased on top of the update that
747 + // hid the suspended component.
748 + // NOTE: These this render happened to not be entangled with the previous
749 + // one. If they had been, this update would have been included in the
750 + // previous render, and there wouldn't be an extra one here. This could
751 + // change if we change our entanglement heurstics. Semantically, it
752 + // shouldn't matter, though in general we try to work on transitions in
753 + // parallel whenever possible. So even though in this particular case, the
754 + // extra render is unnecessary, it's a nice property that it wasn't
755 + // entangled with the other transition.
756 + 'childShouldSuspend: true, showChild: false',
757 + '(empty)',
758 + ]);
759 + expect(root).toMatchRenderedOutput('(empty)');
760 + });
761 });