From 2cf6ae01cf49e6f849c1f9dbea360d332dbc5bf0 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Mon, 22 Mar 2021 11:05:43 -0500 Subject: [PATCH] Use SyncLane for discrete event hydration (#21038) Discrete event hydration doesn't need to be interruptible, since there's nothing higher priority than discrete events. So we can use SyncLane instead of a special hydration lane. --- ...DOMServerPartialHydration-test.internal.js | 119 +++++++++--------- ...MServerSelectiveHydration-test.internal.js | 81 ++++++------ .../DOMPluginEventSystem-test.internal.js | 16 +-- .../src/ReactFiberReconciler.new.js | 5 +- .../src/ReactFiberReconciler.old.js | 5 +- 5 files changed, 121 insertions(+), 105 deletions(-) diff --git a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js index 00ed4050d6a99..5f127d37fcddf 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerPartialHydration-test.internal.js @@ -2187,17 +2187,17 @@ describe('ReactDOMServerPartialHydration', () => { expect(container.textContent).toBe('Click meHello'); // We're now partially hydrated. - a.click(); + await act(async () => { + a.click(); + }); expect(clicks).toBe(0); // Resolving the promise so that rendering can complete. - suspend = false; - resolve(); - await promise; - - Scheduler.unstable_flushAll(); - jest.runAllTimers(); - + await act(async () => { + suspend = false; + resolve(); + await promise; + }); expect(clicks).toBe(1); expect(container.textContent).toBe('Hello'); @@ -2270,18 +2270,19 @@ describe('ReactDOMServerPartialHydration', () => { jest.runAllTimers(); // We're now partially hydrated. - a.click(); + await act(async () => { + a.click(); + }); // We should not have invoked the event yet because we're not // yet hydrated. expect(onEvent).toHaveBeenCalledTimes(0); // Resolving the promise so that rendering can complete. - suspend = false; - resolve(); - await promise; - - Scheduler.unstable_flushAll(); - jest.runAllTimers(); + await act(async () => { + suspend = false; + resolve(); + await promise; + }); expect(onEvent).toHaveBeenCalledTimes(2); @@ -2344,7 +2345,9 @@ describe('ReactDOMServerPartialHydration', () => { root.render(); // We'll do one click before hydrating. - a.click(); + await act(async () => { + a.click(); + }); // This should be delayed. expect(clicks).toBe(0); @@ -2352,17 +2355,17 @@ describe('ReactDOMServerPartialHydration', () => { jest.runAllTimers(); // We're now partially hydrated. - a.click(); + await act(async () => { + a.click(); + }); expect(clicks).toBe(0); // Resolving the promise so that rendering can complete. - suspend = false; - resolve(); - await promise; - - Scheduler.unstable_flushAll(); - jest.runAllTimers(); - + await act(async () => { + suspend = false; + resolve(); + await promise; + }); expect(clicks).toBe(2); document.body.removeChild(container); @@ -2436,19 +2439,19 @@ describe('ReactDOMServerPartialHydration', () => { jest.runAllTimers(); // We're now partially hydrated. - a.click(); + await act(async () => { + a.click(); + }); // We should not have invoked the event yet because we're not // yet hydrated. expect(onEvent).toHaveBeenCalledTimes(0); // Resolving the promise so that rendering can complete. - suspend = false; - resolve(); - await promise; - - Scheduler.unstable_flushAll(); - jest.runAllTimers(); - + await act(async () => { + suspend = false; + resolve(); + await promise; + }); expect(onEvent).toHaveBeenCalledTimes(2); document.body.removeChild(container); @@ -2510,17 +2513,18 @@ describe('ReactDOMServerPartialHydration', () => { jest.runAllTimers(); // We're now partially hydrated. - span.click(); + await act(async () => { + span.click(); + }); expect(clicksOnChild).toBe(0); expect(clicksOnParent).toBe(0); // Resolving the promise so that rendering can complete. - suspend = false; - resolve(); - await promise; - - Scheduler.unstable_flushAll(); - jest.runAllTimers(); + await act(async () => { + suspend = false; + resolve(); + await promise; + }); expect(clicksOnChild).toBe(1); // This will be zero due to the stopPropagation. @@ -2589,16 +2593,17 @@ describe('ReactDOMServerPartialHydration', () => { Scheduler.unstable_flushAll(); // The Suspense boundary is not yet hydrated. - a.click(); + await act(async () => { + a.click(); + }); expect(clicks).toBe(0); // Resolving the promise so that rendering can complete. - suspend = false; - resolve(); - await promise; - - Scheduler.unstable_flushAll(); - jest.runAllTimers(); + await act(async () => { + suspend = false; + resolve(); + await promise; + }); // We're now full hydrated. @@ -2858,20 +2863,22 @@ describe('ReactDOMServerPartialHydration', () => { expect(container.textContent).toBe('Click meHello'); // We're now partially hydrated. - form.dispatchEvent( - new Event('submit', { - bubbles: true, - }), - ); + await act(async () => { + form.dispatchEvent( + new Event('submit', { + bubbles: true, + }), + ); + }); expect(submits).toBe(0); // Resolving the promise so that rendering can complete. - suspend = false; - resolve(); - await promise; + await act(async () => { + suspend = false; + resolve(); + await promise; + }); - Scheduler.unstable_flushAll(); - jest.runAllTimers(); expect(submits).toBe(1); expect(container.textContent).toBe('Hello'); document.body.removeChild(container); diff --git a/packages/react-dom/src/__tests__/ReactDOMServerSelectiveHydration-test.internal.js b/packages/react-dom/src/__tests__/ReactDOMServerSelectiveHydration-test.internal.js index 96adf85ba35a8..cadc00e886fa8 100644 --- a/packages/react-dom/src/__tests__/ReactDOMServerSelectiveHydration-test.internal.js +++ b/packages/react-dom/src/__tests__/ReactDOMServerSelectiveHydration-test.internal.js @@ -255,22 +255,25 @@ describe('ReactDOMServerSelectiveHydration', () => { expect(Scheduler).toHaveYielded([]); // This click target cannot be hydrated yet because it's suspended. - const result = dispatchClickEvent(spanD); - - expect(Scheduler).toHaveYielded(['App']); - - expect(result).toBe(true); - - // Continuing rendering will render B next. - expect(Scheduler).toFlushAndYield(['B', 'C']); - - suspend = false; - resolve(); - await promise; + await act(async () => { + const result = dispatchClickEvent(spanD); + expect(result).toBe(true); + }); + expect(Scheduler).toHaveYielded([ + 'App', + // Continuing rendering will render B next. + 'B', + 'C', + ]); + await act(async () => { + suspend = false; + resolve(); + await promise; + }); // After the click, we should prioritize D and the Click first, // and only after that render A and C. - expect(Scheduler).toFlushAndYield(['D', 'Clicked D', 'A']); + expect(Scheduler).toHaveYielded(['D', 'Clicked D', 'A']); document.body.removeChild(container); }); @@ -348,13 +351,15 @@ describe('ReactDOMServerSelectiveHydration', () => { expect(Scheduler).toHaveYielded(['App']); - suspend = false; - resolve(); - await promise; + await act(async () => { + suspend = false; + resolve(); + await promise; + }); // We should prioritize hydrating A, C and D first since we clicked in // them. Only after they're done will we hydrate B. - expect(Scheduler).toFlushAndYield([ + expect(Scheduler).toHaveYielded([ 'A', 'Clicked A', 'C', @@ -506,21 +511,21 @@ describe('ReactDOMServerSelectiveHydration', () => { // Nothing has been hydrated so far. expect(Scheduler).toHaveYielded([]); - const target = createEventTarget(spanD); - target.virtualclick(); - - expect(Scheduler).toHaveYielded(['App']); - // Continuing rendering will render B next. - expect(Scheduler).toFlushAndYield(['B', 'C']); - - suspend = false; - resolve(); - await promise; + await act(async () => { + const target = createEventTarget(spanD); + target.virtualclick(); + }); + expect(Scheduler).toHaveYielded(['App', 'B', 'C']); // After the click, we should prioritize D and the Click first, // and only after that render A and C. - expect(Scheduler).toFlushAndYield(['D', 'Clicked D', 'A']); + await act(async () => { + suspend = false; + resolve(); + await promise; + }); + expect(Scheduler).toHaveYielded(['D', 'Clicked D', 'A']); document.body.removeChild(container); }); @@ -602,13 +607,15 @@ describe('ReactDOMServerSelectiveHydration', () => { expect(Scheduler).toHaveYielded(['App']); - suspend = false; - resolve(); - await promise; + await act(async () => { + suspend = false; + resolve(); + await promise; + }); // We should prioritize hydrating A, C and D first since we clicked in // them. Only after they're done will we hydrate B. - expect(Scheduler).toFlushAndYield([ + expect(Scheduler).toHaveYielded([ 'A', 'Clicked A', 'C', @@ -701,9 +708,11 @@ describe('ReactDOMServerSelectiveHydration', () => { expect(Scheduler).toHaveYielded(['App']); - suspend = false; - resolve(); - await promise; + await act(async () => { + suspend = false; + resolve(); + await promise; + }); // We should prioritize hydrating D first because we clicked it. // Next we should hydrate C since that's the current hover target. @@ -711,7 +720,7 @@ describe('ReactDOMServerSelectiveHydration', () => { // the same time since B was already scheduled. // This is ok because it will at least not continue for nested // boundary. See the next test below. - expect(Scheduler).toFlushAndYield([ + expect(Scheduler).toHaveYielded([ 'D', 'Clicked D', 'B', // Ideally this should be later. diff --git a/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js b/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js index 004f0bb631f31..3491aa7b94f1d 100644 --- a/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js +++ b/packages/react-dom/src/events/__tests__/DOMPluginEventSystem-test.internal.js @@ -67,6 +67,7 @@ describe('DOMPluginEventSystem', () => { ReactDOM = require('react-dom'); Scheduler = require('scheduler'); ReactDOMServer = require('react-dom/server'); + act = require('react-dom/test-utils').unstable_concurrentAct; container = document.createElement('div'); document.body.appendChild(container); startNativeEventListenerClearDown(); @@ -636,16 +637,17 @@ describe('DOMPluginEventSystem', () => { Scheduler.unstable_flushAll(); // The Suspense boundary is not yet hydrated. - a.click(); + await act(async () => { + a.click(); + }); expect(clicks).toBe(0); // Resolving the promise so that rendering can complete. - suspend = false; - resolve(); - await promise; - - Scheduler.unstable_flushAll(); - jest.runAllTimers(); + await act(async () => { + suspend = false; + resolve(); + await promise; + }); // We're now full hydrated. diff --git a/packages/react-reconciler/src/ReactFiberReconciler.new.js b/packages/react-reconciler/src/ReactFiberReconciler.new.js index 063b70926b196..bd1f901177935 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.new.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.new.js @@ -78,7 +78,6 @@ import { import {StrictLegacyMode} from './ReactTypeOfMode'; import { SyncLane, - InputDiscreteHydrationLane, SelectiveHydrationLane, NoTimestamp, getHighestPriorityPendingLanes, @@ -388,7 +387,7 @@ export function attemptSynchronousHydration(fiber: Fiber): void { // If we're still blocked after this, we need to increase // the priority of any promises resolving within this // boundary so that they next attempt also has higher pri. - const retryLane = InputDiscreteHydrationLane; + const retryLane = SyncLane; markRetryLaneIfNotHydrated(fiber, retryLane); break; } @@ -422,7 +421,7 @@ export function attemptDiscreteHydration(fiber: Fiber): void { return; } const eventTime = requestEventTime(); - const lane = InputDiscreteHydrationLane; + const lane = SyncLane; scheduleUpdateOnFiber(fiber, lane, eventTime); markRetryLaneIfNotHydrated(fiber, lane); } diff --git a/packages/react-reconciler/src/ReactFiberReconciler.old.js b/packages/react-reconciler/src/ReactFiberReconciler.old.js index 8aac687d3e382..0f0520eb71200 100644 --- a/packages/react-reconciler/src/ReactFiberReconciler.old.js +++ b/packages/react-reconciler/src/ReactFiberReconciler.old.js @@ -78,7 +78,6 @@ import { import {StrictLegacyMode} from './ReactTypeOfMode'; import { SyncLane, - InputDiscreteHydrationLane, SelectiveHydrationLane, NoTimestamp, getHighestPriorityPendingLanes, @@ -388,7 +387,7 @@ export function attemptSynchronousHydration(fiber: Fiber): void { // If we're still blocked after this, we need to increase // the priority of any promises resolving within this // boundary so that they next attempt also has higher pri. - const retryLane = InputDiscreteHydrationLane; + const retryLane = SyncLane; markRetryLaneIfNotHydrated(fiber, retryLane); break; } @@ -422,7 +421,7 @@ export function attemptDiscreteHydration(fiber: Fiber): void { return; } const eventTime = requestEventTime(); - const lane = InputDiscreteHydrationLane; + const lane = SyncLane; scheduleUpdateOnFiber(fiber, lane, eventTime); markRetryLaneIfNotHydrated(fiber, lane); }