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] Patch error reporting for blocklisted imports #30652

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

mofeiZ
Copy link
Contributor

@mofeiZ mofeiZ commented Aug 9, 2024

Stack from ghstack (oldest at bottom):


Call handleError to log (and throw, depending on panicThreshold) instead of always throwing an exception. Discovered when syncing internally and failing the babel build

Also added a comment to compileProgram for future reference (pasted below)
compileProgram is directly invoked by the react-compiler babel plugin, so exceptions thrown by this function will fail the babel build.

  • call handleError if your error is recoverable.
    Unless the error is a warning / info diagnostic, compilation of a function / entire file should also be skipped.
  • throw an exception if the error is fatal / not recoverable.
    Examples of this are invalid compiler configs or failure to codegen outlined functions after already emitting optimized components / hooks that invoke the outlined functions.

It looks like this wasn't caught in #30643 because we run snap tests with panicThreshold: 'all_errors'. Thoughts on switching to panicThreshold: 'none' and explicitly labeling fatal errors vs compilation skipped?

Copy link

vercel bot commented Aug 9, 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 Aug 9, 2024 8:47pm

mofeiZ added a commit that referenced this pull request Aug 9, 2024
ghstack-source-id: 614c1e9c04828bfa2da13a6abaeff7ce3e67cb9b
Pull Request resolved: #30652
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Aug 9, 2024
@mofeiZ mofeiZ marked this pull request as ready for review August 9, 2024 20:49
@josephsavona
Copy link
Contributor

Thoughts on switching to panicThreshold: 'none' and explicitly labeling fatal errors vs compilation skipped?

100%

@mofeiZ mofeiZ merged commit 3c91815 into gh/mofeiZ/14/base Aug 9, 2024
19 checks passed
mofeiZ added a commit that referenced this pull request Aug 9, 2024
ghstack-source-id: 614c1e9c04828bfa2da13a6abaeff7ce3e67cb9b
Pull Request resolved: #30652
@mofeiZ mofeiZ deleted the gh/mofeiZ/14/head branch August 9, 2024 22:19
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.

3 participants