Fix suspense replaying forward refs #26535

hansottowirtz · 2023-04-02T10:50:58Z

Continuation of #26420

eps1lon · 2023-04-02T12:01:40Z

packages/react-reconciler/src/ReactFiberWorkLoop.js

@@ -2386,7 +2386,8 @@ function replaySuspendedUnitOfWork(unitOfWork: Fiber): void {
      // TODO: Consider moving this switch statement into that module. Also,
      // could maybe use this as an opportunity to say `use` doesn't work with
      // `defaultProps` :)
-      const Component = unitOfWork.type;
+      const Component = unitOfWork.tag === FunctionComponent ? unitOfWork.type : unitOfWork.type.render;
+      const secondArg = unitOfWork.tag === FunctionComponent ? undefined : unitOfWork.ref;


It's easier to read and refactor if we split these into separate cases. Makes it easier to remove forwardRef cases in the future. We already have updateForwardRef so it makes sense to also fork replayFunctionComponent into replayForwardRef

Thanks, that's done now, replayForwardRef and replayFunctionComponent are very similar though, is that a problem?

Yeah that's fine. The update and mount function are also very similar.

Just pass the type as an argument to replayFunctionComponent. I wouldn't use the similarity of updateForwardRef and updateFunctionComponent as an argument because those should probably be unified, too. It's less about code size (Closure will likely inline them anyway) and more to avoid an accidental divergence in the implementations; they're identical except for how type is accessed so it makes sense to me for that part to be shared.

react-sizebot · 2023-04-02T16:19:18Z

Comparing: 0ae3480...7875d3d

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.03%	162.89 kB	162.95 kB	+0.07%	51.51 kB	51.54 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	+0.03%	164.43 kB	164.48 kB	+0.09%	51.99 kB	52.03 kB
facebook-www/ReactDOM-prod.classic.js	+0.05%	552.75 kB	553.05 kB	+0.07%	98.17 kB	98.24 kB
facebook-www/ReactDOM-prod.modern.js	+0.05%	536.60 kB	536.90 kB	+0.02%	95.54 kB	95.56 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 7875d3d

The only difference between a component wrapped in forwardRef and one that's not is that the former is passed a ref prop. To make it less likely that the behavior diverges, I unified replayFunctionComponent and replayForwardRefComponent into a single function again. We should do this for the regular begin phase functions, too: instead of completely forked path for regular functions and forwardRef, they should both converge into the same path. We already do this for SimpleMemoComponent. I also added more test coverage for the different component types because I noticed we were missing some. Also noticed that legacy context wasn't being handled during the replay of a function component wrapped with `memo` and added a test for that.

These are the same thing.

Oops

Continuation of #26420 Fixes #26385 and #26419 --------- Co-authored-by: eps1lon <silbermann.sebastian@gmail.com> Co-authored-by: Andrew Clark <git@andrewclark.io> DiffTrain build for [7329ea8](7329ea8)

Summary: This sync includes the following changes: - **[58742c21b](facebook/react@58742c21b )**: Delete unused `eventTimes` Fiber field ([#26599](facebook/react#26599)) //<Andrew Clark>// - **[0b931f90e](facebook/react@0b931f90e )**: Remove JND delay for non-transition updates ([#26597](facebook/react#26597)) //<Andrew Clark>// - **[ac43bf687](facebook/react@ac43bf687 )**: Move validation of text nesting into ReactDOMComponent ([#26594](facebook/react#26594)) //<Sebastian Markbåge>// - **[ca41adb8c](facebook/react@ca41adb8c )**: Diff properties in the commit phase instead of generating an update payload ([#26583](facebook/react#26583)) //<Sebastian Markbåge>// - **[dd0619b2e](facebook/react@dd0619b2e )**: rename $$$hostConfig to $$$config ([#26593](facebook/react#26593)) //<Josh Story>// - **[b55d31955](facebook/react@b55d31955 )**: Rename HostConfig files to FiberConfig to clarify they are configs fo… ([#26592](facebook/react#26592)) //<Josh Story>// - **[ffb8eaca5](facebook/react@ffb8eaca5 )**: Rename ReactServerFormatConfig to ReactFizzConfig ([#26591](facebook/react#26591)) //<Josh Story>// - **[f4f873f62](facebook/react@f4f873f62 )**: Implements wiring for Flight to have it's own "HostConfig" ([#26590](facebook/react#26590)) //<Josh Story>// - **[44db16afc](facebook/react@44db16afc )**: Normalize ReactFlightServerConfig and related files ([#26589](facebook/react#26589)) //<Josh Story>// - **[fec97ecbc](facebook/react@fec97ecbc )**: act: Move didScheduleLegacyUpdate to ensureRootIsScheduled ([#26552](facebook/react#26552)) //<Andrew Clark>// - **[9a9da7721](facebook/react@9a9da7721 )**: Don't update textarea defaultValue and input checked unnecessarily ([#26580](facebook/react#26580)) //<Sebastian Markbåge>// - **[e5146cb52](facebook/react@e5146cb52 )**: Refactor some controlled component stuff ([#26573](facebook/react#26573)) //<Sebastian Markbåge>// - **[657698e48](facebook/react@657698e48 )**: [Tests] `waitForThrow` should diff strings ([#26568](facebook/react#26568)) //<Josh Story>// - **[85bb7b685](facebook/react@85bb7b685 )**: Fix: Move `destroy` field to shared instance object ([#26561](facebook/react#26561)) //<Andrew Clark>// - **[9cfba0f6e](facebook/react@9cfba0f6e )**: Clean up discrete event replaying ([#26558](facebook/react#26558)) //<Sebastian Markbåge>// - **[790ebc962](facebook/react@790ebc962 )**: Remove no-fallthrough lint suppressions ([#26553](facebook/react#26553)) //<Sophie Alpert>// - **[c15579631](facebook/react@c15579631 )**: Put common aliases in Map/Set instead of switch over strings ([#26551](facebook/react#26551)) //<Sebastian Markbåge>// - **[d5fd60f7e](facebook/react@d5fd60f7e )**: Remove findInstanceBlockingEvent unused parameters ([#26534](facebook/react#26534)) //<Mohammad Ghorbani>// - **[eeabb7312](facebook/react@eeabb7312 )**: Refactor DOM Bindings Completely Off of DOMProperty Meta Programming ([#26546](facebook/react#26546)) //<Sebastian Markbåge>// - **[da94e8b24](facebook/react@da94e8b24 )**: Revert "Cleanup enableSyncDefaultUpdate flag ([#26236](facebook/react#26236))" ([#26528](facebook/react#26528)) //<Jan Kassens>// - **[0700dd50b](facebook/react@0700dd50b )**: Implement public instances for text nodes in Fabric ([#26516](facebook/react#26516)) //<Rubén Norte>// - **[4a1cc2ddd](facebook/react@4a1cc2ddd )**: Fix logic around attribute seralization ([#26526](facebook/react#26526)) //<Josh Story>// - **[7329ea81c](facebook/react@7329ea81c )**: Fix suspense replaying forward refs ([#26535](facebook/react#26535)) //<Hans Otto Wirtz>// - **[0ae348018](facebook/react@0ae348018 )**: [Float] Suspend unstyled content for up to 1 minute ([#26532](facebook/react#26532)) //<Andrew Clark>// - **[888874673](facebook/react@888874673 )**: Allow transitions to interrupt Suspensey commits ([#26531](facebook/react#26531)) //<Andrew Clark>// - **[09c8d2563](facebook/react@09c8d2563 )**: Move update scheduling to microtask ([#26512](facebook/react#26512)) //<Andrew Clark>// - **[8310854ce](facebook/react@8310854ce )**: Clean up enableCapturePhaseSelectiveHydrationWithoutDiscreteEventReplay ([#26521](facebook/react#26521)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions ca01f35...58742c2 jest_e2e[run_all_tests] bypass-github-export-checks Reviewed By: sammy-SC Differential Revision: D44872333 fbshipit-source-id: 0695e86645955aac7a20afdaf3ed02ad33592f5c

This reverts commit 7329ea8.

This reverts commit adac7b1.

This reverts commit adac7b1. DiffTrain build for [2f18eb0](2f18eb0)

Summary: This sync includes the following changes: - **[58742c21b](facebook/react@58742c21b )**: Delete unused `eventTimes` Fiber field ([facebook#26599](facebook/react#26599)) //<Andrew Clark>// - **[0b931f90e](facebook/react@0b931f90e )**: Remove JND delay for non-transition updates ([facebook#26597](facebook/react#26597)) //<Andrew Clark>// - **[ac43bf687](facebook/react@ac43bf687 )**: Move validation of text nesting into ReactDOMComponent ([facebook#26594](facebook/react#26594)) //<Sebastian Markbåge>// - **[ca41adb8c](facebook/react@ca41adb8c )**: Diff properties in the commit phase instead of generating an update payload ([facebook#26583](facebook/react#26583)) //<Sebastian Markbåge>// - **[dd0619b2e](facebook/react@dd0619b2e )**: rename $$$hostConfig to $$$config ([facebook#26593](facebook/react#26593)) //<Josh Story>// - **[b55d31955](facebook/react@b55d31955 )**: Rename HostConfig files to FiberConfig to clarify they are configs fo… ([facebook#26592](facebook/react#26592)) //<Josh Story>// - **[ffb8eaca5](facebook/react@ffb8eaca5 )**: Rename ReactServerFormatConfig to ReactFizzConfig ([facebook#26591](facebook/react#26591)) //<Josh Story>// - **[f4f873f62](facebook/react@f4f873f62 )**: Implements wiring for Flight to have it's own "HostConfig" ([facebook#26590](facebook/react#26590)) //<Josh Story>// - **[44db16afc](facebook/react@44db16afc )**: Normalize ReactFlightServerConfig and related files ([facebook#26589](facebook/react#26589)) //<Josh Story>// - **[fec97ecbc](facebook/react@fec97ecbc )**: act: Move didScheduleLegacyUpdate to ensureRootIsScheduled ([facebook#26552](facebook/react#26552)) //<Andrew Clark>// - **[9a9da7721](facebook/react@9a9da7721 )**: Don't update textarea defaultValue and input checked unnecessarily ([facebook#26580](facebook/react#26580)) //<Sebastian Markbåge>// - **[e5146cb52](facebook/react@e5146cb52 )**: Refactor some controlled component stuff ([facebook#26573](facebook/react#26573)) //<Sebastian Markbåge>// - **[657698e48](facebook/react@657698e48 )**: [Tests] `waitForThrow` should diff strings ([facebook#26568](facebook/react#26568)) //<Josh Story>// - **[85bb7b685](facebook/react@85bb7b685 )**: Fix: Move `destroy` field to shared instance object ([facebook#26561](facebook/react#26561)) //<Andrew Clark>// - **[9cfba0f6e](facebook/react@9cfba0f6e )**: Clean up discrete event replaying ([facebook#26558](facebook/react#26558)) //<Sebastian Markbåge>// - **[790ebc962](facebook/react@790ebc962 )**: Remove no-fallthrough lint suppressions ([facebook#26553](facebook/react#26553)) //<Sophie Alpert>// - **[c15579631](facebook/react@c15579631 )**: Put common aliases in Map/Set instead of switch over strings ([facebook#26551](facebook/react#26551)) //<Sebastian Markbåge>// - **[d5fd60f7e](facebook/react@d5fd60f7e )**: Remove findInstanceBlockingEvent unused parameters ([facebook#26534](facebook/react#26534)) //<Mohammad Ghorbani>// - **[eeabb7312](facebook/react@eeabb7312 )**: Refactor DOM Bindings Completely Off of DOMProperty Meta Programming ([facebook#26546](facebook/react#26546)) //<Sebastian Markbåge>// - **[da94e8b24](facebook/react@da94e8b24 )**: Revert "Cleanup enableSyncDefaultUpdate flag ([facebook#26236](facebook/react#26236))" ([facebook#26528](facebook/react#26528)) //<Jan Kassens>// - **[0700dd50b](facebook/react@0700dd50b )**: Implement public instances for text nodes in Fabric ([facebook#26516](facebook/react#26516)) //<Rubén Norte>// - **[4a1cc2ddd](facebook/react@4a1cc2ddd )**: Fix logic around attribute seralization ([facebook#26526](facebook/react#26526)) //<Josh Story>// - **[7329ea81c](facebook/react@7329ea81c )**: Fix suspense replaying forward refs ([facebook#26535](facebook/react#26535)) //<Hans Otto Wirtz>// - **[0ae348018](facebook/react@0ae348018 )**: [Float] Suspend unstyled content for up to 1 minute ([facebook#26532](facebook/react#26532)) //<Andrew Clark>// - **[888874673](facebook/react@888874673 )**: Allow transitions to interrupt Suspensey commits ([facebook#26531](facebook/react#26531)) //<Andrew Clark>// - **[09c8d2563](facebook/react@09c8d2563 )**: Move update scheduling to microtask ([facebook#26512](facebook/react#26512)) //<Andrew Clark>// - **[8310854ce](facebook/react@8310854ce )**: Clean up enableCapturePhaseSelectiveHydrationWithoutDiscreteEventReplay ([facebook#26521](facebook/react#26521)) //<Andrew Clark>// Changelog: [General][Changed] - React Native sync for revisions ca01f35...58742c2 jest_e2e[run_all_tests] bypass-github-export-checks Reviewed By: sammy-SC Differential Revision: D44872333 fbshipit-source-id: 0695e86645955aac7a20afdaf3ed02ad33592f5c

Continuation of facebook#26420 Fixes facebook#26385 and facebook#26419 --------- Co-authored-by: eps1lon <silbermann.sebastian@gmail.com> Co-authored-by: Andrew Clark <git@andrewclark.io>

Continuation of #26420 Fixes #26385 and #26419 --------- Co-authored-by: eps1lon <silbermann.sebastian@gmail.com> Co-authored-by: Andrew Clark <git@andrewclark.io> DiffTrain build for commit 7329ea8.

facebook-github-bot added the CLA Signed label Apr 2, 2023

Fix suspense replaying forwardRefs (facebook#26385)

9829d02

eps1lon reviewed Apr 2, 2023

View reviewed changes

hansottowirtz force-pushed the fix-suspense-replaying-forward-refs branch 2 times, most recently from 56b9a11 to 420b31c Compare April 2, 2023 13:06

hansottowirtz mentioned this pull request Apr 2, 2023

Fix suspense replaying forwardRefs #26420

Closed

hansottowirtz added 5 commits April 2, 2023 18:03

Add test for use hook with forwardRef

d561e02

Add test for forwardRefs second arg with use

ede13bc

Fix suspense replaying forwardRefs with correct secondArg

5f0fbbf

Improve suspense replaying forwardRef test

58dd0f4

Add replayForwardRef

4aad797

hansottowirtz force-pushed the fix-suspense-replaying-forward-refs branch from 420b31c to 4aad797 Compare April 2, 2023 16:03

Poke CircleCI

82ab9e0

acdlite added 3 commits April 2, 2023 18:35

Unify FunctionComponent and SimpleMemoComponent cases

f32ae1d

These are the same thing.

Add back accidentally removed comment

7875d3d

Oops

acdlite merged commit 7329ea8 into facebook:main Apr 2, 2023

hansottowirtz deleted the fix-suspense-replaying-forward-refs branch April 3, 2023 07:11

hansottowirtz mentioned this pull request Apr 3, 2023

Bug: forwardRef provides an empty object as ref, causing can't define property "current" error. #26419

Closed

sammy-SC mentioned this pull request Apr 5, 2023

Use native scheduler if defined sammy-SC/react#1

Closed

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

Revert "Fix suspense replaying forward refs (facebook#26535)"

adac7b1

This reverts commit 7329ea8.

kassens added a commit that referenced this pull request Apr 20, 2023

Fix suspense replaying forward refs (#26535)

2f18eb0

This reverts commit adac7b1.

github-actions bot pushed a commit that referenced this pull request Apr 20, 2023

Fix suspense replaying forward refs (#26535)

38b20a3

This reverts commit adac7b1. DiffTrain build for [2f18eb0](2f18eb0)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix suspense replaying forward refs #26535

Fix suspense replaying forward refs #26535

hansottowirtz commented Apr 2, 2023

eps1lon Apr 2, 2023

hansottowirtz Apr 2, 2023

eps1lon Apr 2, 2023

acdlite Apr 2, 2023

react-sizebot commented Apr 2, 2023 •

edited

Loading

Fix suspense replaying forward refs #26535

Fix suspense replaying forward refs #26535

Conversation

hansottowirtz commented Apr 2, 2023

eps1lon Apr 2, 2023

Choose a reason for hiding this comment

hansottowirtz Apr 2, 2023

Choose a reason for hiding this comment

eps1lon Apr 2, 2023

Choose a reason for hiding this comment

acdlite Apr 2, 2023

Choose a reason for hiding this comment

react-sizebot commented Apr 2, 2023 • edited Loading

Critical size changes

Significant size changes

react-sizebot commented Apr 2, 2023 •

edited

Loading