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

fix(flight): improve stack type check to avoid throwing on undefined #29990

Closed
wants to merge 1 commit into from

Conversation

kevva
Copy link

@kevva kevva commented Jun 20, 2024

Summary

Recently, an issue on the Next.js repo arose when importing SVGs in a server component using the svgr Webpack loader. I tracked it down to this release and when I debugged it using the debugger in VS Code I found this line which was recently added in #29632. It explicitly checks for null here but apparently it can be undefined. I added so we always fallback to null in the tuple but also hardened the stack check to make sure it's a string.

How did you test this change?

I hardcoded the changes into the react version in use by Next.js and tested importing a SVG which works fine.

Copy link

vercel bot commented Jun 20, 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 Jun 20, 2024 1:36pm

@react-sizebot
Copy link

Comparing: b15c849...e6657b2

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.66 kB 6.66 kB +0.05% 1.82 kB 1.82 kB
oss-stable/react-dom/cjs/react-dom-client.production.js = 497.93 kB 497.93 kB = 89.26 kB 89.26 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB +0.05% 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 502.75 kB 502.75 kB = 89.96 kB 89.96 kB
facebook-www/ReactDOM-prod.classic.js = 597.17 kB 597.17 kB = 105.33 kB 105.33 kB
facebook-www/ReactDOM-prod.modern.js = 571.52 kB 571.52 kB = 101.27 kB 101.27 kB
test_utils/ReactAllWarnings.js Deleted 62.88 kB 0.00 kB Deleted 15.69 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
test_utils/ReactAllWarnings.js Deleted 62.88 kB 0.00 kB Deleted 15.69 kB 0.00 kB

Generated by 🚫 dangerJS against e6657b2

@sebmarkbage
Copy link
Collaborator

We are intentionally not permissive to find bugs.

My guess would be that this was fixed by #29762

It could also be that maybe SVGR embeds an old copy of React for JSX or some misconfigured environment but I think most likely this was already fixed above and a newer version of Next.js would have it fixed.

@kevva
Copy link
Author

kevva commented Jun 21, 2024

Gotcha, thanks for the link to that PR. I've been testing to see if this is fixed in every new canary version of Next.js but so far it's still not working. And by the looks of things, that PR should already have landed in Next.js.

I don't see anything weird going on in SVGR either.

@kevva kevva deleted the km/fix-stack-type-check branch June 21, 2024 12:02
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