fix(html): externalize rollup.external scripts correctly (#18618)

vitejs · Nov 9, 2024 · 55461b4 · 55461b4
1 parent 2a88f71
commit 55461b4
Show file tree

Hide file tree

Showing 4 changed files with 79 additions and 22 deletions.
diff --git a/packages/vite/src/node/plugins/html.ts b/packages/vite/src/node/plugins/html.ts
@@ -466,17 +466,22 @@ export function buildHtmlPlugin(config: ResolvedConfig): Plugin {
                 inlineModuleIndex++
                 if (url && !isExcludedUrl(url) && !isPublicFile) {
                   setModuleSideEffectPromises.push(
-                    this.resolve(url, id)
-                      .then((resolved) => {
-                        if (!resolved) {
-                          return Promise.reject()
-                        }
-                        return this.load(resolved)
-                      })
-                      .then((mod) => {
-                        // set this to keep the module even if `treeshake.moduleSideEffects=false` is set
-                        mod.moduleSideEffects = true
-                      }),
+                    this.resolve(url, id).then((resolved) => {
+                      if (!resolved) {
+                        return Promise.reject(
+                          new Error(`Failed to resolve ${url} from ${id}`),
+                        )
+                      }
+                      // set moduleSideEffects to keep the module even if `treeshake.moduleSideEffects=false` is set
+                      const moduleInfo = this.getModuleInfo(resolved.id)
+                      if (moduleInfo) {
+                        moduleInfo.moduleSideEffects = true
+                      } else if (!resolved.external) {
+                        return this.load(resolved).then((mod) => {
+                          mod.moduleSideEffects = true
+                        })
+                      }
+                    }),
                   )
                   // <script type="module" src="..."/>
                   // add it as an import
@@ -715,23 +720,28 @@ export function buildHtmlPlugin(config: ResolvedConfig): Plugin {
       const getImportedChunks = (
         chunk: OutputChunk,
         seen: Set<string> = new Set(),
-      ): OutputChunk[] => {
-        const chunks: OutputChunk[] = []
+      ): (OutputChunk | string)[] => {
+        const chunks: (OutputChunk | string)[] = []
         chunk.imports.forEach((file) => {
           const importee = bundle[file]
-          if (importee?.type === 'chunk' && !seen.has(file)) {
-            seen.add(file)
+          if (importee) {
+            if (importee.type === 'chunk' && !seen.has(file)) {
+              seen.add(file)
 
-            // post-order traversal
-            chunks.push(...getImportedChunks(importee, seen))
-            chunks.push(importee)
+              // post-order traversal
+              chunks.push(...getImportedChunks(importee, seen))
+              chunks.push(importee)
+            }
+          } else {
+            // external imports
+            chunks.push(file)
           }
         })
         return chunks
       }
 
       const toScriptTag = (
-        chunk: OutputChunk,
+        chunkOrUrl: OutputChunk | string,
         toOutputPath: (filename: string) => string,
         isAsync: boolean,
       ): HtmlTagDescriptor => ({
@@ -746,7 +756,10 @@ export function buildHtmlPlugin(config: ResolvedConfig): Plugin {
           // https://developer.chrome.com/blog/modulepreload/#ok-so-why-doesnt-link-relpreload-work-for-modules:~:text=For%20%3Cscript%3E,of%20other%20modules.
           // Now `<script type="module">` uses `same origin`: https://github.com/whatwg/html/pull/3656#:~:text=Module%20scripts%20are%20always%20fetched%20with%20credentials%20mode%20%22same%2Dorigin%22%20by%20default%20and%20can%20no%20longer%0Ause%20%22omit%22
           crossorigin: true,
-          src: toOutputPath(chunk.fileName),
+          src:
+            typeof chunkOrUrl === 'string'
+              ? chunkOrUrl
+              : toOutputPath(chunkOrUrl.fileName),
         },
       })
 
@@ -863,7 +876,9 @@ export function buildHtmlPlugin(config: ResolvedConfig): Plugin {
               const resolveDependencies =
                 typeof modulePreload === 'object' &&
                 modulePreload.resolveDependencies
-              const importsFileNames = imports.map((chunk) => chunk.fileName)
+              const importsFileNames = imports
+                .filter((chunkOrUrl) => typeof chunkOrUrl !== 'string')
+                .map((chunk) => chunk.fileName)
               const resolvedDeps = resolveDependencies
                 ? resolveDependencies(chunk.fileName, importsFileNames, {
                     hostId: relativeUrlPath,

diff --git a/playground/html/__tests__/html.spec.ts b/playground/html/__tests__/html.spec.ts
@@ -117,12 +117,24 @@ describe('main', () => {
       expect(serverLogs).not.toEqual(
         expect.arrayContaining([
           expect.stringMatching(
-            'can\'t be bundled without type="module" attribute',
+            /"\/external-path\.js".*can't be bundled without type="module" attribute/,
           ),
         ]),
       )
     }
   })
+
+  test.runIf(isBuild)(
+    'external paths by rollupOptions.external works',
+    async () => {
+      expect(await page.textContent('.external-path-by-rollup-options')).toBe(
+        'works',
+      )
+      expect(serverLogs).not.toEqual(
+        expect.arrayContaining([expect.stringContaining('Could not load')]),
+      )
+    },
+  )
 })
 
 describe('nested', () => {

diff --git a/playground/html/index.html b/playground/html/index.html
@@ -13,3 +13,7 @@ <h1>Hello</h1>
 <div>External path: <span class="external-path"></span></div>
 <script type="module" src="/external-path.js" vite-ignore></script>
 <link rel="stylesheet" href="/external-path.css" vite-ignore />
+<div>
+  External path by rollupOptions.external (build only):
+  <span class="external-path-by-rollup-options"></span>
+</div>
diff --git a/playground/html/vite.config.js b/playground/html/vite.config.js
@@ -40,6 +40,7 @@ export default defineConfig({
           resolve(__dirname, 'relative-input.html'),
         ),
       },
+      external: ['/external-path-by-rollup-options.js'],
     },
   },
 
@@ -228,6 +229,26 @@ ${
         },
       },
     },
+    {
+      name: 'append-external-path-by-rollup-options',
+      apply: 'build', // this does not work in serve
+      transformIndexHtml: {
+        order: 'pre',
+        handler(_, ctx) {
+          if (!ctx.filename.endsWith('html/index.html')) return
+          return [
+            {
+              tag: 'script',
+              attrs: {
+                type: 'module',
+                src: '/external-path-by-rollup-options.js',
+              },
+              injectTo: 'body',
+            },
+          ]
+        },
+      },
+    },
     serveExternalPathPlugin(),
   ],
 })
@@ -241,6 +262,11 @@ function serveExternalPathPlugin() {
     } else if (req.url === '/external-path.css') {
       res.setHeader('Content-Type', 'text/css')
       res.end('.external-path{color:red}')
+    } else if (req.url === '/external-path-by-rollup-options.js') {
+      res.setHeader('Content-Type', 'application/javascript')
+      res.end(
+        'document.querySelector(".external-path-by-rollup-options").textContent = "works"',
+      )
     } else {
       next()
     }