-
-
Notifications
You must be signed in to change notification settings - Fork 164
feat: merge plugin-react-oxc
into plugin-react
#609
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
I like the idea! The question is how easy should we make to use the react compiler? If most users will use it, maybe we could also add it as a dependency so users only need a single line change to use it. Or we could even enable it by default and add a boolean to opt out. I still want to do some tests on real world code (not triangle benchmark were the code path is really simple) to better weight the performance impact of the compiler on build time Given that the default template already include eslint, I'm not sure the argument of extra node modules weights a lot even I would prefer less packages. Note: I think we should update the oxc recommendation in the swc plugin |
ca17542
to
044f872
Compare
I think it shouldn't be the default at least until react-compiler reaches stable (currently it's RC).
FWIW I saw some real world numbers at reactwg/react-compiler#33, reactwg/react-compiler#52.
Done 👍 |
test.describe('dev-production', () => { | ||
test.skip('rolldownVersion' in vite) | ||
|
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 have test cases for NODE_ENV=production vite dev
and NODE_ENV=development vite build
#606, but these are failing on oxc since jsx runtime transform respects command
instead of NODE_ENV
.
For now I'll skip this case on rolldown since the behavior is same as previous plugin-react-oxc
and also I'm not sure about this use case in the first place.
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 think it is a bug in plugin-react-oxc
. It should skip setting the development
option as then it would default to isProduction
.
I'll fix this in a separate 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.
Created an issue #622
This are runtime benefits. I tested on my app bundled with esbuild, adding babel with compiler on all tsx files make the bundling time go from 700ms to 13s. So for now I don't think it's good to enable it by default given the non negligeable build time cost. |
I just noticed that we should keep |
Description
Built on top of #306The initial plan was to make
@vitejs/plugin-react-oxc
the default plugin when rolldown-vite lands. But I changed my mind and thought that mergingplugin-react-oxc
intoplugin-react
is better.The upsides are:
@vitejs/plugin-react-babel
. With this change, they can use@vitejs/plugin-react
.rolldown-vite
: In the original plan, early users have to use@vitejs/plugin-react-oxc
to try the behavior. With this change, they can simply switch torolldown-vite
.@vitejs/plugin-react-babel
The downsides are:
@vitejs/plugin-react-oxc
. I think this is acceptable.@babel/core
+ some plugins would be downloaded even if the user doesn't need it: babel requires 11.3MB of deps. Maybe we should make@babel/core
an optional peer dep? (when rolldown-vite stabilizes) That would cut down 11.1MB.