fix: importing an optional peer dep should throw an runtime error

packages/vite/src/node/optimizer/esbuildDepPlugin.ts

@@ -282,15 +282,11 @@ module.exports = Object.create(new Proxy({}, {
       build.onLoad(
         { filter: /.*/, namespace: 'optional-peer-dep' },
         ({ path }) => {
-          if (isProduction) {
-            return {
-              contents: 'module.exports = {}',
-            }
-          } else {
-            const [, peerDep, parentDep] = path.split(':')
-            return {
-              contents: `throw new Error(\`Could not resolve "${peerDep}" imported by "${parentDep}". Is it installed?\`)`,
-            }
+          const [, peerDep, parentDep] = path.split(':')
+          return {
+            contents:
+              'module.exports = {};' +
+              `throw new Error(\`Could not resolve "${peerDep}" imported by "${parentDep}".${isProduction ? '' : ' Is it installed?'}\`)`,
           }
         },
       )

packages/vite/src/node/plugins/resolve.ts

@@ -488,12 +488,18 @@ export function resolvePlugin(
           }
         }
         if (id.startsWith(optionalPeerDepId)) {
-          if (isProduction) {
-            return `export default {}`
-          } else {
-            const [, peerDep, parentDep] = id.split(':')
-            return `throw new Error(\`Could not resolve "${peerDep}" imported by "${parentDep}". Is it installed?\`)`
+          const [, peerDep, parentDep, isRequire] = id.split(':')
+          // rollup + @rollup/plugin-commonjs hoists dynamic `require`s by default
+          // If we add a `throw` statement, it will be injected to the top-level and break the whole bundle
+          // Instead, we mock the module for now
+          // This can be fixed when we migrate to rolldown
+          if (isRequire === 'true' && isProduction) {
+            return 'export default {}'
           }
+          return (
+            'export default {};' +
+            `throw new Error(\`Could not resolve "${peerDep}" imported by "${parentDep}".${isProduction ? '' : ' Is it installed?'}\`)`
+          )
         }
       },
     },
@@ -758,7 +764,7 @@ export function tryNodeResolve(
           mainPkg.peerDependenciesMeta?.[pkgName]?.optional
         ) {
           return {
-            id: `${optionalPeerDepId}:${id}:${mainPkg.name}`,
+            id: `${optionalPeerDepId}:${id}:${mainPkg.name}:${!!options.isRequire}`,
           }
         }
       }

playground/optimize-deps/__tests__/optimize-deps.spec.ts

@@ -142,26 +142,28 @@ test('dep with optional peer dep', async () => {
   await expectWithRetry(() =>
     page.textContent('.dep-with-optional-peer-dep'),
   ).toMatch(`[success]`)
-  if (isServe) {
-    expect(browserErrors.map((error) => error.message)).toEqual(
-      expect.arrayContaining([
-        'Could not resolve "foobar" imported by "@vitejs/test-dep-with-optional-peer-dep". Is it installed?',
-      ]),
-    )
-  }
+  await expectWithRetry(() =>
+    page.textContent('.dep-with-optional-peer-dep-error'),
+  ).toMatch(`[success]`)
 })
 
 test('dep with optional peer dep submodule', async () => {
   await expectWithRetry(() =>
     page.textContent('.dep-with-optional-peer-dep-submodule'),
   ).toMatch(`[success]`)
-  if (isServe) {
-    expect(browserErrors.map((error) => error.message)).toEqual(
-      expect.arrayContaining([
-        'Could not resolve "foobar/baz" imported by "@vitejs/test-dep-with-optional-peer-dep-submodule". Is it installed?',
-      ]),
-    )
-  }
+  await expectWithRetry(() =>
+    page.textContent('.dep-with-optional-peer-dep-submodule-error'),
+  ).toMatch(`[success]`)
+})
+
+test('dep with optional peer dep (cjs)', async () => {
+  await expectWithRetry(() =>
+    page.textContent('.dep-with-optional-peer-dep-cjs'),
+  ).toMatch(`[success]`)
+  // FIXME
+  // await expectWithRetry(() =>
+  //   page.textContent('.dep-with-optional-peer-dep-cjs-error'),
+  // ).toMatch(`[success]`)
 })
 
 test('dep with css import', async () => {

playground/optimize-deps/dep-with-optional-peer-dep-cjs/index.js

@@ -0,0 +1,11 @@
+exports.callItself = function () {
+  return '[success]'
+}
+
+exports.callPeerDep = function () {
+  try {
+    return require('foobar')
+  } catch {
+    return 'fallback'
+  }
+}

playground/optimize-deps/dep-with-optional-peer-dep-cjs/package.json

@@ -0,0 +1,14 @@
+{
+  "name": "@vitejs/test-dep-with-optional-peer-dep-cjs",
+  "private": true,
+  "version": "0.0.0",
+  "main": "index.js",
+  "peerDependencies": {
+    "foobar": "0.0.0"
+  },
+  "peerDependenciesMeta": {
+    "foobar": {
+      "optional": true
+    }
+  }
+}

playground/optimize-deps/index.html

@@ -68,9 +68,15 @@ <h2>Import from dependency with dynamic import</h2>
 
 <h2>Import from dependency with optional peer dep</h2>
 <div class="dep-with-optional-peer-dep"></div>
+<div class="dep-with-optional-peer-dep-error"></div>
 
 <h2>Import from dependency with optional peer dep submodule</h2>
 <div class="dep-with-optional-peer-dep-submodule"></div>
+<div class="dep-with-optional-peer-dep-submodule-error"></div>
+
+<h2>Import from dependency with optional peer dep (cjs)</h2>
+<div class="dep-with-optional-peer-dep-cjs"></div>
+<div class="dep-with-optional-peer-dep-cjs-error"></div>
 
 <h2>Externalize known non-js files in optimize included dep</h2>
 <div class="externalize-known-non-js"></div>
@@ -232,8 +238,20 @@ <h2>Import the CommonJS external package that omits the js suffix</h2>
     callPeerDep,
   } from '@vitejs/test-dep-with-optional-peer-dep'
   text('.dep-with-optional-peer-dep', callItself())
+
   // expect error as optional peer dep not installed
-  callPeerDep()
+  callPeerDep().catch((e) => {
+    text(
+      '.dep-with-optional-peer-dep-error',
+      e &&
+        typeof e.message === 'string' &&
+        e.message.includes(
+          'Could not resolve "foobar" imported by "@vitejs/test-dep-with-optional-peer-dep".',
+        )
+        ? '[success]'
+        : `[failed]: ${e.message}`,
+    )
+  })
 </script>
 
 <script type="module">
@@ -242,8 +260,34 @@ <h2>Import the CommonJS external package that omits the js suffix</h2>
     callPeerDepSubmodule,
   } from '@vitejs/test-dep-with-optional-peer-dep-submodule'
   text('.dep-with-optional-peer-dep-submodule', callItself())
+
   // expect error as optional peer dep not installed
-  callPeerDepSubmodule()
+  callPeerDepSubmodule().catch((e) => {
+    text(
+      '.dep-with-optional-peer-dep-submodule-error',
+      e &&
+        typeof e.message === 'string' &&
+        e.message.includes(
+          'Could not resolve "foobar/baz" imported by "@vitejs/test-dep-with-optional-peer-dep-submodule".',
+        )
+        ? '[success]'
+        : `[failed]: ${e.message}`,
+    )
+  })
+</script>
+
+<script type="module">
+  import {
+    callItself,
+    callPeerDep,
+  } from '@vitejs/test-dep-with-optional-peer-dep-cjs'
+  text('.dep-with-optional-peer-dep-cjs', callItself())
+
+  // expect fallback as optional peer dep not installed
+  text(
+    '.dep-with-optional-peer-dep-cjs-error',
+    callPeerDep() === 'fallback' ? '[success]' : `[failed]: did not fallback`,
+  )
 </script>
 
 <script type="module">

playground/optimize-deps/package.json

@@ -37,6 +37,7 @@
     "@vitejs/test-dep-with-builtin-module-esm": "file:./dep-with-builtin-module-esm",
     "@vitejs/test-dep-with-dynamic-import": "file:./dep-with-dynamic-import",
     "@vitejs/test-dep-with-optional-peer-dep": "file:./dep-with-optional-peer-dep",
+    "@vitejs/test-dep-with-optional-peer-dep-cjs": "file:./dep-with-optional-peer-dep-cjs",
     "@vitejs/test-dep-with-optional-peer-dep-submodule": "file:./dep-with-optional-peer-dep-submodule",
     "@vitejs/test-dep-non-optimized": "file:./dep-non-optimized",
     "@vitejs/test-added-in-entries": "file:./added-in-entries",