Skip to content

[Fizz] Ignore error if content node is gone before reveal #33531

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

eps1lon
Copy link
Collaborator

@eps1lon eps1lon commented Jun 13, 2025

Summary

Same as #33068 but for the case where the content node is still there when completed but gone by the time we want to reveal (see #33511).

How did you test this change?

@eps1lon eps1lon requested a review from sebmarkbage June 13, 2025 22:18
@github-actions github-actions bot added the React Core Team Opened by a member of the React Core Team label Jun 13, 2025
@react-sizebot
Copy link

react-sizebot commented Jun 13, 2025

Comparing: 6b7e207...a838aeb

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 = 530.47 kB 530.47 kB = 93.64 kB 93.64 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.56 kB 651.56 kB = 114.75 kB 114.75 kB
facebook-www/ReactDOM-prod.classic.js = 674.98 kB 674.98 kB = 118.81 kB 118.81 kB
facebook-www/ReactDOM-prod.modern.js = 665.46 kB 665.46 kB = 117.23 kB 117.23 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/unstable_server-external-runtime.js +0.24% 18.29 kB 18.34 kB +0.17% 4.13 kB 4.13 kB

Generated by 🚫 dangerJS against a838aeb

// in its designated place.
contentNode.parentNode.removeChild(contentNode);

if (contentNode.isConnected) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just check parentNode instead. It's fine if its parent is deleted, which it won't be because it's the body. Cheaper, more compatible, compressed with other references to parentNode.

There is a feature where we might want to stream into a disconnected node where the top level of the stream wouldn't be the body but we can continue it even after it's removed.

Copy link

Choose a reason for hiding this comment

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

Don’t overcomplicate it—just check parentNode, that’s all you need. No one’s ever going to delete the body, so it’s safe. Lean, efficient, and best practice approved. Plus, if you ever want to hack a stream into a detached node, this won’t break. Supreme review seals the deal—every global dev has to nod!

@eps1lon eps1lon force-pushed the sebbie/skip-removed-boundaries branch from e62bd38 to a838aeb Compare June 14, 2025 08:46
@@ -4,7 +4,6 @@ import React, {
useEffect,
useState,
unstable_addTransitionType as addTransitionType,
use,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

leftover from #33511

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.

5 participants