Skip to content

New children notify fragment instances in Fabric #33093

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 21, 2025

Conversation

jackpope
Copy link
Member

@jackpope jackpope commented May 1, 2025

When a new child of a fragment instance is inserted, we need to notify the instance to keep any relevant tracking up to date. For example, we automatically observe the new child with any active IntersectionObserver.

For mutable renderers (DOM), we reuse the existing traversal in commitPlacement that does the insertions for HostComponents. Immutable renderers (Fabric) exit this path before the traversal though, so currently we can't notify the fragment instances.

Here I've created a separate traversal in commitPlacement, specifically for immutable renders when enableFragmentRefs is on.

@react-sizebot
Copy link

react-sizebot commented May 1, 2025

Comparing: 4448b18...503b959

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.js = 6.68 kB 6.68 kB = 1.83 kB 1.83 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 529.74 kB 529.74 kB = 93.49 kB 93.49 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.69 kB 6.69 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 651.48 kB 651.37 kB = 114.76 kB 114.74 kB
facebook-www/ReactDOM-prod.classic.js = 675.72 kB 675.59 kB = 118.85 kB 118.86 kB
facebook-www/ReactDOM-prod.modern.js = 666.00 kB 665.87 kB +0.01% 117.23 kB 117.25 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.57% 385.06 kB 387.26 kB +0.46% 66.53 kB 66.84 kB
react-native/implementations/ReactFabric-profiling.fb.js +0.53% 413.27 kB 415.46 kB +0.42% 70.64 kB 70.94 kB
react-native/implementations/ReactFabric-dev.fb.js +0.33% 674.86 kB 677.10 kB +0.27% 109.53 kB 109.83 kB

Generated by 🚫 dangerJS against 503b959

@jackpope
Copy link
Member Author

jackpope commented May 7, 2025

This is too early and causes the node to be observed before layout information is available. Going to to work on moving it later.

@jackpope jackpope marked this pull request as draft May 7, 2025 15:20
@jackpope jackpope force-pushed the fr-rn-placements branch from 9d3ddac to ce8e004 Compare May 16, 2025 14:34
@jackpope jackpope force-pushed the fr-rn-placements branch from ce8e004 to cd6dbf0 Compare May 16, 2025 16:02
@jackpope jackpope marked this pull request as ready for review May 16, 2025 16:42
@jackpope
Copy link
Member Author

Republishing as @rubennorte is working on RN changes to allow the earlier observe on new children.

@jackpope jackpope requested a review from jbrown215 May 19, 2025 23:48

if (!supportsMutation) {
if (enableFragmentRefs) {
appendImmutableNodeToFragmentInstances(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what supportsMutation means, but wanted to point out that we are appending something in the fragment refs path which might be considered a mutation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

append isn't great naming here. I'll update.

supportsMutation refers to mutation mode https://github.com/facebook/react/blob/main/packages/react-reconciler/README.md#modes

In this case, we're targeting Fabric behavior which does not support mutation.

The append isn't an append to a mutable tree. Its more like letting the parent fragment know it has a new child, independent of the actual insert of that node.

@jackpope jackpope merged commit 1835b3f into facebook:main May 21, 2025
238 of 239 checks passed
@jackpope jackpope deleted the fr-rn-placements branch May 21, 2025 19:47
github-actions bot pushed a commit that referenced this pull request May 21, 2025
When a new child of a fragment instance is inserted, we need to notify
the instance to keep any relevant tracking up to date. For example, we
automatically observe the new child with any active
IntersectionObserver.

For mutable renderers (DOM), we reuse the existing traversal in
`commitPlacement` that does the insertions for HostComponents. Immutable
renderers (Fabric) exit this path before the traversal though, so
currently we can't notify the fragment instances.

Here I've created a separate traversal in `commitPlacement`,
specifically for immutable renders when `enableFragmentRefs` is on.

DiffTrain build for [1835b3f](1835b3f)
github-actions bot pushed a commit that referenced this pull request May 21, 2025
When a new child of a fragment instance is inserted, we need to notify
the instance to keep any relevant tracking up to date. For example, we
automatically observe the new child with any active
IntersectionObserver.

For mutable renderers (DOM), we reuse the existing traversal in
`commitPlacement` that does the insertions for HostComponents. Immutable
renderers (Fabric) exit this path before the traversal though, so
currently we can't notify the fragment instances.

Here I've created a separate traversal in `commitPlacement`,
specifically for immutable renders when `enableFragmentRefs` is on.

DiffTrain build for [1835b3f](1835b3f)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants