-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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?
Changes from 1 commit
3f34cf6
3f052e2
6e98abf
6eb244b
83bb05f
b8c2f1a
a919be5
409ae84
a1d3562
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,6 +46,9 @@ const dynamicImportPrefixRE = /import\s*\(/ | |
const optimizedDepChunkRE = /\/chunk-[A-Z\d]{8}\.js/ | ||
const optimizedDepDynamicRE = /-[A-Z\d]{8}\.js/ | ||
|
||
const dynamicImportTreeshakenRE = | ||
/(\b(const|let|var)\s+(\{[^}.]+\})\s*=\s*await\s+import\([^)]+\))|(\(\s*await\s+import\([^)]+\)\s*\)(\??\.[^;[\s]+)+)|\bimport\([^)]+\)(\.then\([^{]*\{([^}]+)\})/g | ||
|
||
function toRelativePath(filename: string, importer: string) { | ||
const relPath = path.relative(path.dirname(importer), filename) | ||
return relPath[0] === '.' ? relPath : `./${relPath}` | ||
|
@@ -285,6 +288,39 @@ export function buildImportAnalysisPlugin(config: ResolvedConfig): Plugin { | |
return [url, resolved.id] | ||
} | ||
|
||
const dynamicImports: Record< | ||
number, | ||
{ declaration?: string; names?: string; chains?: string } | ||
> = {} | ||
|
||
if (insertPreload) { | ||
let match | ||
while ((match = dynamicImportTreeshakenRE.exec(source))) { | ||
// handle `const {foo} = await import('foo')` | ||
if (match[1]) { | ||
dynamicImports[dynamicImportTreeshakenRE.lastIndex] = { | ||
declaration: `${match[2]} ${match[3]}`, | ||
names: match[3]?.trim(), | ||
sun0day marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
continue | ||
} | ||
|
||
// handle `(await import('foo')).foo` | ||
if (match[4]) { | ||
dynamicImports[ | ||
dynamicImportTreeshakenRE.lastIndex - match[5]?.length - 1 | ||
] = { chains: match[5] } | ||
sun0day marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that would be better 👍 |
||
continue | ||
} | ||
|
||
// handle `import('foo').then(({foo})=>{})` | ||
const names = match[7]?.trim() | ||
dynamicImports[ | ||
dynamicImportTreeshakenRE.lastIndex - match[6]?.length | ||
] = { declaration: `const {${names}}`, names: `{ ${names} }` } | ||
sun0day marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
let s: MagicString | undefined | ||
const str = () => s || (s = new MagicString(source)) | ||
let needPreloadHelper = false | ||
|
@@ -309,7 +345,24 @@ export function buildImportAnalysisPlugin(config: ResolvedConfig): Plugin { | |
|
||
if (isDynamicImport && insertPreload) { | ||
needPreloadHelper = true | ||
str().prependLeft(expStart, `${preloadMethod}(() => `) | ||
const { declaration, names, chains } = dynamicImports[expEnd] || {} | ||
if (names) { | ||
str().prependLeft( | ||
expStart, | ||
`${preloadMethod}(async () => { ${declaration} = await `, | ||
) | ||
str().appendRight(expEnd, `;return ${names}}`) | ||
} else if (chains) { | ||
const name = chains.match(/\.([^.?]+)/)?.[1] || '' | ||
str().prependLeft( | ||
expStart, | ||
`${preloadMethod}(async () => { const ${name} = (await `, | ||
) | ||
str().appendRight(expEnd, `).${name}; return { ${name} }}`) | ||
} else { | ||
str().prependLeft(expStart, `${preloadMethod}(() => `) | ||
} | ||
|
||
str().appendRight( | ||
expEnd, | ||
`,${isModernFlag}?"${preloadMarker}":void 0${ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
export const foo = () => { | ||
console.log('treeshaken foo') | ||
} | ||
export const bar = () => { | ||
console.log('treeshaken bar') | ||
} | ||
export const baz1 = () => { | ||
console.log('treeshaken baz1') | ||
} | ||
export const baz2 = { | ||
log: () => { | ||
console.log('treeshaken baz2') | ||
}, | ||
} | ||
export const baz3 = { | ||
log: () => { | ||
console.log('treeshaken baz3') | ||
}, | ||
} | ||
export const baz4 = () => { | ||
console.log('treeshaken baz4') | ||
} | ||
export const baz5 = () => { | ||
console.log('treeshaken baz5') | ||
} | ||
export const removed = () => { | ||
console.log('treeshaken removed') | ||
} | ||
export default () => { | ||
console.log('treeshaken 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.
The below case cannot be supported.
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.