-
Notifications
You must be signed in to change notification settings - Fork 24
build: Ship components optimized with React Compiler #992
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pull request successfully integrates the React Compiler into the build process, which is a significant step towards improving runtime performance. The changes to the build configuration and dependencies are logical. I've identified two minor issues in the custom logger implementation within babel.config.js: one regarding inconsistent log output that could hinder debugging, and another related to potential runtime errors due to unsafe property access. After addressing these points, the PR should be ready for merging.
| event.kind === 'CompileSkip' || | ||
| event.kind === 'PipelineError' | ||
| ) { | ||
| if (event.kind === 'CompileError') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P2] Inconsistent Log Header: The current logging logic only prints a contextual header with the filename for CompileError events. For CompileSkip and PipelineError events, this header is omitted, which makes it difficult to determine which file the logged details refer to. To improve debuggability, a header should be logged for all handled event types.
| if (event.kind === 'CompileError') { | |
| if (event.kind === 'PipelineError') { | |
| console.error( | |
| `\n[React Compiler] \x1b[31mCompilation failed\x1b[0m: ${filename}`, | |
| ) | |
| } else { | |
| console.warn( | |
| `\n[React Compiler] \x1b[33mCompilation skipped\x1b[0m: ${filename}`, | |
| ) | |
| } |
| console.error(`Details: ${event.detail.description}`) | ||
| } | ||
|
|
||
| if (event.detail.primaryLocation) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[P1] Potential Runtime Error: The event.detail property may be undefined for some event kinds, such as PipelineError. Accessing event.detail.primaryLocation directly can cause a TypeError and crash the logging utility. Using optional chaining (?.) here will prevent this potential runtime error.
| if (event.detail.primaryLocation) { | |
| if (event.detail?.primaryLocation) { |
Short description
This integrates the React compiler to optimize components within the distributed packages, minimizing re-renders.
Caveats are that not all components/hooks can be optimized at this time, and those that are have not been deprecated will be addressed in future PRs:
The bundle size would also be increased by about 37%, due to the compiler's generated code (but not the runtime, which is externalized):
The left shows the bundle size using the current build step on main, using Rollup's TypeScript plugin. The right is with the compiler enabled with Babel, which had some of babel's runtime helpers inlined. Once externalized, the bundle size is decreased slightly:
Taking out the React compiler, while leaving the Babel build flow in place, results in a smaller bundle compared to when building with the TypeScript plugin (but very slightly larger gzipped), confirming that the size increase can be mostly attributed to the compiler's output.
References
https://react.dev/reference/react-compiler/compiling-libraries
PR Checklist