Synchronously flush the transition lane scheduled in a popstate event #26025

tyao1 · 2023-01-20T03:28:30Z

Summary

Browsers restore state like forms and scroll position right after the popstate event. To make sure the page work as expected on back or forward button, we need to flush transitions scheduled in a popstate synchronously, and only yields if it suspends.
This PR adds a new HostConfig method to check if window.event === 'popstate', and scheduleMicrotask if a transition is scheduled in a PopStateEvent.

How did you test this change?

yarn test

packages/react-reconciler/src/ReactFiberWorkLoop.js

packages/react-dom-bindings/src/client/ReactDOMHostConfig.js

sebmarkbage · 2023-01-24T03:40:18Z

packages/react-reconciler/src/ReactInternalTypes.js

@@ -259,6 +259,8 @@ type BaseFiberRootProperties = {
  pooledCache: Cache | null,
  pooledCacheLanes: Lanes,

+  forceSync: boolean,


It's not really a "forced" sync because it just an best-effort attempt. This could easily be confused with the API called ReactDOM.flushSync(...) which is more like a force.

This would be more like attemptEagerTransition. I don't think you should necessarily need this flag though with the new refactoring because once we schedule an eager microtask all the time, you can just do the work right there if you check for it.

if (shouldAttemptEagerTransition()) performWorkOnRoot()

A lot of things will be a lot simpler with that model.

sebmarkbage · 2023-01-24T03:41:39Z

packages/react-reconciler/src/ReactFiberWorkLoop.js

@@ -1469,7 +1478,7 @@ function performSyncWorkOnRoot(root: FiberRoot) {
  flushPassiveEffects();

  let lanes = getNextLanes(root, NoLanes);
-  if (!includesSyncLane(lanes)) {
+  if (!includesSyncLaneOrForceSync(lanes, root)) {


Instead of using performSyncWorkOnRoot for this, I wonder if you can just use performConcurrentWorkOnRoot without yielding. That way you don't have to mess with the lanes and other special cases for sync.

We already have cases for performing concurrent work without yielding.

react/packages/react-reconciler/src/ReactFiberWorkLoop.js

Lines 1047 to 1052 in a48e54f

// We disable time-slicing in some cases: if the work has been CPU-bound

// for too long ("expired" work, to prevent starvation), or we're in

// sync-updates-by-default mode.

// TODO: We only check `didTimeout` defensively, to account for a Scheduler

// bug we're still investigating. Once the bug in Scheduler is fixed,

// we can remove this, since we track expiration ourselves.

react-sizebot · 2023-04-04T02:24:46Z

Comparing: 72c890e...b340309

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	+0.04%	164.40 kB	164.47 kB	+0.06%	51.65 kB	51.68 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	+0.04%	166.84 kB	166.91 kB	+0.06%	52.30 kB	52.33 kB
facebook-www/ReactDOM-prod.classic.js	+0.03%	565.32 kB	565.49 kB	+0.05%	99.49 kB	99.55 kB
facebook-www/ReactDOM-prod.modern.js	+0.03%	549.11 kB	549.28 kB	+0.06%	96.79 kB	96.85 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-experimental/react-noop-renderer/cjs/react-noop-renderer.production.min.js	+0.32%	15.57 kB	15.62 kB	+0.37%	4.66 kB	4.67 kB
oss-stable-semver/react-noop-renderer/cjs/react-noop-renderer.production.min.js	+0.32%	15.57 kB	15.62 kB	+0.37%	4.66 kB	4.67 kB
oss-stable/react-noop-renderer/cjs/react-noop-renderer.production.min.js	+0.32%	15.57 kB	15.62 kB	+0.37%	4.66 kB	4.67 kB
oss-experimental/react-noop-renderer/cjs/react-noop-renderer-persistent.production.min.js	+0.32%	15.64 kB	15.69 kB	+0.34%	4.67 kB	4.69 kB
oss-stable-semver/react-noop-renderer/cjs/react-noop-renderer-persistent.production.min.js	+0.32%	15.64 kB	15.69 kB	+0.34%	4.67 kB	4.69 kB
oss-stable/react-noop-renderer/cjs/react-noop-renderer-persistent.production.min.js	+0.32%	15.64 kB	15.69 kB	+0.34%	4.67 kB	4.69 kB

Generated by 🚫 dangerJS against b340309

packages/react-reconciler/src/ReactFiberRootScheduler.js

packages/react-reconciler/src/ReactFiberLane.js

sophiebits

Worth adding a test for the behavior where the transition can’t finish synchronously without triggering a fallback?

packages/react-art/src/ReactARTHostConfig.js

tyao1 · 2023-04-12T19:39:04Z

Worth adding a test for the behavior where the transition can’t finish synchronously without triggering a fallback?

I'm not sure what would the expected behavior be when that happens. Maybe we can address it in a follow up PR.

sophiebits · 2023-04-12T20:03:47Z

My understanding of the original goal was that if it can't finish the transition synchronously then the transition should suspend and not trigger any fallbacks. So it would fail to finish synchronously but the fallback behavior would be the same as if it didn't try for the eager transition. That's why it is called "should attempt eager transition" not "should force eager transition" or something. It's a nice to have for the common case so that browser scroll restoration, etc. works correctly but if for some reason the data isn't available then it would do the async transition. However my knowledge may be out of date so definitely defer to others. Also I don't understand how that intersects with async components since it seems like those could never finish synchronously? (I guess someone could cache their previous output somehow?)

Either way, it seems like an important aspect of the behavior here so I think it would be good to encode it in a test.

packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js

acdlite · 2023-04-13T19:10:18Z

My understanding of the original goal was that if it can't finish the transition synchronously then the transition should suspend and not trigger any fallbacks.

The thinking is that it's more important for it to be synchronous than to avoid a fallback in this case because 1) it needs to be sync in order for the browser's scroll restoration mechanism to work 2) during a back navigation it's likely the data is cached so it probably won't suspend the important stuff anyway.

However, if something suspends and there's no Suspense boundary above that can handle it, this trick isn't going to work regardless, so the thinking was we would fall back to the regular transition behavior (give up and change it back to async). This PR doesn't implement that but it's not as urgent as the main fix and we can add that later.

acdlite · 2023-04-13T19:16:48Z

I need this for another feature I'm working on so I'm going to go ahead and merge it, if that's alright

…#26025)  ## Summary Browsers restore state like forms and scroll position right after the popstate event. To make sure the page work as expected on back or forward button, we need to flush transitions scheduled in a popstate synchronously, and only yields if it suspends. This PR adds a new HostConfig method to check if `window.event === 'popstate'`, and `scheduleMicrotask` if a transition is scheduled in a `PopStateEvent`. ## How did you test this change? yarn test DiffTrain build for [d121c67](d121c67)

…te event (facebook#26025)" This reverts commit d121c67.

…#26025)  ## Summary Browsers restore state like forms and scroll position right after the popstate event. To make sure the page work as expected on back or forward button, we need to flush transitions scheduled in a popstate synchronously, and only yields if it suspends. This PR adds a new HostConfig method to check if `window.event === 'popstate'`, and `scheduleMicrotask` if a transition is scheduled in a `PopStateEvent`. ## How did you test this change? yarn test

…facebook#26025)  ## Summary Browsers restore state like forms and scroll position right after the popstate event. To make sure the page work as expected on back or forward button, we need to flush transitions scheduled in a popstate synchronously, and only yields if it suspends. This PR adds a new HostConfig method to check if `window.event === 'popstate'`, and `scheduleMicrotask` if a transition is scheduled in a `PopStateEvent`. ## How did you test this change? yarn test

…#26025)  ## Summary Browsers restore state like forms and scroll position right after the popstate event. To make sure the page work as expected on back or forward button, we need to flush transitions scheduled in a popstate synchronously, and only yields if it suspends. This PR adds a new HostConfig method to check if `window.event === 'popstate'`, and `scheduleMicrotask` if a transition is scheduled in a `PopStateEvent`. ## How did you test this change? yarn test DiffTrain build for commit d121c67.

tyao1 requested review from sebmarkbage and acdlite January 20, 2023 03:28

facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jan 20, 2023

sebmarkbage reviewed Jan 24, 2023

View reviewed changes

packages/react-reconciler/src/ReactFiberWorkLoop.js Outdated Show resolved Hide resolved

sebmarkbage reviewed Jan 24, 2023

View reviewed changes

packages/react-dom-bindings/src/client/ReactDOMHostConfig.js Outdated Show resolved Hide resolved

sebmarkbage reviewed Jan 24, 2023

View reviewed changes

tyao1 force-pushed the ty-popstate-transition branch from ffb7566 to 1bceba2 Compare April 4, 2023 02:20

tyao1 force-pushed the ty-popstate-transition branch from 1bceba2 to 638cd73 Compare April 4, 2023 02:47

acdlite reviewed Apr 4, 2023

View reviewed changes

packages/react-reconciler/src/ReactFiberRootScheduler.js Outdated Show resolved Hide resolved

tyao1 force-pushed the ty-popstate-transition branch from cd4e4cc to f5622fa Compare April 5, 2023 23:10

tyao1 requested a review from acdlite April 10, 2023 16:41

acdlite reviewed Apr 10, 2023

View reviewed changes

packages/react-reconciler/src/ReactFiberLane.js Outdated Show resolved Hide resolved

acdlite approved these changes Apr 10, 2023

View reviewed changes

sophiebits reviewed Apr 10, 2023

View reviewed changes

packages/react-art/src/ReactARTHostConfig.js Outdated Show resolved Hide resolved

tyao1 added 4 commits April 12, 2023 12:00

Synchronously render the transition lane on popstate

79294e9

Use currentEventTransitionLane

6aaa397

Fix the test

bfaa019

feedback

f642605

tyao1 force-pushed the ty-popstate-transition branch from f5622fa to f642605 Compare April 12, 2023 19:38

tyao1 added 2 commits April 12, 2023 15:02

flow

7fb6163

add test

7a34b06

gaearon reviewed Apr 12, 2023

View reviewed changes

packages/react-dom/src/__tests__/ReactDOMFiberAsync-test.js Show resolved Hide resolved

Cleanup window.event

b340309

acdlite merged commit d121c67 into main Apr 13, 2023

kassens added a commit to kassens/react that referenced this pull request Apr 17, 2023

Revert "Synchronously flush the transition lane scheduled in a popsta…

f567431

…te event (facebook#26025)" This reverts commit d121c67.

tyao1 deleted the ty-popstate-transition branch October 12, 2023 17:39

CodyJasonBennett mentioned this pull request Apr 30, 2024

[React 19] react-reconciler README is missing scheduler and form hooks #28956

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Synchronously flush the transition lane scheduled in a popstate event #26025

Synchronously flush the transition lane scheduled in a popstate event #26025

tyao1 commented Jan 20, 2023

sebmarkbage Jan 24, 2023

sebmarkbage Jan 24, 2023

react-sizebot commented Apr 4, 2023 •

edited

Loading

sophiebits left a comment

tyao1 commented Apr 12, 2023

sophiebits commented Apr 12, 2023

acdlite commented Apr 13, 2023 •

edited

Loading

acdlite commented Apr 13, 2023

	// We disable time-slicing in some cases: if the work has been CPU-bound
	// for too long ("expired" work, to prevent starvation), or we're in
	// sync-updates-by-default mode.
	// TODO: We only check `didTimeout` defensively, to account for a Scheduler
	// bug we're still investigating. Once the bug in Scheduler is fixed,
	// we can remove this, since we track expiration ourselves.

Synchronously flush the transition lane scheduled in a popstate event #26025

Synchronously flush the transition lane scheduled in a popstate event #26025

Conversation

tyao1 commented Jan 20, 2023

Summary

How did you test this change?

sebmarkbage Jan 24, 2023

Choose a reason for hiding this comment

sebmarkbage Jan 24, 2023

Choose a reason for hiding this comment

react-sizebot commented Apr 4, 2023 • edited Loading

Critical size changes

Significant size changes

sophiebits left a comment

Choose a reason for hiding this comment

tyao1 commented Apr 12, 2023

sophiebits commented Apr 12, 2023

acdlite commented Apr 13, 2023 • edited Loading

acdlite commented Apr 13, 2023

react-sizebot commented Apr 4, 2023 •

edited

Loading

acdlite commented Apr 13, 2023 •

edited

Loading