Bugfix: Effects should never have higher than normal priority

packages/react-reconciler/src/ReactFiberWorkLoop.js

@@ -38,6 +38,7 @@ import {
   shouldYield,
   requestPaint,
   now,
+  NoPriority,
   ImmediatePriority,
   UserBlockingPriority,
   NormalPriority,
@@ -232,6 +233,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<
@@ -1478,20 +1480,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();
 
@@ -1714,6 +1719,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
@@ -1921,9 +1927,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', () => {});
 });