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

compiler: Handle TSNonNullAssertion expressions #29218

Merged
merged 1 commit into from
May 22, 2024

Conversation

henryqdineen
Copy link
Contributor

Summary

We ran React compiler against part of our codebase and collected compiler errors. One of the more common non-actionable errors is caused by usage of the ! TypeScript non-null assertion operation:

(BuildHIR::lowerExpression) Handle TSNonNullExpression expressions

It seems like React Compiler should be able to support this by just ignoring the syntax and using the underlying expression. I'm sure a lot of our non-null assertion usage should not exist and I understand if React Compiler does not want to support this syntax. It wasn't obvious to me if this omission was intentional or if there are future plans to use TSNonNullExpression as part of the compiler's analysis. If there are no future plans it seems like just ignoring it should be fine.

How did you test this change?

❯ yarn snap --filter
yarn run v1.17.3
$ yarn workspace babel-plugin-react-compiler run snap --filter
$ node ../snap/dist/main.js --filter
 PASS  non-null-assertion
1 Tests, 1 Passed, 0 Failed

Copy link

vercel bot commented May 22, 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 May 22, 2024 6:02pm

@react-sizebot
Copy link

react-sizebot commented May 22, 2024

Comparing: 3ac551e...cff78dd

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 = 495.89 kB 495.89 kB = 88.82 kB 88.83 kB
oss-experimental/react-dom/cjs/react-dom.production.js = 6.67 kB 6.67 kB = 1.83 kB 1.83 kB
oss-experimental/react-dom/cjs/react-dom-client.production.js = 500.69 kB 500.69 kB = 89.51 kB 89.51 kB
facebook-www/ReactDOM-prod.classic.js = 593.05 kB 593.05 kB = 104.32 kB 104.32 kB
facebook-www/ReactDOM-prod.modern.js = 569.27 kB 569.27 kB = 100.72 kB 100.72 kB
test_utils/ReactAllWarnings.js Deleted 64.35 kB 0.00 kB Deleted 16.05 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 64.35 kB 0.00 kB Deleted 16.05 kB 0.00 kB

Generated by 🚫 dangerJS against cff78dd

@josephsavona
Copy link
Contributor

Awesome, thanks for the contribution! We may eventually choose to use information from non-null assertions in some additional ways, but we'd test any such changes carefully. This makes sense to enable.

Thanks again!

@josephsavona josephsavona merged commit 4c2e457 into facebook:main May 22, 2024
50 checks passed
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