Skip to content
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

Reconciler: Combine identical cases in findParent #32210

Merged
merged 2 commits into from
Jan 31, 2025

Conversation

OnlyWick
Copy link
Contributor

@OnlyWick OnlyWick commented Jan 24, 2025

Summary

When lookup Parent, HostRoot and HostPortal should be merged, because when creating a Portal, it will also include containerInfo(So we can directly use this containerInfo to delete the real DOM nodes.), so there is no need to handle them separately.

How did you test this change?

No behavior changes, all existing tests pass.

@OnlyWick
Copy link
Contributor Author

Based on the code I am currently debugging, HostRoot and HostPortal have some commonalities.

see below code:

case HostRoot:
case HostPortal: {
const parent: Container = parentFiber.stateNode.containerInfo;
const before = getHostSibling(finishedWork);
insertOrAppendPlacementNodeIntoContainer(finishedWork, before, parent);
break;
}

case HostRoot:
case HostPortal: {
if (supportsResources) {
const previousHoistableRoot = currentHoistableRoot;
const container: Container = fiber.stateNode.containerInfo;
currentHoistableRoot = getHoistableRoot(container);
recursivelyAccumulateSuspenseyCommit(fiber);
currentHoistableRoot = previousHoistableRoot;
} else {
recursivelyAccumulateSuspenseyCommit(fiber);
}
break;
}

@gnoff
Copy link
Collaborator

gnoff commented Jan 31, 2025

Can you edit your PR top comment to contain a description of what is changing and why?

@OnlyWick
Copy link
Contributor Author

@gnoff Done, I haven't done more exploration on Portal yet(But the code here does feel a bit redundant, because Portal has a containerInfo property, and it is a container.)

@react-sizebot
Copy link

Comparing: a657bc5...3baa725

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 = 514.64 kB 514.52 kB = 91.76 kB 91.76 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 = 557.67 kB 557.55 kB = 99.01 kB 99.01 kB
facebook-www/ReactDOM-prod.classic.js = 596.61 kB 596.50 kB = 104.89 kB 104.89 kB
facebook-www/ReactDOM-prod.modern.js = 587.04 kB 586.92 kB = 103.36 kB 103.35 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 90027d7

@gnoff gnoff merged commit 19ca800 into facebook:main Jan 31, 2025
191 checks passed
@gnoff
Copy link
Collaborator

gnoff commented Jan 31, 2025

ty!

github-actions bot pushed a commit that referenced this pull request Jan 31, 2025
## Summary

When lookup `Parent`, `HostRoot` and `HostPortal` should be merged,
because when creating a `Portal`, it will also include
`containerInfo`(So we can directly use this `containerInfo` to delete
the real DOM nodes.), so there is no need to handle them separately.

## How did you test this change?

No behavior changes, all existing tests pass.

DiffTrain build for [19ca800](19ca800)
github-actions bot pushed a commit that referenced this pull request Jan 31, 2025
## Summary

When lookup `Parent`, `HostRoot` and `HostPortal` should be merged,
because when creating a `Portal`, it will also include
`containerInfo`(So we can directly use this `containerInfo` to delete
the real DOM nodes.), so there is no need to handle them separately.

## How did you test this change?

No behavior changes, all existing tests pass.

DiffTrain build for [19ca800](19ca800)
@OnlyWick OnlyWick deleted the combine-switch-case branch February 1, 2025 03:23
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