[Fabric] Pass children when cloning #27458

javache · 2023-10-04T16:10:28Z

Summary

Currently when cloning nodes in Fabric, we reset a node's children on each clone, and then repeatedly call appendChild to restore the previous list of children (even if it was quasi-identical to before). This causes unnecessary invalidation of the layout state in Fabric's ShadowNode data (which in turn may require additional yoga clones) and extra JSI calls.

This PR adds a feature flag to pass in the children as part of the clone call, so Fabric always has a complete view of the node that's being mutated.

This feature flag requires matching changes in the react-native repo: facebook/react-native#39817

How did you test this change?

Unit test added demonstrates the new behaviour

yarn test -r www-modern ReactFabric-test
yarn test ReactFabric-test.internal

Tested a manual sync into React Native and verified core surfaces render correctly.

react-sizebot · 2023-10-04T16:14:37Z

Comparing: dddfe68...ad7c13a

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	=	174.46 kB	174.46 kB	=	54.27 kB	54.27 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	176.31 kB	176.31 kB	=	54.88 kB	54.88 kB
facebook-www/ReactDOM-prod.classic.js	=	564.48 kB	564.48 kB	=	99.37 kB	99.37 kB
facebook-www/ReactDOM-prod.modern.js	=	548.21 kB	548.21 kB	=	96.44 kB	96.44 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
react-native/implementations/ReactFabric-prod.fb.js	+0.35%	326.70 kB	327.85 kB	+0.32%	57.96 kB	58.14 kB
react-native/implementations/ReactFabric-profiling.fb.js	+0.31%	354.13 kB	355.23 kB	+0.27%	62.21 kB	62.38 kB
oss-experimental/react-noop-renderer/cjs/react-noop-renderer-persistent.development.js	=	42.28 kB	42.18 kB	=	9.57 kB	9.56 kB
oss-stable-semver/react-noop-renderer/cjs/react-noop-renderer-persistent.development.js	=	42.28 kB	42.18 kB	=	9.57 kB	9.56 kB
oss-stable/react-noop-renderer/cjs/react-noop-renderer-persistent.development.js	=	42.28 kB	42.18 kB	=	9.57 kB	9.56 kB
oss-experimental/react-noop-renderer/cjs/react-noop-renderer.development.js	=	42.15 kB	42.05 kB	=	9.55 kB	9.54 kB
oss-stable-semver/react-noop-renderer/cjs/react-noop-renderer.development.js	=	42.15 kB	42.05 kB	=	9.55 kB	9.54 kB
oss-stable/react-noop-renderer/cjs/react-noop-renderer.development.js	=	42.15 kB	42.05 kB	=	9.55 kB	9.54 kB

Generated by 🚫 dangerJS against ad7c13a

acdlite · 2023-10-04T19:31:52Z

packages/react-native-renderer/src/__tests__/ReactFabric-test.internal.js

@@ -220,6 +220,60 @@ describe('ReactFabric', () => {
    ).toMatchSnapshot();
  });

+  it.each([true, false])(
+    'should not clone nodes without children when updating props (feature flag = %p)',
+    async passChildren => {


Because you set up a the feature flag with __VARIANT__, CI will already run these tests in both versions. (You can do this yourself on the command line by passing --no-variant to yarn test.)

You can then check the value of the flag with if (gate(flags => flags.passChildrenWhenCloningPersistedNodes)) {...}.

We prefer that pattern instead of mutating the ReactFeatureFlags module because it preserves the ability to run tests on the compiled build artifacts.

Hmm, I'm unable to get this to work. It's only set as __VARIANT__ in ReactFeatureFlags.native-fb-dynamic.js and as far as I can trace from logs, I only get the false path.

Running with yarn test -r www-modern ReactFabric-test gets the variant, but the tests fail in other codepaths then (which could also be on me).

Could there be a problem with this test explicitly mocking FeatureFlags itself?

jest.mock('shared/ReactFeatureFlags', () => require('shared/forks/ReactFeatureFlags.native-oss'), );

Test failures were legit :) Bit weird to use www-modern for this, but it does give better coverage across tests.

Oh that’s right I forgot we’ve never set up a test config for the Meta RN builds, like we did for www. I’ll work on adding one of those, not too difficult.

javache · 2023-10-05T08:57:13Z

packages/shared/forks/ReactFeatureFlags.native-oss.js

@@ -79,6 +79,7 @@ export const enableAsyncActions = false;
 export const alwaysThrottleRetries = true;

 export const useMicrotasksForSchedulingInFabric = false;
+export const passChildrenWhenCloningPersistedNodes = __VARIANT__;


Open problem: I need this to get the test to run both variants, but it fails the OSS RN build

/home/circleci/project/build/react-native/implementations/ReactFabric-prod.js 1317:45 error '__VARIANT__' is not defined no-undef

I think you can just set this one to false right? It should just be off for the non-Meta builds.

I can, but then I can't run tests against both variants anymore.

Summary: Update the UIManagerBinding interface to support the upcoming API changes in facebook/react#27458 Changelog: [Internal] Differential Revision: D49912532

Summary: Update the UIManagerBinding interface to support the upcoming API changes in facebook/react#27458 Changelog: [Internal] Reviewed By: rubennorte, sammy-SC Differential Revision: D49912532

Summary: Update the UIManagerBinding interface to support the upcoming API changes in facebook/react#27458 Changelog: [Internal] Pull Request resolved: #39817 Reviewed By: rubennorte, sammy-SC Differential Revision: D49912532 fbshipit-source-id: 436a3488ad4059467070f4ce41ae2b04dda19019

acdlite · 2023-10-09T17:49:13Z

I'm working on setting up proper RN test configs in #27489 but if you don't want to wait for that, what you could also do is set the flag to __EXPERIMENTAL__ in the native-oss feature flags file, rather than __VARIANT__. That will turn it on for the experimental builds in open source but that should be OK.

javache · 2023-10-09T19:44:11Z

I'm working on setting up proper RN test configs in #27489 but if you don't want to wait for that, what you could also do is set the flag to __EXPERIMENTAL__ in the native-oss feature flags file, rather than __VARIANT__. That will turn it on for the experimental builds in open source but that should be OK.

Thanks! I'll just keep this as false for now, and when your PR is ready we should have coverage for both branches.

Any other concerns here? Otherwise, I was going to run through it with @sammy-SC tomorrow and merge it so we can set up testing.

javache · 2023-10-10T13:36:21Z

packages/react-reconciler/src/ReactFiberCompleteWork.js

@@ -328,21 +333,21 @@ function appendAllChildrenToContainer(
    let node = workInProgress.child;
    while (node !== null) {
      // eslint-disable-next-line no-labels


## Summary Currently when cloning nodes in Fabric, we reset a node's children on each clone, and then repeatedly call appendChild to restore the previous list of children (even if it was quasi-identical to before). This causes unnecessary invalidation of the layout state in Fabric's ShadowNode data (which in turn may require additional yoga clones) and extra JSI calls. This PR adds a feature flag to pass in the children as part of the clone call, so Fabric always has a complete view of the node that's being mutated. This feature flag requires matching changes in the react-native repo: facebook/react-native#39817 ## How did you test this change? Unit test added demonstrates the new behaviour ``` yarn test -r www-modern ReactFabric-test yarn test ReactFabric-test.internal ``` Tested a manual sync into React Native and verified core surfaces render correctly. DiffTrain build for [151e75a](151e75a)

## Summary Currently when cloning nodes in Fabric, we reset a node's children on each clone, and then repeatedly call appendChild to restore the previous list of children (even if it was quasi-identical to before). This causes unnecessary invalidation of the layout state in Fabric's ShadowNode data (which in turn may require additional yoga clones) and extra JSI calls. This PR adds a feature flag to pass in the children as part of the clone call, so Fabric always has a complete view of the node that's being mutated. This feature flag requires matching changes in the react-native repo: facebook/react-native#39817 ## How did you test this change? Unit test added demonstrates the new behaviour ``` yarn test -r www-modern ReactFabric-test yarn test ReactFabric-test.internal ``` Tested a manual sync into React Native and verified core surfaces render correctly.

…experimental prefix for server action APIs (#56809) The latest React canary builds have a few changes that need to be adopted for compatability. 1. the `useFormState` and `useFormStatus` hooks in `react-dom` and the `formData` opiont in `react-dom/server` are no longer prefixed with `experimental_` 2. server content (an undocumented React feature) has been removed. Next only had trivial intenral use of this API and did not expose a coherent feature to Next users (no ability to seed context on refetches). It is still possible that some users used the React server context APIs which is why this should go into Next 14. ### React upstream changes - facebook/react#27513 - facebook/react#27514 - facebook/react#27511 - facebook/react#27508 - facebook/react#27502 - facebook/react#27474 - facebook/react#26789 - facebook/react#27500 - facebook/react#27488 - facebook/react#27458 - facebook/react#27471 - facebook/react#27470 - facebook/react#27464 - facebook/react#27456 - facebook/react#27462 - facebook/react#27461 - facebook/react#27460 - facebook/react#27459 - facebook/react#27454 - facebook/react#27457 - facebook/react#27453 - facebook/react#27401 - facebook/react#27443 - facebook/react#27445 - facebook/react#27364 - facebook/react#27440 - facebook/react#27436 --------- Co-authored-by: Zack Tanner <zacktanner@gmail.com> Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com> Co-authored-by: Jiachi Liu <inbox@huozhi.im>

## Summary Currently when cloning nodes in Fabric, we reset a node's children on each clone, and then repeatedly call appendChild to restore the previous list of children (even if it was quasi-identical to before). This causes unnecessary invalidation of the layout state in Fabric's ShadowNode data (which in turn may require additional yoga clones) and extra JSI calls. This PR adds a feature flag to pass in the children as part of the clone call, so Fabric always has a complete view of the node that's being mutated. This feature flag requires matching changes in the react-native repo: facebook/react-native#39817 ## How did you test this change? Unit test added demonstrates the new behaviour ``` yarn test -r www-modern ReactFabric-test yarn test ReactFabric-test.internal ``` Tested a manual sync into React Native and verified core surfaces render correctly.

## Summary Currently when cloning nodes in Fabric, we reset a node's children on each clone, and then repeatedly call appendChild to restore the previous list of children (even if it was quasi-identical to before). This causes unnecessary invalidation of the layout state in Fabric's ShadowNode data (which in turn may require additional yoga clones) and extra JSI calls. This PR adds a feature flag to pass in the children as part of the clone call, so Fabric always has a complete view of the node that's being mutated. This feature flag requires matching changes in the react-native repo: facebook/react-native#39817 ## How did you test this change? Unit test added demonstrates the new behaviour ``` yarn test -r www-modern ReactFabric-test yarn test ReactFabric-test.internal ``` Tested a manual sync into React Native and verified core surfaces render correctly. DiffTrain build for commit 151e75a.

facebook-github-bot added the CLA Signed label Oct 4, 2023

javache force-pushed the pass-children-when-cloning branch from daeade5 to 442a8c5 Compare October 4, 2023 16:18

acdlite reviewed Oct 4, 2023

View reviewed changes

javache changed the title ~~Pass children when cloning~~ [Fabric] Pass children when cloning Oct 4, 2023

javache force-pushed the pass-children-when-cloning branch from 442a8c5 to fe94a4e Compare October 4, 2023 21:18

javache commented Oct 5, 2023

View reviewed changes

javache marked this pull request as ready for review October 5, 2023 08:57

javache force-pushed the pass-children-when-cloning branch from fe94a4e to 837e1e6 Compare October 5, 2023 12:29

javache mentioned this pull request Oct 6, 2023

Support passing in children directly when cloning nodes facebook/react-native#39817

Closed

javache added 2 commits October 9, 2023 20:30

Add feature flag for passChildrenWhenCloningPersistedNodes

91d7328

[Fabric] Track child set as array instead of native datastructure

cc1ac3e

javache force-pushed the pass-children-when-cloning branch from 837e1e6 to f518241 Compare October 9, 2023 19:44

javache commented Oct 10, 2023

View reviewed changes

[Fabric] Pass children when cloning nodes

ad7c13a

javache force-pushed the pass-children-when-cloning branch from f518241 to ad7c13a Compare October 10, 2023 13:48

sammy-SC self-requested a review October 10, 2023 13:49

sammy-SC approved these changes Oct 10, 2023

View reviewed changes

javache merged commit 151e75a into facebook:main Oct 10, 2023
36 checks passed

javache deleted the pass-children-when-cloning branch October 10, 2023 14:11

shuding mentioned this pull request Oct 11, 2023

Upgrade vendored React vercel/next.js#56705

Closed

gnoff mentioned this pull request Oct 13, 2023

Update React from d900fadbf to 09fbee89d. Removes server context and experimental prefix for server action APIs vercel/next.js#56809

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Fabric] Pass children when cloning #27458

[Fabric] Pass children when cloning #27458

javache commented Oct 4, 2023 •

edited

Loading

react-sizebot commented Oct 4, 2023 •

edited

Loading

acdlite Oct 4, 2023 •

edited

Loading

javache Oct 4, 2023 •

edited

Loading

javache Oct 4, 2023

acdlite Oct 5, 2023

acdlite Oct 9, 2023

javache Oct 5, 2023

acdlite Oct 5, 2023

javache Oct 5, 2023

acdlite Oct 9, 2023

acdlite commented Oct 9, 2023

javache commented Oct 9, 2023

javache Oct 10, 2023

[Fabric] Pass children when cloning #27458

[Fabric] Pass children when cloning #27458

Conversation

javache commented Oct 4, 2023 • edited Loading

Summary

How did you test this change?

react-sizebot commented Oct 4, 2023 • edited Loading

Critical size changes

Significant size changes

acdlite Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

javache Oct 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acdlite commented Oct 9, 2023

javache commented Oct 9, 2023

Choose a reason for hiding this comment

javache commented Oct 4, 2023 •

edited

Loading

react-sizebot commented Oct 4, 2023 •

edited

Loading

acdlite Oct 4, 2023 •

edited

Loading

javache Oct 4, 2023 •

edited

Loading