From 21d314acc748ff92706069756ac7512e3120ec54 Mon Sep 17 00:00:00 2001
From: Hiroshi Ogawa <hi.ogawa.zz@gmail.com>
Date: Fri, 20 Sep 2024 15:40:40 +0900
Subject: [PATCH 1/3] fix(optimizer): don't externalize transitive dep package
 name with asset extension

---
 .../src/node/optimizer/esbuildDepPlugin.ts    | 16 +++++++-----
 .../__tests__/optimize-deps.spec.ts           |  6 +++++
 .../dep-with-asset-ext/dep1/index.js          |  1 +
 .../dep-with-asset-ext/dep1/package.json      |  6 +++++
 .../dep-with-asset-ext/dep2/index.js          |  1 +
 .../dep-with-asset-ext/dep2/package.json      |  9 +++++++
 playground/optimize-deps/index.html           |  8 ++++++
 playground/optimize-deps/package.json         |  2 ++
 pnpm-lock.yaml                                | 26 +++++++++++++++++++
 9 files changed, 68 insertions(+), 7 deletions(-)
 create mode 100644 playground/optimize-deps/dep-with-asset-ext/dep1/index.js
 create mode 100644 playground/optimize-deps/dep-with-asset-ext/dep1/package.json
 create mode 100644 playground/optimize-deps/dep-with-asset-ext/dep2/index.js
 create mode 100644 playground/optimize-deps/dep-with-asset-ext/dep2/package.json

diff --git a/packages/vite/src/node/optimizer/esbuildDepPlugin.ts b/packages/vite/src/node/optimizer/esbuildDepPlugin.ts
index 4fc5a1d53b1560..1bcd52d3100839 100644
--- a/packages/vite/src/node/optimizer/esbuildDepPlugin.ts
+++ b/packages/vite/src/node/optimizer/esbuildDepPlugin.ts
@@ -157,15 +157,17 @@ export function esbuildDepPlugin(
 
           const resolved = await resolve(id, importer, kind)
           if (resolved) {
-            if (kind === 'require-call') {
-              // #16116 fix: Import the module.scss path, which is actually module.scss.js
-              if (resolved.endsWith('.js')) {
-                return {
-                  path: resolved,
-                  external: false,
-                }
+            // `resolved` can be javascript even when `id` matches `allExternalTypes`
+            // due to cjs resolution (e.g. require("./test.pdf") for "./test.pdf.js")
+            // or package name (e.g. import "some-package.pdf")
+            if (resolved.endsWith('.js')) {
+              return {
+                path: resolved,
+                external: false,
               }
+            }
 
+            if (kind === 'require-call') {
               // here it is not set to `external: true` to convert `require` to `import`
               return {
                 path: resolved,
diff --git a/playground/optimize-deps/__tests__/optimize-deps.spec.ts b/playground/optimize-deps/__tests__/optimize-deps.spec.ts
index 26bf6f9b2d9b48..218cc2b153f388 100644
--- a/playground/optimize-deps/__tests__/optimize-deps.spec.ts
+++ b/playground/optimize-deps/__tests__/optimize-deps.spec.ts
@@ -343,3 +343,9 @@ test('import the CommonJS external package that omits the js suffix', async () =
     page.textContent('.external-package-tsx-js'),
   ).toBe('tsx')
 })
+
+test('import the CommonJS external package that omits the js suffix', async () => {
+  await expectWithRetry(() => page.textContent('.dep-with-asset-ext')).toBe(
+    'good',
+  )
+})
diff --git a/playground/optimize-deps/dep-with-asset-ext/dep1/index.js b/playground/optimize-deps/dep-with-asset-ext/dep1/index.js
new file mode 100644
index 00000000000000..dd70b4fb0f17a1
--- /dev/null
+++ b/playground/optimize-deps/dep-with-asset-ext/dep1/index.js
@@ -0,0 +1 @@
+export default { random: Math.random() }
diff --git a/playground/optimize-deps/dep-with-asset-ext/dep1/package.json b/playground/optimize-deps/dep-with-asset-ext/dep1/package.json
new file mode 100644
index 00000000000000..620170d8f66566
--- /dev/null
+++ b/playground/optimize-deps/dep-with-asset-ext/dep1/package.json
@@ -0,0 +1,6 @@
+{
+  "name": "@vitejs/test-dep-with-asset-ext1.pdf",
+  "private": true,
+  "type": "module",
+  "exports": "./index.js"
+}
diff --git a/playground/optimize-deps/dep-with-asset-ext/dep2/index.js b/playground/optimize-deps/dep-with-asset-ext/dep2/index.js
new file mode 100644
index 00000000000000..51eb63870348db
--- /dev/null
+++ b/playground/optimize-deps/dep-with-asset-ext/dep2/index.js
@@ -0,0 +1 @@
+export { default } from '@vitejs/test-dep-with-asset-ext1.pdf'
diff --git a/playground/optimize-deps/dep-with-asset-ext/dep2/package.json b/playground/optimize-deps/dep-with-asset-ext/dep2/package.json
new file mode 100644
index 00000000000000..2134bc6de3eabb
--- /dev/null
+++ b/playground/optimize-deps/dep-with-asset-ext/dep2/package.json
@@ -0,0 +1,9 @@
+{
+  "name": "@vitejs/test-dep-with-asset-ext2.pdf",
+  "private": true,
+  "type": "module",
+  "exports": "./index.js",
+  "dependencies": {
+    "@vitejs/test-dep-with-asset-ext1.pdf": "file:../dep1"
+  }
+}
diff --git a/playground/optimize-deps/index.html b/playground/optimize-deps/index.html
index b98a3174497ef6..200aac420f87a8 100644
--- a/playground/optimize-deps/index.html
+++ b/playground/optimize-deps/index.html
@@ -287,3 +287,11 @@ <h2>Import the CommonJS external package that omits the js suffix</h2>
   import loadSub from '@vitejs/test-dep-incompatible'
   loadSub() // should show an error that tells there's an incompatible dep
 </script>
+
+<h2>Pre-bundle transitive dependency 'some-package.pdf'</h2>
+<div class="dep-with-asset-ext">???</div>
+<script type="module">
+  import original from '@vitejs/test-dep-with-asset-ext1.pdf'
+  import reexport from '@vitejs/test-dep-with-asset-ext2.pdf'
+  text('.dep-with-asset-ext', original === reexport ? 'good' : 'bad')
+</script>
diff --git a/playground/optimize-deps/package.json b/playground/optimize-deps/package.json
index a2c95b53217d21..6c54ac7ec90a88 100644
--- a/playground/optimize-deps/package.json
+++ b/playground/optimize-deps/package.json
@@ -29,6 +29,8 @@
     "@vitejs/test-dep-optimize-exports-with-root-glob": "file:./dep-optimize-exports-with-root-glob",
     "@vitejs/test-dep-optimize-with-glob": "file:./dep-optimize-with-glob",
     "@vitejs/test-dep-relative-to-main": "file:./dep-relative-to-main",
+    "@vitejs/test-dep-with-asset-ext1.pdf": "file:./dep-with-asset-ext/dep1",
+    "@vitejs/test-dep-with-asset-ext2.pdf": "file:./dep-with-asset-ext/dep2",
     "@vitejs/test-dep-with-builtin-module-cjs": "file:./dep-with-builtin-module-cjs",
     "@vitejs/test-dep-with-builtin-module-esm": "file:./dep-with-builtin-module-esm",
     "@vitejs/test-dep-with-dynamic-import": "file:./dep-with-dynamic-import",
diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml
index 8f45890b982c35..4d07bf1ab404f6 100644
--- a/pnpm-lock.yaml
+++ b/pnpm-lock.yaml
@@ -950,6 +950,12 @@ importers:
       '@vitejs/test-dep-relative-to-main':
         specifier: file:./dep-relative-to-main
         version: file:playground/optimize-deps/dep-relative-to-main
+      '@vitejs/test-dep-with-asset-ext1.pdf':
+        specifier: file:./dep-with-asset-ext/dep1
+        version: file:playground/optimize-deps/dep-with-asset-ext/dep1
+      '@vitejs/test-dep-with-asset-ext2.pdf':
+        specifier: file:./dep-with-asset-ext/dep2
+        version: file:playground/optimize-deps/dep-with-asset-ext/dep2
       '@vitejs/test-dep-with-builtin-module-cjs':
         specifier: file:./dep-with-builtin-module-cjs
         version: file:playground/optimize-deps/dep-with-builtin-module-cjs
@@ -1069,6 +1075,14 @@ importers:
 
   playground/optimize-deps/dep-relative-to-main: {}
 
+  playground/optimize-deps/dep-with-asset-ext/dep1: {}
+
+  playground/optimize-deps/dep-with-asset-ext/dep2:
+    dependencies:
+      '@vitejs/test-dep-with-asset-ext1.pdf':
+        specifier: file:../dep1
+        version: file:playground/optimize-deps/dep-with-asset-ext/dep1
+
   playground/optimize-deps/dep-with-builtin-module-cjs: {}
 
   playground/optimize-deps/dep-with-builtin-module-esm: {}
@@ -3572,6 +3586,12 @@ packages:
   '@vitejs/test-dep-to-optimize@file:playground/worker/dep-to-optimize':
     resolution: {directory: playground/worker/dep-to-optimize, type: directory}
 
+  '@vitejs/test-dep-with-asset-ext1.pdf@file:playground/optimize-deps/dep-with-asset-ext/dep1':
+    resolution: {directory: playground/optimize-deps/dep-with-asset-ext/dep1, type: directory}
+
+  '@vitejs/test-dep-with-asset-ext2.pdf@file:playground/optimize-deps/dep-with-asset-ext/dep2':
+    resolution: {directory: playground/optimize-deps/dep-with-asset-ext/dep2, type: directory}
+
   '@vitejs/test-dep-with-builtin-module-cjs@file:playground/optimize-deps/dep-with-builtin-module-cjs':
     resolution: {directory: playground/optimize-deps/dep-with-builtin-module-cjs, type: directory}
 
@@ -9137,6 +9157,12 @@ snapshots:
 
   '@vitejs/test-dep-to-optimize@file:playground/worker/dep-to-optimize': {}
 
+  '@vitejs/test-dep-with-asset-ext1.pdf@file:playground/optimize-deps/dep-with-asset-ext/dep1': {}
+
+  '@vitejs/test-dep-with-asset-ext2.pdf@file:playground/optimize-deps/dep-with-asset-ext/dep2':
+    dependencies:
+      '@vitejs/test-dep-with-asset-ext1.pdf': file:playground/optimize-deps/dep-with-asset-ext/dep1
+
   '@vitejs/test-dep-with-builtin-module-cjs@file:playground/optimize-deps/dep-with-builtin-module-cjs': {}
 
   '@vitejs/test-dep-with-builtin-module-esm@file:playground/optimize-deps/dep-with-builtin-module-esm': {}

From b321b87b699750aa5f15cdb827b91985b91cb289 Mon Sep 17 00:00:00 2001
From: Hiroshi Ogawa <hi.ogawa.zz@gmail.com>
Date: Fri, 20 Sep 2024 16:09:42 +0900
Subject: [PATCH 2/3] test: tweak

---
 .../optimize-deps/__tests__/optimize-deps.spec.ts     | 11 +++++++----
 .../optimize-deps/dep-with-asset-ext/dep1/index.js    |  2 ++
 playground/optimize-deps/index.html                   | 10 +++++++---
 3 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/playground/optimize-deps/__tests__/optimize-deps.spec.ts b/playground/optimize-deps/__tests__/optimize-deps.spec.ts
index 218cc2b153f388..e1226ad0556ba2 100644
--- a/playground/optimize-deps/__tests__/optimize-deps.spec.ts
+++ b/playground/optimize-deps/__tests__/optimize-deps.spec.ts
@@ -344,8 +344,11 @@ test('import the CommonJS external package that omits the js suffix', async () =
   ).toBe('tsx')
 })
 
-test('import the CommonJS external package that omits the js suffix', async () => {
-  await expectWithRetry(() => page.textContent('.dep-with-asset-ext')).toBe(
-    'good',
-  )
+test('external package name with asset extension', async () => {
+  await expectWithRetry(() =>
+    page.textContent('.dep-with-asset-ext-no-dual-package'),
+  ).toBe('true')
+  await expectWithRetry(() =>
+    page.textContent('.dep-with-asset-ext-prebundled'),
+  ).toBe(String(isServe))
 })
diff --git a/playground/optimize-deps/dep-with-asset-ext/dep1/index.js b/playground/optimize-deps/dep-with-asset-ext/dep1/index.js
index dd70b4fb0f17a1..0c18cd78e5a5ac 100644
--- a/playground/optimize-deps/dep-with-asset-ext/dep1/index.js
+++ b/playground/optimize-deps/dep-with-asset-ext/dep1/index.js
@@ -1 +1,3 @@
 export default { random: Math.random() }
+
+export const isPreBundled = import.meta.url.includes('/.vite/deps/')
diff --git a/playground/optimize-deps/index.html b/playground/optimize-deps/index.html
index 200aac420f87a8..664e1d6673770d 100644
--- a/playground/optimize-deps/index.html
+++ b/playground/optimize-deps/index.html
@@ -289,9 +289,13 @@ <h2>Import the CommonJS external package that omits the js suffix</h2>
 </script>
 
 <h2>Pre-bundle transitive dependency 'some-package.pdf'</h2>
-<div class="dep-with-asset-ext">???</div>
+<div>prebundled: <span class="dep-with-asset-ext-prebundled">???</span></div>
+<div>
+  no dual package: <span class="dep-with-asset-ext-no-dual-package">???</span>
+</div>
 <script type="module">
-  import original from '@vitejs/test-dep-with-asset-ext1.pdf'
+  import original, { isPreBundled } from '@vitejs/test-dep-with-asset-ext1.pdf'
   import reexport from '@vitejs/test-dep-with-asset-ext2.pdf'
-  text('.dep-with-asset-ext', original === reexport ? 'good' : 'bad')
+  text('.dep-with-asset-ext-prebundled', isPreBundled)
+  text('.dep-with-asset-ext-no-dual-package', original === reexport)
 </script>

From 10aef190835d6d5e268badd02dd0d1c278163ad1 Mon Sep 17 00:00:00 2001
From: Hiroshi Ogawa <hi.ogawa.zz@gmail.com>
Date: Fri, 20 Sep 2024 16:30:40 +0900
Subject: [PATCH 3/3] fix: handle other js types

---
 packages/vite/src/node/optimizer/esbuildDepPlugin.ts          | 4 ++--
 .../dep-with-asset-ext/dep1/{index.js => index.mjs}           | 0
 playground/optimize-deps/dep-with-asset-ext/dep1/package.json | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)
 rename playground/optimize-deps/dep-with-asset-ext/dep1/{index.js => index.mjs} (100%)

diff --git a/packages/vite/src/node/optimizer/esbuildDepPlugin.ts b/packages/vite/src/node/optimizer/esbuildDepPlugin.ts
index 1bcd52d3100839..ede440f6f8bb00 100644
--- a/packages/vite/src/node/optimizer/esbuildDepPlugin.ts
+++ b/packages/vite/src/node/optimizer/esbuildDepPlugin.ts
@@ -1,6 +1,6 @@
 import path from 'node:path'
 import type { ImportKind, Plugin } from 'esbuild'
-import { KNOWN_ASSET_TYPES } from '../constants'
+import { JS_TYPES_RE, KNOWN_ASSET_TYPES } from '../constants'
 import type { PackageCache } from '../packages'
 import {
   escapeRegex,
@@ -160,7 +160,7 @@ export function esbuildDepPlugin(
             // `resolved` can be javascript even when `id` matches `allExternalTypes`
             // due to cjs resolution (e.g. require("./test.pdf") for "./test.pdf.js")
             // or package name (e.g. import "some-package.pdf")
-            if (resolved.endsWith('.js')) {
+            if (JS_TYPES_RE.test(resolved)) {
               return {
                 path: resolved,
                 external: false,
diff --git a/playground/optimize-deps/dep-with-asset-ext/dep1/index.js b/playground/optimize-deps/dep-with-asset-ext/dep1/index.mjs
similarity index 100%
rename from playground/optimize-deps/dep-with-asset-ext/dep1/index.js
rename to playground/optimize-deps/dep-with-asset-ext/dep1/index.mjs
diff --git a/playground/optimize-deps/dep-with-asset-ext/dep1/package.json b/playground/optimize-deps/dep-with-asset-ext/dep1/package.json
index 620170d8f66566..cec7eb4442bec8 100644
--- a/playground/optimize-deps/dep-with-asset-ext/dep1/package.json
+++ b/playground/optimize-deps/dep-with-asset-ext/dep1/package.json
@@ -2,5 +2,5 @@
   "name": "@vitejs/test-dep-with-asset-ext1.pdf",
   "private": true,
   "type": "module",
-  "exports": "./index.js"
+  "exports": "./index.mjs"
 }