-
-
Notifications
You must be signed in to change notification settings - Fork 7.4k
feat(create-vite): support React Compiler #20704
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
Merged
Merged
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
one thing that's missing is also using [email protected]. Version 6 will include compiler diagnostics as part of the plugin by default.
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.
That's quite a bump in dependencies. plugin-proposal-private-methods is deprecated, and I'm surprise of the need for zod in a linter plugin.
Is the hermes parser necessary given that the build will use the babel parser?
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.
Not as much an issue, but are you open to switching to ESM for the next major? ESLint v9 is now quite the norm and fully support ESM. I can make a PR to either reduce the dependency or switch to ESM if you think there is a chance it lands
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.
@ArnaudBarre yes, the eslint plugin now bundles the compiler as well since it can be used standalone. That means a few extra dependencies are needed. Is that blocking anything?
As for ESM I think that will require a larger discussion with the rest of the React team.
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.
The react compiler only requires
@babel/types
as a dependency so I'm still unsure what are the need for all these extra dependencies. It adds extra install size (zod 3 & hermes-parser probably not be present for a number of apps) and extra attack surface for package hijacking like what happen yesterday.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.
@ArnaudBarre we inline most dependencies in babel-plugin-react-compiler which is why it only appears to depend on @babel/types (this is mostly a consequence of some Meta-internal infra quirks). For eslint-plugin-react-hooks we didn't, but I can inline as well if it's a hard requirement to get this shipped. Effectively the eslint plugin now includes the entire compiler as it needs to be able to be used standalone, hence the zod (validating compiler configs) and hermes-parser (parsing both Flow and JavaScript/TypeScript into a Babel compatible AST) dependencies.
Uh oh!
There was an error while loading. Please reload this page.
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 don't think it's needed to depend on it, it's not directly used: https://github.com/search?q=repo%3Afacebook%2Freact%20path%3A%2F%5Epackages%5C%2Feslint-plugin-react-hooks%5C%2F%2F%20zod&type=code
Instead I think the eslint plugin should make
babel-plugin-react-compiler
a peer-dependency? (maybe optional if it's not required for some rules and lazy loaded for others)Edit: Ok I see that the plugin rebundle the compiler, why not depend on it (to avoid having it twice). It would also make the runtime & lint rules in sync for users?
Also the current published version as two 2MB files, is it possible to only publish the development version?
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.
@poteto is there somewhere we could discuss this stuff outside of this PR too?
the modern approach to custom parsers in ESLint is to provide a language implementation, like how
@eslint/json
works. the hooks plugin shouldn't really need to care about parsing at all, since a language should've already been setup to parse the various formats correctly (in this case, flow and a custom JS parse with the compiler plugin).this is a fairly modern part of ESLint though so makes sense the hooks plugin doesn't do it this way yet. so maybe we should track it in an issue somewhere?