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: restore dynamic-import-polyfill #3434

Merged
merged 5 commits into from
May 17, 2021
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions docs/config/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -480,6 +480,23 @@ export default async ({ command, mode }) => {

Note the build will fail if the code contains features that cannot be safely transpiled by esbuild. See [esbuild docs](https://esbuild.github.io/content-types/#javascript) for more details.

### build.polyfillDynamicImport

- **Type:** `boolean`
- **Default:** `false`

Whether to automatically inject [dynamic import polyfill](https://github.com/GoogleChromeLabs/dynamic-import-polyfill).

If set to true, the polyfill is auto injected into the proxy module of each `index.html` entry. If the build is configured to use a non-html custom entry via `build.rollupOptions.input`, then it is necessary to manually import the polyfill in your custom entry:

```js
import 'vite/dynamic-import-polyfill'
```

When using [`@vitejs/plugin-legacy`](https://github.com/vitejs/vite/tree/main/packages/plugin-legacy), the plugin sets this option to `true` automatically.

Note: the polyfill does **not** apply to [Library Mode](/guide/build#library-mode). If you need to support browsers without native dynamic import, you should probably avoid using it in your library.

### build.outDir

- **Type:** `string`
Expand Down
7 changes: 7 additions & 0 deletions docs/guide/backend-integration.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,13 @@ Or you can follow these steps to configure it manually:
}
```

If you use [`@vitejs/plugin-legacy`](https://github.com/vitejs/vite/tree/main/packages/plugin-legacy) or manually enable the [`build.dynamicImportPolyfill` option](/config/#build-polyfilldynamicimport), remember to add the [dynamic import polyfill](/config/#build-polyfilldynamicimport) to your entry, since it will no longer be auto-injected:

```js
// add the beginning of your app entry
import 'vite/dynamic-import-polyfill'
```

2. For development, inject the following in your server's HTML template (substitute `http://localhost:3000` with the local URL Vite is running at):

```html
Expand Down
21 changes: 20 additions & 1 deletion packages/plugin-legacy/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,20 @@ function viteLegacyPlugin(options = {}) {
})
}

/**
* @type {import('vite').Plugin}
*/
const legacyConfigPlugin = {
name: 'legacy-config',
patak-dev marked this conversation as resolved.
Show resolved Hide resolved

config(config) {
patak-dev marked this conversation as resolved.
Show resolved Hide resolved
if (!config.build) {
config.build = {}
}
config.build.polyfillDynamicImport = true
}
}

/**
* @type {import('vite').Plugin}
*/
Expand Down Expand Up @@ -398,7 +412,12 @@ function viteLegacyPlugin(options = {}) {
}
}

return [legacyGenerateBundlePlugin, legacyPostPlugin, legacyEnvPlugin]
return [
legacyConfigPlugin,
legacyGenerateBundlePlugin,
legacyPostPlugin,
legacyEnvPlugin
]
}

/**
Expand Down
7 changes: 3 additions & 4 deletions packages/vite/src/node/build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export interface BuildOptions {
/**
* whether to inject dynamic import polyfill.
* Note: does not apply to library mode.
* @deprecated the dynamic import polyfill has been removed
* @default false
*/
polyfillDynamicImport?: boolean
/**
Expand Down Expand Up @@ -194,13 +194,12 @@ export interface LibraryOptions {

export type LibraryFormats = 'es' | 'cjs' | 'umd' | 'iife'

export type ResolvedBuildOptions = Required<
Omit<BuildOptions, 'base' | 'polyfillDynamicImport'>
>
export type ResolvedBuildOptions = Required<Omit<BuildOptions, 'base'>>

export function resolveBuildOptions(raw?: BuildOptions): ResolvedBuildOptions {
const resolved: ResolvedBuildOptions = {
target: 'modules',
polyfillDynamicImport: false,
outDir: 'dist',
assetsDir: 'assets',
assetsInlineLimit: 4096,
Expand Down
19 changes: 0 additions & 19 deletions packages/vite/src/node/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -470,25 +470,6 @@ export async function resolveConfig(
}
})

if (config.build?.polyfillDynamicImport) {
logDeprecationWarning(
'build.polyfillDynamicImport',
'"polyfillDynamicImport" has been removed. Please use @vitejs/plugin-legacy if your target browsers do not support dynamic imports.'
)
}

Object.defineProperty(resolvedBuildOptions, 'polyfillDynamicImport', {
enumerable: false,
get() {
logDeprecationWarning(
'build.polyfillDynamicImport',
'"polyfillDynamicImport" has been removed. Please use @vitejs/plugin-legacy if your target browsers do not support dynamic imports.',
new Error()
)
return false
}
})

if (config.alias) {
logDeprecationWarning('alias', 'Use "resolve.alias" instead.')
}
Expand Down
130 changes: 124 additions & 6 deletions packages/vite/src/node/plugins/dynamicImportPolyfill.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,27 @@
import { ResolvedConfig } from '..'
import { Plugin } from '../plugin'
import { isModernFlag } from './importAnalysisBuild'
import path from 'path'

export const polyfillId = 'vite/dynamic-import-polyfill'

/**
* @deprecated
*/
function resolveModulePath(config: ResolvedConfig) {
const {
base,
build: { assetsDir }
} = config
// #2918 path.posix.join returns a wrong path when config.base is a URL
if (/^(https?:)?(\/\/)/i.test(base)) {
return `${base.replace(/\/$/, '')}/${assetsDir}/`
}
return path.posix.join(base, assetsDir, '/')
}

export function dynamicImportPolyfillPlugin(config: ResolvedConfig): Plugin {
const enabled = config.build.polyfillDynamicImport
const skip = !enabled || config.command === 'serve' || config.build.ssr
let polyfillString: string | undefined

return {
name: 'vite:dynamic-import-polyfill',
resolveId(id) {
Expand All @@ -16,11 +31,114 @@ export function dynamicImportPolyfillPlugin(config: ResolvedConfig): Plugin {
},
load(id) {
if (id === polyfillId) {
config.logger.warn(
`\n'vite/dynamic-import-polyfill' is no longer needed, refer to https://github.com/vitejs/vite/blob/main/packages/vite/CHANGELOG.md#230-2021-05-10`
if (!enabled) {
config.logger.warnOnce(
`\n'vite/dynamic-import-polyfill' is no longer needed if you target modern browsers`
)
}
if (skip) {
return ''
}
// return a placeholder here and defer the injection to renderChunk
// so that we can selectively skip the injection based on output format
if (!polyfillString) {
polyfillString =
`const p = ${polyfill.toString()};` +
`${isModernFlag}&&p(${JSON.stringify(resolveModulePath(config))});`
}
return polyfillString
}
},

renderDynamicImport({ format }) {
if (skip || format !== 'es') {
return null
}
if (!polyfillString) {
throw new Error(
`Vite's dynamic import polyfill is enabled but was never imported. This ` +
`should only happen when using custom non-html rollup inputs. Make ` +
`sure to add \`import "${polyfillId}"\` as the first statement in ` +
`your custom entry.`
)
return ''
}
// we do not actually return anything here because rewriting here would
// make it impossible to use es-module-lexer on the rendered chunks, which
// we need for import graph optimization in ./importAnalysisBuild.
}
}
}

/**
The following polyfill function is meant to run in the browser and adapted from
https://github.com/GoogleChromeLabs/dynamic-import-polyfill
MIT License
Copyright (c) 2018 uupaa and 2019 Google LLC
Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
in the Software without restriction, including without limitation the rights
to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
copies of the Software, and to permit persons to whom the Software is
furnished to do so, subject to the following conditions:
The above copyright notice and this permission notice shall be included in all
copies or substantial portions of the Software.
THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
*/

declare const self: any
declare const location: any
declare const document: any
declare const URL: any
declare const Blob: any

function polyfill(modulePath = '.', importFunctionName = '__import__') {
try {
self[importFunctionName] = new Function('u', `return import(u)`)
} catch (error) {
const baseURL = new URL(modulePath, location)
const cleanup = (script: any) => {
Copy link
Member

Choose a reason for hiding this comment

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

question: Is script an HTMLScriptElement?

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 was the exact code from the polyfill from google that was removed, so I would prefer to avoid touching it. If we want to improve it, we can do so in another PR.

Copy link
Member

Choose a reason for hiding this comment

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

HTMLScriptElement would be just only a compile time code addition
Would not touch runtime code 🤔

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's discuss it in another PR 👍🏼

URL.revokeObjectURL(script.src)
script.remove()
}

self[importFunctionName] = (url: string) =>
new Promise((resolve, reject) => {
const absURL = new URL(url, baseURL)

// If the module has already been imported, resolve immediately.
if (self[importFunctionName].moduleMap[absURL]) {
return resolve(self[importFunctionName].moduleMap[absURL])
}

const moduleBlob = new Blob(
[
`import * as m from '${absURL}';`,
`${importFunctionName}.moduleMap['${absURL}']=m;`
],
{ type: 'text/javascript' }
)

const script = Object.assign(document.createElement('script'), {
type: 'module',
src: URL.createObjectURL(moduleBlob),
onerror() {
reject(new Error(`Failed to import: ${url}`))
cleanup(script)
},
onload() {
resolve(self[importFunctionName].moduleMap[absURL])
cleanup(script)
}
})

document.head.appendChild(script)
})

self[importFunctionName].moduleMap = {}
}
}
7 changes: 7 additions & 0 deletions packages/vite/src/node/plugins/html.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {
getAssetFilename
} from './asset'
import { isCSSRequest, chunkToEmittedCssFileMap } from './css'
import { polyfillId } from './dynamicImportPolyfill'
import {
AttributeNode,
NodeTransform,
Expand Down Expand Up @@ -262,6 +263,12 @@ export function buildHtmlPlugin(config: ResolvedConfig): Plugin {
}

processedHtml.set(id, s.toString())

// inject dynamic import polyfill
if (config.build.polyfillDynamicImport) {
js = `import "${polyfillId}";\n${js}`
}

return js
}
},
Expand Down
8 changes: 7 additions & 1 deletion packages/vite/src/node/plugins/importAnalysisBuild.ts
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,7 @@ export function buildImportAnalysisPlugin(config: ResolvedConfig): Plugin {
return
}

const isPolyfillEnabled = config.build.polyfillDynamicImport
for (const file in bundle) {
const chunk = bundle[file]
// can't use chunk.dynamicImports.length here since some modules e.g.
Expand All @@ -220,7 +221,12 @@ export function buildImportAnalysisPlugin(config: ResolvedConfig): Plugin {
if (imports.length) {
const s = new MagicString(code)
for (let index = 0; index < imports.length; index++) {
const { s: start, e: end } = imports[index]
const { s: start, e: end, d: dynamicIndex } = imports[index]
// if dynamic import polyfill is used, rewrite the import to
// use the polyfilled function.
if (isPolyfillEnabled) {
s.overwrite(dynamicIndex, dynamicIndex + 6, `__import__`)
Copy link
Member

Choose a reason for hiding this comment

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

question: What is 6? It's a magic number in this context 🤔
Should we extract it into a variable/constant?

Copy link
Member Author

Choose a reason for hiding this comment

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

6 is the number of letters in import, this is the same code that was removed before so it is ok to add it back as it was.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, so 'import'.length would theoretically better here, but yeah, feel free to add it in a new PR or we could use non-squash-merge specially for this PR and use two commits 🤔
For readability and understandment, I would much prefer to use 'import'.length here

Copy link
Member Author

@patak-dev patak-dev May 15, 2021

Choose a reason for hiding this comment

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

I think the context is enough here, if we add something in another PR maybe it could be a hint in the comment above like 'import'.length === 6. I think this kind of patterns is not uncommon in the Vite code base though

Copy link
Member

Choose a reason for hiding this comment

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

Honestly I don't think it's a magic number, as it's quite clear to see it's the length of import and giving the previous comment saying "rewrite the import". 'import'.length would do but it's runtime, which to me, is not worth to bring the extra memory and CPU cost for the tiny DX that is not commonly touched anyway. I think it's ok to leave it as-is

}
// check the chunk being imported
const url = code.slice(start, end)
const deps: Set<string> = new Set()
Expand Down