perf(react): improve react compiler preset so that slightly more modules are filtered out#1138
Conversation
…les are filtered out
| options.compilationMode === 'annotation' | ||
| ? /['"]use memo['"]/ | ||
| : /\b[A-Z]|\buse/, | ||
| : /\b[A-Z]|\buse[A-Z0-9]/, |
There was a problem hiding this comment.
I think the only way to define a function without a whitespace before is const A,B= ... which I don't know why you would do that in anyworld for defining hooks or components
| : /\b[A-Z]|\buse[A-Z0-9]/, | |
| : /\s[A-Z]|\suse[A-Z0-9]/, |
But I think the better tradeoff is always running on jsx|tsx|mdx (in a normal codebase, you have like 98/99% of jsx files that contrains components, so not sure trying to get a 2% improvement with a regex is worth it) and run on js|ts only with when code matches \suse[A-Z0-9] (Vite always ban using JSX inside JS, nobody writes manual jsx calls and plugin should not rename file ids once transform from jsx to js
The only issue with the approach if for people having custom DSL that compile to React, but I think these frameworks should provide their own react compiler preset
Also can you put the link from the PR into the source code? I think it's good to have it as a reference if someone challenges it later
There was a problem hiding this comment.
If it's possible to have , before the name, I think we can use [\s,]. I guess \s and [\s,] won't have much perf difference.
I think the better tradeoff is always running on
jsx|tsx|mdx(in a normal codebase, you have like 98/99% of jsx files that contrains components, so not sure trying to get a 2% improvement with a regex is worth it) and run onjs|tsonly with when code matches\suse[A-Z0-9](Vite always ban using JSX inside JS, nobody writes manualjsxcalls and plugin should not rename file ids once transform from jsx to js
It's currently difficult to achieve, but it would be easier to do once Vite implements composable filters (https://github.com/vitejs/rolldown-vite/issues/605).
There was a problem hiding this comment.
After some AI checking, I think this is pretty safe and code not matching this regex will certainly break a lot a React rules:
(?:const|let|var|function)\s+(?:[A-Z]|use[A-Z0-9])
(The way to break it is using new Function or putting your component in a class method, both sounds horrible)
Speaking of classes, I checked and the playground doesn't touch class component, so no need to match this
Edit: Ok no actually this would be valid IMO but the playground doesn't touch it:
import { useState } from "react";
const hooks = {
useTest: () => {
const [state, setState] = useState(0);
if (state === 0) return <div>Nothing</div>
else return <div>{state}</div>
}
}I think the React compiler is only looking for top-level functions, and this sounds reasonable, so we can make an more aggressive regex
There was a problem hiding this comment.
The way to break it is using
new Functionor putting your component in a class method, both sounds horrible
I guess new Function cannot be handled by the compiler anyways. Classes (& class methods) are skipped here.
I think the React compiler is only looking for top-level functions
In compilationMode: 'all', it seems to only look for top-level ones. But I guess it does otherwise.
Something like this won't match the regex you suggested, while the compiler optimizes:
import React, { useState } from "react";
import { jsx } from "react/jsx-runtime"
export const components = {
A: React.memo(() => {
const [state, setStae] = useState(0);
return jsx("div", { children: state })
})
}playground
This case matches this part.
That said, I think we can ignore this case if the speed up is big enough.
There was a problem hiding this comment.
It feels quite unexpected that the component is optimized only if wrap a in a specific function.
We could also add ["']react['"] to the regex in that case, it should cover a lot of extra cases like this while still skipping files that are unrelated to React
There was a problem hiding this comment.
No I would call this one safe because it's really try to match cases where functions are anonymous, which should be avoided and linted against
There was a problem hiding this comment.
Because top-level and nested are matched with the same part of the regex, now it doesn't work for top level component with complex type annotation:
const X: SomeComplexType<Generic, number> = () => {
return <></>
}There was a problem hiding this comment.
Ah, I forgot that we need to handle TS code. I've fixed that now.
I also noticed that this filter doesn't work when there're comments in between.
There was a problem hiding this comment.
I think you actively trying to break your tools if you use comments between const/function and the function name so I would not bother handling these
There was a problem hiding this comment.
yeah, I think the same
I noticed that we can add
[A-Z0-9]to filter out a bit more modules.https://github.com/facebook/react/blob/9c0323e2cf9be543d6eaa44419598af56922603f/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts#L897-L899