-
Notifications
You must be signed in to change notification settings - Fork 45.7k
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: add verbose logging to react-compiler-healthcheck
#29080
base: main
Are you sure you want to change the base?
Conversation
d429825
to
4e30b93
Compare
Comparing: 5e11e7f...a33ece0 Critical size changesIncludes critical production bundles, as well as any change greater than 2%:
Significant size changesIncludes any change greater than 0.2%: Expand to show
|
ad2698e
to
25e02cb
Compare
25e02cb
to
a33ece0
Compare
@poteto, @josephsavona would appreciate if you could review this |
react-compiler-healthcheck
Can you explain more about how you would use this mode? The healthcheck command is intended as a lightweight way to get a general sense of how likely it is that your app will be compatible w the compiler. |
@josephsavona This mode would tell me the files that compile correctly, this would help me refactor those files and get rid of the manual This would also help me figure out which files break the rules of React and understand the reason they failed compilation and further investigate. |
Thanks for the context, we'll discuss as a team about where we want to take the tool and whether this fits in. The original idea was just to give folks a quick way to check how easy it would be to adopt the compiler. |
Just accept the PR! |
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.
Final code after applying all of my suggestions:
for (const compilation of [...SucessfulCompilation, ...ActionableFailures, ...OtherFailures]) {
const filename = compilation.fnLoc?.filename;
switch (compilation.kind) {
case "CompileSuccess": {
const name = compilation.fnName;
const isHook = name?.startsWith('use');
if (name) {
console.log(
chalk.green(
`Successfully compiled ${isHook ? "hook" : "component" } [${name}](${filename})`
)
);
} else {
console.log(chalk.green(`Successfully compiled ${compilation.fnLoc?.filename}`));
}
break;
}
case "CompileError": {
const { reason, severity, loc } = compilation.detail;
const lnNo = loc.start?.line;
const colNo = loc.start?.column;
const isTodo = severity === ErrorSeverity.Todo;
console.log(
chalk[isTodo ? 'yellow' : 'red'](
`Failed to compile ${
filename
}${
lnNo !== undefined ? `:${lnNo}${
colNo !== undefined ? `:${colNo}` : ""
}` : ""
}`
),
reason? `\n Reason: ${isTodo ? 'Unimplemented' : reason}` : ""
);
break;
}
default:
break;
}
}
@@ -111,7 +111,7 @@ export default { | |||
} | |||
}, | |||
|
|||
report(): void { | |||
report(verbose: boolean): void { |
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.
To make future options easier to implement, consider
report(verbose: boolean): void { | |
report({ verbose }: { verbose: boolean }): void { |
} | ||
|
||
if (compilation.kind === "CompileError") { | ||
const reason = compilation.detail.description; |
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.
I found description to be null in most cases. For example:
{
kind: 'CompileError',
fnLoc: {
start: { line: 34, column: 15, index: 993 },
end: { line: 68, column: 1, index: 1956 },
filename: 'src/components/StationFinder/components/StationsMarkers.tsx',
identifierName: undefined
},
detail: {
reason: 'React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. This value may be mutated later, which could cause the value to change unexpectedly',
description: null,
severity: 'CannotPreserveMemoization',
suggestions: null,
loc: {
start: [Object],
end: [Object],
filename: 'src/components/StationFinder/components/StationsMarkers.tsx',
identifierName: undefined
}
}
}
Also, adding exact error location would be nice.
So something like this could work better:
const { reason, severity, loc } = compilation.detail;
const lnNo = loc.start?.line;
const colNo = loc.start?.column;
const isTodo = severity === ErrorSeverity.Todo;
console.log(
chalk[isTodo ? 'yellow' : 'red'](
`Failed to compile ${
filename
}${
lnNo !== undefined ? `:${lnNo}${
colNo !== undefined ? `:${colNo}` : ""
}` : ""
}`
),
reason? `\n Reason: ${isTodo ? 'Unimplemented' : reason}` : ""
);
for (const compilation of [...SucessfulCompilation, ...ActionableFailures, ...OtherFailures]) { | ||
const filename = compilation.fnLoc?.filename; | ||
|
||
if (compilation.kind === "CompileSuccess") { |
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.
Could be a switch()
} | ||
} | ||
|
||
if (compilation.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.
Certain errors have severity ErrorSeverity.Todo
. I don't think it's worth making it all red, but I don't think we should skip this information either. So perhaps make it a different color or something?
I think that’s what the ESLint plugin is for, rather than the healthcheck tool. |
ESLint plugin is nowhere near as informative. It doesn't tell you when React Compiler bails out because of TODOs on RC side, or manual memoization. |
That's why we need a detailed list of files with the exact issues they have. When this tool tells me "issue in 100 components out of 1000" - it gives me no info, I still have 0 clue on how easy will it be to integrate anything. So I have to install eslint plugin, run the scan over my codebase, and eslint tells me that I have an issue not with 100 files, but with 6. What kind of conclusion may I have? Probably only 1 - react compiler is not even close to being production-ready, so I need to wait until the tooling around it becomes more useful. Currently, it feels like this utility is not just useless, but misleading. And until there will be no clear way to check it (like having a path to problematic file, with an eslint error there), people won't trust this tool |
@wojtekmaj I appreciate the suggestions. |
I'm considering using this tool in CI. We use Biome for linting/formatting etc, so we cannot use the eslint plugin. There is a feature request for Biome and it looks like there isn't going to be support any time soon, since Biome is written in Rust. There exists a non-maintained, not-supported Rust implementation of RC which could be used as a base, but that would take long, too (the Rust version also has its own parse tree, query engine etc, which may not be so ideal for use in Biome). Maybe Biome will support plugins some day, so we can implement a plugin that works similar to how the eslint plugin works. For me, using the react-compiler-healthcheck would solve most of my problems if it prints the list of affected files and returns with != 0 exit code if there were incompatible components. |
That is a fair use case you’ve mentioned @nikeee. In the meantime you can use the changes in this PR and patch the package locally. |
Summary
This PR adds a verbose option to
react-compiler-healthcheck
CLI which logs all the compiled components/hooks names and files location.Fixes: #29078
Test plan
Run the
react-compiler-healthcheck
CLI with the—-verbose
flag inside a react app and you should be able to see the compiled components/hooks names or the files path printed in green.If there are any files that failed to compile, they will be printed in red.