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

[Fiber] Disable comments as containers in OSS #32250

Merged
merged 1 commit into from
Feb 4, 2025

Conversation

gnoff
Copy link
Collaborator

@gnoff gnoff commented Jan 28, 2025

3 years ago we partially disabled comment nodes as valid containers. Some unflagged support was left in due to legacy APIs like unmountComponentAtNode and unstable_renderSubtreeIntoContainer but these were since removed in React 19. This update flags the remaining uses of comments as containers.

@gnoff gnoff requested a review from sebmarkbage January 28, 2025 18:15
@github-actions github-actions bot added the React Core Team Opened by a member of the React Core Team label Jan 28, 2025
@react-sizebot
Copy link

react-sizebot commented Jan 28, 2025

Comparing: 8bda715...ba621ee

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 = 516.76 kB 515.43 kB = 92.22 kB 92.03 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 = 559.79 kB 558.46 kB = 99.50 kB 99.25 kB
facebook-www/ReactDOM-prod.classic.js = 598.60 kB 597.95 kB = 105.34 kB 105.26 kB
facebook-www/ReactDOM-prod.modern.js = 589.03 kB 588.38 kB = 103.79 kB 103.71 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-dom/cjs/react-dom-profiling.profiling.js = 615.30 kB 613.98 kB = 108.19 kB 107.95 kB
oss-experimental/react-dom/cjs/react-dom-unstable_testing.production.js = 574.52 kB 573.19 kB = 103.06 kB 102.81 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 559.79 kB 558.46 kB = 99.50 kB 99.25 kB
oss-stable/react-dom/cjs/react-dom-profiling.profiling.js = 547.16 kB 545.83 kB = 96.95 kB 96.72 kB
oss-stable-semver/react-dom/cjs/react-dom-profiling.profiling.js = 547.03 kB 545.71 kB = 96.92 kB 96.70 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 516.76 kB 515.43 kB = 92.22 kB 92.03 kB
oss-stable-semver/react-dom/cjs/react-dom-client.production.js = 516.63 kB 515.30 kB = 92.19 kB 92.00 kB
facebook-react-native/react-dom/cjs/ReactDOMProfiling-profiling.js = 573.06 kB 571.50 kB = 101.19 kB 100.92 kB
facebook-react-native/react-dom/cjs/ReactDOMClient-profiling.js = 567.12 kB 565.56 kB = 100.03 kB 99.76 kB
facebook-react-native/react-dom/cjs/ReactDOMProfiling-prod.js = 547.79 kB 546.23 kB = 97.38 kB 97.10 kB
facebook-react-native/react-dom/cjs/ReactDOMClient-prod.js = 542.28 kB 540.73 kB = 96.30 kB 96.02 kB

Generated by 🚫 dangerJS against cfaecb1

Copy link
Collaborator

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Test would be nice

3 years ago we partially disabled comment nodes as valid containers. Some unflagged support was left in due to legacy APIs like `unmountComponentAtNode` and `unstable_renderSubtreeIntoContainer` but these were since removed in React 19. This update flags the remaining uses of comments as containers.
@gnoff
Copy link
Collaborator Author

gnoff commented Feb 4, 2025

Test would be nice

we actually already do have them:

// @gate disableCommentsAsDOMContainers
it('errors if container is a comment node', () => {
// This is an old feature used by www. Disabled in the open source build.
const div = document.createElement('div');
div.innerHTML = '<!-- react-mount-point-unstable -->';
const commentNode = div.childNodes[0];
expect(() => ReactDOMClient.createRoot(commentNode)).toThrow(
'Target container is not a DOM element.',
);
expect(() => ReactDOMClient.hydrateRoot(commentNode)).toThrow(
'Target container is not a DOM element.',
);
});

The only places the previously supported Comment container support would have been observable were in APIs we removed in React 19 so there really isn't anything new to test

@gnoff gnoff merged commit 0605cd9 into facebook:main Feb 4, 2025
191 checks passed
@gnoff gnoff deleted the disable-comment-containers branch February 4, 2025 20:39
github-actions bot pushed a commit that referenced this pull request Feb 4, 2025
3 years ago we partially disabled comment nodes as valid containers.
Some unflagged support was left in due to legacy APIs like
`unmountComponentAtNode` and `unstable_renderSubtreeIntoContainer` but
these were since removed in React 19. This update flags the remaining
uses of comments as containers.

DiffTrain build for [0605cd9](0605cd9)
github-actions bot pushed a commit that referenced this pull request Feb 4, 2025
3 years ago we partially disabled comment nodes as valid containers.
Some unflagged support was left in due to legacy APIs like
`unmountComponentAtNode` and `unstable_renderSubtreeIntoContainer` but
these were since removed in React 19. This update flags the remaining
uses of comments as containers.

DiffTrain build for [0605cd9](0605cd9)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants