-
-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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(build): allow dynamic import treeshaking when injecting preload #14221
Conversation
Run & review this pull request in StackBlitz Codeflow. |
IIUC this PR transforms dynamic imports with But I think the regex and implementation is fairly impressive. I'd lean to a more robust alternative still, but it's also harder. Ideally if |
Yeah, that's what this pr mainly does. The regexp contains three sub-regexps related to three dynamic import formats, e.g:
After injecting
|
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'm open to trying this approach in the beta to see if it works, but I'll see what the others think about it too.
/ecosystem-ci run |
📝 Ran ecosystem CI on
✅ analogjs, astro, ladle, laravel, marko, previewjs, quasar, qwik, rakkas, remix, unocss, vike, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress |
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.
This is a very smart trick 👀
Probably too magical but I can't think of a better way to do this at this point. Maybe we should rethink the way preload works in general to do a simpler scheme. Let's add it to the beta so we can test before the release.
* to `__vitePreload(async () => { const {foo} = await import('foo');return { foo }},...).then(({foo})=>{})` | ||
* | ||
* transform `(await import('foo')).foo` | ||
* to `__vitePreload(async () => { const {foo} = (await import('foo')).foo; return { foo }},...)).foo` |
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.
Upgrading to vite 5.3.0-beta.1 has broken a storybook build, and I wonder if it's because of this PR. Is it possible that this can cause an invalid shorthand object? In my case, I have let axe=(await import('axe-core')).default
, and I'm getting an error during build: RollupError: Cannot use a reserved word as a shorthand property
Is it possible that this is being transformed to {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.
I think so! We may need to handle this like on line 278. Seems like we need more tests that covers 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.
@IanVS FWIW, hand editing the storybook code to let {default: x} = (await import('axe-core'))
fixes the problem, so your theory seems confirmed.
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.
FYI I've released a workaround in Storybook https://github.com/storybookjs/storybook/releases/tag/v8.1.9
Description
fix #14145
refs rollup/rollup#4952
After this PR, vite can treeshaken following dynamic imports when injecting
__vitePreload
:Additional context
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).