Skip to content

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?

@vercel
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
@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
Member

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
mofeiZ added a commit that referenced this pull request Aug 9, 2024
@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.

4 participants