-
-
Notifications
You must be signed in to change notification settings - Fork 6k
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
fix: enable failing dependencies to be optimised by pre-processing them with esbuild #4275
Conversation
config.logger.warn( | ||
`Unable to parse dependency: ${id}. Trying again with an esbuild transform.` | ||
) | ||
const transformed = await transform(entryContent, config.esbuild || {}) |
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.
Why not use { loader: { '.js': 'jsx' } }
automatically, removing the need for user to configure it?
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 a good question. Even if we hardcode this here, you still do need the following to make it work, because it would otherwise fail at a later stage when optimising deps:
optimizeDeps: {
esbuildOptions: {
loader: {
".js": "jsx",
},
},
},
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.
We could move this statement up:
vite/packages/vite/src/node/optimizer/index.ts
Lines 266 to 267 in 5935055
const { plugins = [], ...esbuildOptions } = | |
config.optimizeDeps?.esbuildOptions ?? {} |
..then mutate the
esbuildOptions
from here:
esbuildOptions.loader ??= {}
esbuildOptions.loader['.js'] ??= 'jsx'
const transformed = await transform(entryContent, esbuildOptions)
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.
Nice idea. I did that, although couldn't directly use esbuildOptions
for the transform
call (since the loader
field type isn't compatible). Let me know what you think!
A git repo that reproduces the issue would be super handy. :) |
You're right, I should have thought about that! Here you go: https://github.com/fwouts/vite-gatsby-sample-app |
I faced the same problem. When I use storybook as a vite bandler, |
I think we should discuss this PR with Evan before merging it, as it introduced special handling for JSX. It would be useful for that discussion to know what is preventing this fallback to be added as a userland plugin (or as a more generic feature that could enable the userland plugin) |
That's a great point. If special handling of JSX isn't desired, perhaps a good alternative would be to enable plugins to somehow be able to override the handling in this part of the code. |
Using I will add this to this week's meeting with Evan. |
Hey @antfu, I was just wondering what was the outcome of the meeting? :) |
The meeting was rescheduled to today |
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.
After this we are good to go and got an approval from Evan at todays meeting
Accidentally pressed the wrong button :D
Description
Some packages such as Gatsby ship with JSX which is expected to be compiled by the library users' bundler.
While there are workarounds to force Vite to interpret
.js
files as JSX (see discussion), these are not sufficient when the JSX is insidenode_modules/
. In particular,es-module-lexer
doesn't support JSX (this was considered infeasible as per guybedford/es-module-lexer#47). This means that theparse()
call at:vite/packages/vite/src/node/optimizer/index.ts
Line 245 in 887c247
While the best approach is certainly to ensure that packages don't ship as JSX, it's possible to improve the situation by adding a fallback mechanism which transforms the code with esbuild prior to re-trying.
This fix enables Gatsby to work when passing Vite the following options:
Since it's only a fallback mechanism, it shouldn't affect performance adversely.
Additional context
This was originally surfaced as a bug report for the Viteshot project, see fwouts/viteshot#53.
Example project: https://github.com/fwouts/vite-gatsby-sample-app
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).