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

Log Fragment name when trying to render a lazy fragment #30372

Merged
merged 6 commits into from
Jul 23, 2024

Conversation

tom-sherman
Copy link
Contributor

@tom-sherman tom-sherman commented Jul 18, 2024

Summary

Fixes #26910

How did you test this change?

See test.

Copy link

vercel bot commented Jul 18, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-compiler-playground ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 23, 2024 4:47pm

@react-sizebot
Copy link

react-sizebot commented Jul 18, 2024

Comparing: 163365a...228669e

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 +0.35% 499.44 kB 501.17 kB +0.40% 89.59 kB 89.95 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 +0.34% 504.26 kB 505.99 kB +0.39% 90.30 kB 90.66 kB
facebook-www/ReactDOM-prod.classic.js +0.01% 599.20 kB 599.28 kB +0.02% 105.80 kB 105.82 kB
facebook-www/ReactDOM-prod.modern.js +0.37% 573.37 kB 575.47 kB +0.43% 101.68 kB 102.11 kB
test_utils/ReactAllWarnings.js Deleted 64.07 kB 0.00 kB Deleted 15.86 kB 0.00 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show
Name +/- Base Current +/- gzip Base gzip Current gzip
facebook-www/ReactART-prod.modern.js +0.60% 348.84 kB 350.94 kB +0.75% 59.19 kB 59.63 kB
oss-stable-rc/react-art/cjs/react-art.production.js +0.59% 292.58 kB 294.30 kB +0.72% 50.59 kB 50.96 kB
oss-stable-semver/react-art/cjs/react-art.production.js +0.59% 292.58 kB 294.30 kB +0.72% 50.59 kB 50.96 kB
oss-stable/react-art/cjs/react-art.production.js +0.59% 292.62 kB 294.35 kB +0.72% 50.62 kB 50.98 kB
oss-experimental/react-art/cjs/react-art.production.js +0.58% 297.27 kB 299.00 kB +0.70% 51.38 kB 51.74 kB
react-native/implementations/ReactFabric-prod.js +0.50% 344.76 kB 346.47 kB +0.63% 60.51 kB 60.90 kB
react-native/implementations/ReactNativeRenderer-prod.js +0.48% 354.15 kB 355.86 kB +0.61% 62.17 kB 62.55 kB
react-native/implementations/ReactFabric-profiling.js +0.46% 371.99 kB 373.70 kB +0.60% 64.69 kB 65.07 kB
react-native/implementations/ReactNativeRenderer-profiling.js +0.45% 381.31 kB 383.02 kB +0.58% 66.43 kB 66.81 kB
facebook-www/ReactDOM-prod.modern.js +0.37% 573.37 kB 575.47 kB +0.43% 101.68 kB 102.11 kB
facebook-www/ReactDOM-profiling.modern.js +0.35% 602.59 kB 604.69 kB +0.40% 105.94 kB 106.36 kB
oss-stable-rc/react-dom/cjs/react-dom-client.production.js +0.35% 499.34 kB 501.07 kB +0.40% 89.57 kB 89.93 kB
oss-stable-semver/react-dom/cjs/react-dom-client.production.js +0.35% 499.34 kB 501.07 kB +0.40% 89.57 kB 89.93 kB
oss-stable/react-dom/cjs/react-dom-client.production.js +0.35% 499.44 kB 501.17 kB +0.40% 89.59 kB 89.95 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js +0.34% 504.26 kB 505.99 kB +0.39% 90.30 kB 90.66 kB
oss-stable-rc/react-dom/cjs/react-dom-profiling.profiling.js +0.32% 532.12 kB 533.85 kB +0.38% 94.75 kB 95.11 kB
oss-stable-semver/react-dom/cjs/react-dom-profiling.profiling.js +0.32% 532.12 kB 533.85 kB +0.38% 94.75 kB 95.11 kB
oss-stable/react-dom/cjs/react-dom-profiling.profiling.js +0.32% 532.23 kB 533.95 kB +0.38% 94.77 kB 95.14 kB
oss-experimental/react-dom/cjs/react-dom-profiling.profiling.js +0.32% 537.04 kB 538.77 kB +0.40% 95.47 kB 95.85 kB
test_utils/ReactAllWarnings.js Deleted 64.07 kB 0.00 kB Deleted 15.86 kB 0.00 kB

Generated by 🚫 dangerJS against 228669e

@@ -1891,11 +1892,13 @@ function mountLazyComponent(
}
}

const loggedComponent = Component === REACT_FRAGMENT_TYPE ? '<Fragment>' : Component;
Copy link
Collaborator

Choose a reason for hiding this comment

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

We can use getComponentNameFromType from shared/getComponentNameFromType here instead. That way we also get a better name for all the other types you're not supposed to use.

Suggested change
const loggedComponent = Component === REACT_FRAGMENT_TYPE ? '<Fragment>' : Component;
const loggedComponent = getComponentNameFromType(Component) || String(Component);

Copy link
Collaborator

Choose a reason for hiding this comment

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

Forgot that we use + '' for perf so || (Component + '') it is.

tom-sherman and others added 3 commits July 23, 2024 10:05
Co-authored-by: Sebastian Silbermann <silbermann.sebastian@gmail.com>
@tom-sherman tom-sherman requested a review from eps1lon July 23, 2024 09:40
@eps1lon eps1lon changed the title Log proper error when trying to render a lazy fragment Log Fragment name when trying to render a lazy fragment Jul 23, 2024
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.

Thanks

@eps1lon
Copy link
Collaborator

eps1lon commented Jul 23, 2024

Prettier and lint failing.

@tom-sherman
Copy link
Contributor Author

@eps1lon I chose to ignore the string coercion lint, let me know if that's not ok and I can find a different way forward - probably by introducing a new string coercion check function for components?

@eps1lon
Copy link
Collaborator

eps1lon commented Jul 23, 2024

Let's just be simple about it and do what the lint rule asks.

@eps1lon
Copy link
Collaborator

eps1lon commented Jul 23, 2024

Seems like common practice though.

@tom-sherman
Copy link
Contributor Author

Let's just be simple about it and do what the lint rule asks.

What solution are you thinking of? The lint rule asks to use a "checkXxxxxStringCoercion" function but none exists for components right now, which is why I suggested creating one.

@eps1lon
Copy link
Collaborator

eps1lon commented Jul 23, 2024

What solution are you thinking of?

Just kept the original behavior.

@eps1lon eps1lon merged commit 9cc0f6e into facebook:main Jul 23, 2024
190 checks passed
@tom-sherman tom-sherman deleted the gh-26910 branch July 23, 2024 17:06
github-actions bot pushed a commit that referenced this pull request Jul 23, 2024
github-actions bot pushed a commit that referenced this pull request Jul 23, 2024
felixshiftellecon added a commit to felixshiftellecon/react that referenced this pull request Jul 24, 2024
felixshiftellecon added a commit to felixshiftellecon/react that referenced this pull request Jul 24, 2024
felixshiftellecon added a commit to felixshiftellecon/react that referenced this pull request Jul 24, 2024
felixshiftellecon added a commit to felixshiftellecon/react that referenced this pull request Jul 24, 2024
felixshiftellecon added a commit to felixshiftellecon/react that referenced this pull request Jul 24, 2024
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.

Bug: React fails to log invariant 306 message when lazy() resolves to a Fragment
4 participants