Skip to content
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(esbuild): always support dynamic import and import meta #9105

Merged
merged 3 commits into from Jul 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 35 additions & 7 deletions packages/vite/src/node/__tests__/plugins/esbuild.spec.ts
Expand Up @@ -21,7 +21,11 @@ describe('resolveEsbuildTranspileOptions', () => {
format: 'esm',
keepNames: true,
minify: true,
treeShaking: true
treeShaking: true,
supported: {
'dynamic-import': true,
'import-meta': true
}
})
})

Expand Down Expand Up @@ -62,7 +66,11 @@ describe('resolveEsbuildTranspileOptions', () => {
minifyIdentifiers: false,
minifySyntax: true,
minifyWhitespace: true,
treeShaking: true
treeShaking: true,
supported: {
'dynamic-import': true,
'import-meta': true
}
})
})

Expand All @@ -87,7 +95,11 @@ describe('resolveEsbuildTranspileOptions', () => {
minifyIdentifiers: false,
minifySyntax: false,
minifyWhitespace: false,
treeShaking: false
treeShaking: false,
supported: {
'dynamic-import': true,
'import-meta': true
}
})
})

Expand All @@ -114,7 +126,11 @@ describe('resolveEsbuildTranspileOptions', () => {
minifyIdentifiers: true,
minifySyntax: true,
minifyWhitespace: false,
treeShaking: true
treeShaking: true,
supported: {
'dynamic-import': true,
'import-meta': true
}
})
})

Expand All @@ -138,7 +154,11 @@ describe('resolveEsbuildTranspileOptions', () => {
format: 'cjs',
keepNames: true,
minify: true,
treeShaking: true
treeShaking: true,
supported: {
'dynamic-import': true,
'import-meta': true
}
})
})

Expand Down Expand Up @@ -167,7 +187,11 @@ describe('resolveEsbuildTranspileOptions', () => {
minifyIdentifiers: true,
minifySyntax: true,
minifyWhitespace: false,
treeShaking: true
treeShaking: true,
supported: {
'dynamic-import': true,
'import-meta': true
}
})
})

Expand Down Expand Up @@ -197,7 +221,11 @@ describe('resolveEsbuildTranspileOptions', () => {
minifyIdentifiers: true,
minifySyntax: false,
minifyWhitespace: true,
treeShaking: true
treeShaking: true,
supported: {
'dynamic-import': true,
'import-meta': true
}
})
})
})
Expand Down
13 changes: 11 additions & 2 deletions packages/vite/src/node/plugins/esbuild.ts
Expand Up @@ -298,10 +298,19 @@ export function resolveEsbuildTranspileOptions(
// pure annotations and break tree-shaking
// https://github.com/vuejs/core/issues/2860#issuecomment-926882793
const isEsLibBuild = config.build.lib && format === 'es'
const esbuildOptions = config.esbuild || {}
const options: TransformOptions = {
...config.esbuild,
...esbuildOptions,
target: target || undefined,
format: rollupToEsbuildFormatMap[format]
format: rollupToEsbuildFormatMap[format],
// the final build should always support dynamic import and import.meta.
// if they need to be polyfilled, plugin-legacy should be used.
// plugin-legacy detects these two features when checking for modern code.
supported: {
'dynamic-import': true,
'import-meta': true,
...esbuildOptions.supported
}
Comment on lines +309 to +313
Copy link
Member

@poyoho poyoho Jul 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As comment said, if they need to be polyfilled, plugin-legacy should be used.. I think the internal config should had high priority. 🤔

And the minimum node version supported by vite also supports dynamic import. I think we don't need esbuild polyfilled.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part is the same as in the optimizer where the user can set optimizeDeps.esbuildOptions.supported to override too, even if it doesn't make sense. I think it might still be helpful if someone wants to override it for some reason. Though I don't have a hard opinion on this, if we want to change this to be stricter, we should change it for the optimizer too.

}

// If no minify, disable all minify options
Expand Down
11 changes: 11 additions & 0 deletions playground/build-old/__tests__/build-old.spec.ts
@@ -0,0 +1,11 @@
import { describe, test } from 'vitest'
import { page } from '~utils'

describe('syntax preserve', () => {
test('import.meta.url', async () => {
expect(await page.textContent('.import-meta-url')).toBe('string')
})
test('dynamic import', async () => {
expect(await page.textContent('.dynamic-import')).toBe('success')
})
})
1 change: 1 addition & 0 deletions playground/build-old/dynamic.js
@@ -0,0 +1 @@
export default 'success'
19 changes: 19 additions & 0 deletions playground/build-old/index.html
@@ -0,0 +1,19 @@
<h1>Build Old</h1>

<h2>import meta url</h2>
<p class="import-meta-url"></p>

<h2>dynamic import</h2>
<p class="dynamic-import"></p>

<script type="module">
text('.import-meta-url', typeof import.meta.url)

import('./dynamic.js').then((m) => {
text('.dynamic-import', m.default)
})

function text(el, text) {
document.querySelector(el).textContent = text
}
</script>
11 changes: 11 additions & 0 deletions playground/build-old/package.json
@@ -0,0 +1,11 @@
{
"name": "test-build-old",
"private": true,
"version": "0.0.0",
"scripts": {
"dev": "vite",
"build": "vite build",
"debug": "node --inspect-brk ../../packages/vite/bin/vite",
"preview": "vite preview"
}
}
8 changes: 8 additions & 0 deletions playground/build-old/vite.config.js
@@ -0,0 +1,8 @@
import { defineConfig } from 'vite'

export default defineConfig({
build: {
// old browsers only
target: ['chrome60']
}
})