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-import-analysis): treeshaken dynamic import when injecting preload #14221
base: main
Are you sure you want to change the base?
Conversation
Run & review this pull request in StackBlitz Codeflow. |
const dynamicImportTreeshakenRE = | ||
/(\b(const|let|var)\s+(\{[^}.]+\})\s*=\s*await\s+import\([^)]+\))|(\(\s*await\s+import\([^)]+\)\s*\)(\??\.[^;[\s]+)+)|\bimport\([^)]+\)(\.then\([^{]*\{([^}]+)\})/g |
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 below case cannot be supported.
import('./deps.js')
.then({ a } => {})
You need to add \s*
before .then
IMO, using regexp to handle this might break many edge-case
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.
Yeah, regexp can hardly cover all the edge-cases, but it seems there's no better way to support dynamic import treeshaking within current logic. Parsing the ast can completely handle this issue but it's too expensive.
IMO, we could support the common usages of dynamic import for now, and then patch the related issues in the future.
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.
if (match[4]) { | ||
dynamicImports[ | ||
dynamicImportTreeshakenRE.lastIndex - match[5]?.length - 1 | ||
] = { chains: match[5] } |
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.
Does it make sense to make this:
const names = match[5].match(/\.([^.?]+)/)?.[1] || ''
... = { declarations: `const {${names}}`, names: : `{ ${names} }` }
The regex is borrowed from line 386, which maybe could be simplified and extracted.
This way all cases are consistently typed?
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.
Yeah, that would be better 👍
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
).