Bugfix: Effects should never have higher than normal priority (#16257)

* Bugfix: Priority when effects are flushed early

The priority of passive effects is supposed to be the same as the
priority of the render. This fixes a bug where the priority is sometimes
wrong if the effects are flushed early.

But the priority should really never be higher than Normal Priority.
I'll change that in the next commit.

* Effects never have higher than normal priority

Effects currently have the same priority as the render that spawns them.
This changes the behavior so that effects always have normal priority,
or lower if the render priority is lower (e.g. offscreen prerendering).

The implementation is a bit awkward because of the way `renderRoot`,
`commitRoot`, and `flushPassiveEffects` are split. This is a known
factoring problem that I'm planning to address once 16.9 is released.

Author: acdlite

packages/react-reconciler/src/ReactFiberWorkLoop.js

@@ -40,6 +40,7 @@ import {
   shouldYield,
   requestPaint,
   now,
+  NoPriority,
   ImmediatePriority,
   UserBlockingPriority,
   NormalPriority,
@@ -237,6 +238,7 @@ let legacyErrorBoundariesThatAlreadyFailed: Set<mixed> | null = null;
 
 let rootDoesHavePassiveEffects: boolean = false;
 let rootWithPendingPassiveEffects: FiberRoot | null = null;
+let pendingPassiveEffectsRenderPriority: ReactPriorityLevel = NoPriority;
 let pendingPassiveEffectsExpirationTime: ExpirationTime = NoWork;
 
 let rootsWithPendingDiscreteUpdates: Map<
@@ -1494,20 +1496,23 @@ function resetChildExpirationTime(completedWork: Fiber) {
 }
 
 function commitRoot(root) {
-  runWithPriority(ImmediatePriority, commitRootImpl.bind(null, root));
+  const renderPriorityLevel = getCurrentPriorityLevel();
+  runWithPriority(
+    ImmediatePriority,
+    commitRootImpl.bind(null, root, renderPriorityLevel),
+  );
   // If there are passive effects, schedule a callback to flush them. This goes
   // outside commitRootImpl so that it inherits the priority of the render.
   if (rootWithPendingPassiveEffects !== null) {
-    const priorityLevel = getCurrentPriorityLevel();
-    scheduleCallback(priorityLevel, () => {
+    scheduleCallback(NormalPriority, () => {
       flushPassiveEffects();
       return null;
     });
   }
   return null;
 }
 
-function commitRootImpl(root) {
+function commitRootImpl(root, renderPriorityLevel) {
   flushPassiveEffects();
   flushRenderPhaseStrictModeWarningsInDEV();
 
@@ -1730,6 +1735,7 @@ function commitRootImpl(root) {
     rootDoesHavePassiveEffects = false;
     rootWithPendingPassiveEffects = root;
     pendingPassiveEffectsExpirationTime = expirationTime;
+    pendingPassiveEffectsRenderPriority = renderPriorityLevel;
   } else {
     // We are done with the effect chain at this point so let's clear the
     // nextEffect pointers to assist with GC. If we have passive effects, we'll
@@ -1937,9 +1943,19 @@ export function flushPassiveEffects() {
   }
   const root = rootWithPendingPassiveEffects;
   const expirationTime = pendingPassiveEffectsExpirationTime;
+  const renderPriorityLevel = pendingPassiveEffectsRenderPriority;
   rootWithPendingPassiveEffects = null;
   pendingPassiveEffectsExpirationTime = NoWork;
+  pendingPassiveEffectsRenderPriority = NoPriority;
+  const priorityLevel =
+    renderPriorityLevel > NormalPriority ? NormalPriority : renderPriorityLevel;
+  return runWithPriority(
+    priorityLevel,
+    flushPassiveEffectsImpl.bind(null, root, expirationTime),
+  );
+}
 
+function flushPassiveEffectsImpl(root, expirationTime) {
   let prevInteractions: Set<Interaction> | null = null;
   if (enableSchedulerTracing) {
     prevInteractions = __interactionsRef.current;

packages/react-reconciler/src/SchedulerWithReactIntegration.js

@@ -43,7 +43,7 @@ if (enableSchedulerTracing) {
   );
 }
 
-export opaque type ReactPriorityLevel = 99 | 98 | 97 | 96 | 95 | 90;
+export type ReactPriorityLevel = 99 | 98 | 97 | 96 | 95 | 90;
 export type SchedulerCallback = (isSync: boolean) => SchedulerCallback | null;
 
 type SchedulerCallbackOptions = {

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

@@ -139,35 +139,78 @@ describe('ReactSchedulerIntegration', () => {
     ]);
   });
 
-  it('passive effects have the same priority as render', () => {
+  it('passive effects never have higher than normal priority', async () => {
     const {useEffect} = React;
-    function ReadPriority() {
+    function ReadPriority({step}) {
       Scheduler.unstable_yieldValue(
-        'Render priority: ' + getCurrentPriorityAsString(),
+        `Render priority: ${getCurrentPriorityAsString()}`,
       );
       useEffect(() => {
         Scheduler.unstable_yieldValue(
-          'Passive priority: ' + getCurrentPriorityAsString(),
+          `Effect priority: ${getCurrentPriorityAsString()}`,
         );
       });
       return null;
     }
-    ReactNoop.act(() => {
-      ReactNoop.render(<ReadPriority />);
-      expect(Scheduler).toFlushAndYield([
-        'Render priority: Normal',
-        'Passive priority: Normal',
-      ]);
 
-      runWithPriority(UserBlockingPriority, () => {
+    // High priority renders spawn effects at normal priority
+    await ReactNoop.act(async () => {
+      Scheduler.unstable_runWithPriority(ImmediatePriority, () => {
+        ReactNoop.render(<ReadPriority />);
+      });
+    });
+    expect(Scheduler).toHaveYielded([
+      'Render priority: Immediate',
+      'Effect priority: Normal',
+    ]);
+    await ReactNoop.act(async () => {
+      Scheduler.unstable_runWithPriority(UserBlockingPriority, () => {
         ReactNoop.render(<ReadPriority />);
       });
+    });
+    expect(Scheduler).toHaveYielded([
+      'Render priority: UserBlocking',
+      'Effect priority: Normal',
+    ]);
 
-      expect(Scheduler).toFlushAndYield([
-        'Render priority: UserBlocking',
-        'Passive priority: UserBlocking',
-      ]);
+    // Renders lower than normal priority spawn effects at the same priority
+    await ReactNoop.act(async () => {
+      Scheduler.unstable_runWithPriority(IdlePriority, () => {
+        ReactNoop.render(<ReadPriority />);
+      });
     });
+    expect(Scheduler).toHaveYielded([
+      'Render priority: Idle',
+      'Effect priority: Idle',
+    ]);
+  });
+
+  it('passive effects have correct priority even if they are flushed early', async () => {
+    const {useEffect} = React;
+    function ReadPriority({step}) {
+      Scheduler.unstable_yieldValue(
+        `Render priority [step ${step}]: ${getCurrentPriorityAsString()}`,
+      );
+      useEffect(() => {
+        Scheduler.unstable_yieldValue(
+          `Effect priority [step ${step}]: ${getCurrentPriorityAsString()}`,
+        );
+      });
+      return null;
+    }
+    await ReactNoop.act(async () => {
+      ReactNoop.render(<ReadPriority step={1} />);
+      Scheduler.unstable_flushUntilNextPaint();
+      expect(Scheduler).toHaveYielded(['Render priority [step 1]: Normal']);
+      Scheduler.unstable_runWithPriority(UserBlockingPriority, () => {
+        ReactNoop.render(<ReadPriority step={2} />);
+      });
+    });
+    expect(Scheduler).toHaveYielded([
+      'Effect priority [step 1]: Normal',
+      'Render priority [step 2]: UserBlocking',
+      'Effect priority [step 2]: Normal',
+    ]);
   });
 
   it('after completing a level of work, infers priority of the next batch based on its expiration time', () => {
@@ -213,7 +256,4 @@ describe('ReactSchedulerIntegration', () => {
     Scheduler.unstable_flushUntilNextPaint();
     expect(Scheduler).toHaveYielded(['A', 'B', 'C']);
   });
-
-  // TODO
-  it.skip('passive effects have render priority even if they are flushed early', () => {});
 });