feat: handle named imports of builtin modules (#8338)

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

@@ -219,15 +219,37 @@ export function esbuildDepPlugin(
 
       build.onLoad(
         { filter: /.*/, namespace: 'browser-external' },
-        ({ path: id }) => {
+        ({ path }) => {
           return {
-            contents:
-              `export default new Proxy({}, {
-  get() {
-    throw new Error('Module "${id}" has been externalized for ` +
-              `browser compatibility and cannot be accessed in client code.')
+            // Return in CJS to intercept named imports. Use `Object.create` to
+            // create the Proxy in the prototype to workaround esbuild issue. Why?
+            //
+            // In short, esbuild cjs->esm flow:
+            // 1. Create empty object using `Object.create(Object.getPrototypeOf(module.exports))`.
+            // 2. Assign props of `module.exports` to the object.
+            // 3. Return object for ESM use.
+            //
+            // If we do `module.exports = new Proxy({}, {})`, step 1 returns empty object,
+            // step 2 does nothing as there's no props for `module.exports`. The final object
+            // is just an empty object.
+            //
+            // Creating the Proxy in the prototype satisfies step 1 immediately, which means
+            // the returned object is a Proxy that we can intercept.
+            //
+            // Note: Skip keys that are accessed by esbuild and browser devtools.
+            contents: `\
+module.exports = Object.create(new Proxy({}, {
+  get(_, key) {
+    if (
+      key !== '__esModule' &&
+      key !== '__proto__' &&
+      key !== 'constructor' &&
+      key !== 'splice'
+    ) {
+      throw new Error(\`Module "${path}" has been externalized for browser compatibility. Cannot access "${path}.\${key}" in client code.\`)
+    }
   }
-})`
+}))`
           }
         }
       )

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

@@ -58,6 +58,7 @@ import {
   delayDepsOptimizerUntil
 } from './optimizedDeps'
 import { isCSSRequest, isDirectCSSRequest } from './css'
+import { browserExternalId } from './resolve'
 
 const isDebug = !!process.env.DEBUG
 const debug = createDebugger('vite:import-analysis')
@@ -322,11 +323,9 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin {
           s: start,
           e: end,
           ss: expStart,
-          se: expEnd,
           d: dynamicIndex,
           // #2083 User may use escape path,
           // so use imports[index].n to get the unescaped string
-          // @ts-ignore
           n: specifier
         } = imports[index]
 
@@ -434,29 +433,20 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin {
                 }
               } else if (needsInterop) {
                 debug(`${url} needs interop`)
-                if (isDynamicImport) {
-                  // rewrite `import('package')` to expose the default directly
-                  str().overwrite(
-                    expStart,
-                    expEnd,
-                    `import('${url}').then(m => m.default && m.default.__esModule ? m.default : ({ ...m.default, default: m.default }))`,
-                    { contentOnly: true }
-                  )
-                } else {
-                  const exp = source.slice(expStart, expEnd)
-                  const rewritten = transformCjsImport(exp, url, rawUrl, index)
-                  if (rewritten) {
-                    str().overwrite(expStart, expEnd, rewritten, {
-                      contentOnly: true
-                    })
-                  } else {
-                    // #1439 export * from '...'
-                    str().overwrite(start, end, url, { contentOnly: true })
-                  }
-                }
+                interopNamedImports(str(), imports[index], url, index)
                 rewriteDone = true
               }
             }
+            // If source code imports builtin modules via named imports, the stub proxy export
+            // would fail as it's `export default` only. Apply interop for builtin modules to
+            // correctly throw the error message.
+            else if (
+              url.includes(browserExternalId) &&
+              source.slice(expStart, start).includes('{')
+            ) {
+              interopNamedImports(str(), imports[index], url, index)
+              rewriteDone = true
+            }
             if (!rewriteDone) {
               str().overwrite(start, end, isDynamicImport ? `'${url}'` : url, {
                 contentOnly: true
@@ -639,6 +629,41 @@ export function importAnalysisPlugin(config: ResolvedConfig): Plugin {
   }
 }
 
+function interopNamedImports(
+  str: MagicString,
+  importSpecifier: ImportSpecifier,
+  rewrittenUrl: string,
+  importIndex: number
+) {
+  const source = str.original
+  const {
+    s: start,
+    e: end,
+    ss: expStart,
+    se: expEnd,
+    d: dynamicIndex
+  } = importSpecifier
+  if (dynamicIndex > -1) {
+    // rewrite `import('package')` to expose the default directly
+    str.overwrite(
+      expStart,
+      expEnd,
+      `import('${rewrittenUrl}').then(m => m.default && m.default.__esModule ? m.default : ({ ...m.default, default: m.default }))`,
+      { contentOnly: true }
+    )
+  } else {
+    const exp = source.slice(expStart, expEnd)
+    const rawUrl = source.slice(start, end)
+    const rewritten = transformCjsImport(exp, rewrittenUrl, rawUrl, importIndex)
+    if (rewritten) {
+      str.overwrite(expStart, expEnd, rewritten, { contentOnly: true })
+    } else {
+      // #1439 export * from '...'
+      str.overwrite(start, end, rewrittenUrl, { contentOnly: true })
+    }
+  }
+}
+
 type ImportNameSpecifier = { importedName: string; localName: string }
 
 /**

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

@@ -339,15 +339,17 @@ export function resolvePlugin(baseOptions: InternalResolveOptions): Plugin {
 
     load(id) {
       if (id.startsWith(browserExternalId)) {
-        return isProduction
-          ? `export default {}`
-          : `export default new Proxy({}, {
-  get() {
-    throw new Error('Module "${id.slice(
-      browserExternalId.length + 1
-    )}" has been externalized for browser compatibility and cannot be accessed in client code.')
+        if (isProduction) {
+          return `export default {}`
+        } else {
+          id = id.slice(browserExternalId.length + 1)
+          return `\
+export default new Proxy({}, {
+  get(_, key) {
+    throw new Error(\`Module "${id}" has been externalized for browser compatibility. Cannot access "${id}.\${key}" in client code.\`)
   }
 })`
+        }
       }
     }
   }

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

@@ -1,4 +1,11 @@
-import { getColor, page } from '~utils'
+import {
+  browserErrors,
+  browserLogs,
+  getColor,
+  isBuild,
+  isServe,
+  page
+} from '~utils'
 
 test('default + named imports from cjs dep (react)', async () => {
   expect(await page.textContent('.cjs button')).toBe('count is 0')
@@ -63,7 +70,7 @@ test('import * from optimized dep', async () => {
 })
 
 test('import from dep with process.env.NODE_ENV', async () => {
-  expect(await page.textContent('.node-env')).toMatch(`prod`)
+  expect(await page.textContent('.node-env')).toMatch(isBuild ? 'prod' : 'dev')
 })
 
 test('import from dep with .notjs files', async () => {
@@ -113,3 +120,33 @@ test('import aliased package with colon', async () => {
 test('variable names are reused in different scripts', async () => {
   expect(await page.textContent('.reused-variable-names')).toBe('reused')
 })
+
+test.runIf(isServe)('error on builtin modules usage', () => {
+  expect(browserLogs).toEqual(
+    expect.arrayContaining([
+      // from dep-with-builtin-module-esm top-level try-catch
+      expect.stringContaining(
+        'dep-with-builtin-module-esm Error: Module "fs" has been externalized for browser compatibility. Cannot access "fs.readFileSync" in client code.'
+      ),
+      expect.stringContaining(
+        'dep-with-builtin-module-esm Error: Module "path" has been externalized for browser compatibility. Cannot access "path.join" in client code.'
+      ),
+      // from dep-with-builtin-module-cjs top-level try-catch
+      expect.stringContaining(
+        'dep-with-builtin-module-cjs Error: Module "path" has been externalized for browser compatibility. Cannot access "path.join" in client code.'
+      )
+    ])
+  )
+
+  expect(browserErrors.map((error) => error.message)).toEqual(
+    expect.arrayContaining([
+      // from user source code
+      'Module "buffer" has been externalized for browser compatibility. Cannot access "buffer.Buffer" in client code.',
+      'Module "child_process" has been externalized for browser compatibility. Cannot access "child_process.execSync" in client code.',
+      // from dep-with-builtin-module-esm read()
+      'Module "fs" has been externalized for browser compatibility. Cannot access "fs.readFileSync" in client code.',
+      // from dep-with-builtin-module-esm read()
+      'Module "fs" has been externalized for browser compatibility. Cannot access "fs.readFileSync" in client code.'
+    ])
+  )
+})

playground/optimize-deps/dep-with-builtin-module-cjs/index.js

@@ -0,0 +1,18 @@
+const fs = require('fs')
+const path = require('path')
+
+// NOTE: require destructure would error immediately because of how esbuild
+// compiles it. There's no way around it as it's direct property access, which
+// triggers the Proxy get trap.
+
+// access from default import
+try {
+  path.join()
+} catch (e) {
+  console.log('dep-with-builtin-module-cjs', e)
+}
+
+// access from function
+module.exports.read = () => {
+  return fs.readFileSync('test')
+}

playground/optimize-deps/dep-with-builtin-module-cjs/package.json

@@ -0,0 +1,6 @@
+{
+  "name": "dep-with-builtin-module-cjs",
+  "private": true,
+  "version": "0.0.0",
+  "main": "index.js"
+}

playground/optimize-deps/dep-with-builtin-module-esm/index.js

@@ -0,0 +1,21 @@
+import { readFileSync } from 'fs'
+import path from 'path'
+
+// access from named import
+try {
+  readFileSync()
+} catch (e) {
+  console.log('dep-with-builtin-module-esm', e)
+}
+
+// access from default import
+try {
+  path.join()
+} catch (e) {
+  console.log('dep-with-builtin-module-esm', e)
+}
+
+// access from function
+export function read() {
+  return readFileSync('test')
+}

playground/optimize-deps/dep-with-builtin-module-esm/package.json

@@ -0,0 +1,7 @@
+{
+  "name": "dep-with-builtin-module-esm",
+  "private": true,
+  "version": "0.0.0",
+  "main": "index.js",
+  "type": "module"
+}

playground/optimize-deps/index.html

@@ -138,3 +138,34 @@ <h2>Reused variable names</h2>
   const reusedName = 'reused'
   text('.reused-variable-names', reusedName)
 </script>
+
+<script type="module">
+  // should error on builtin modules (named import)
+  import { Buffer } from 'buffer'
+  // named imports error immediately
+</script>
+
+<script type="module">
+  // should error on builtin modules (default import)
+  import cp from 'child_process'
+  // must access property to error
+  if (import.meta.env.DEV) {
+    cp.execSync()
+  }
+</script>
+
+<script type="module">
+  // should error on builtin modules from dep
+  import { read } from 'dep-with-builtin-module-esm'
+  if (import.meta.env.DEV) {
+    read()
+  }
+</script>
+
+<script type="module">
+  // should error on builtin modules from dep
+  import { read } from 'dep-with-builtin-module-cjs'
+  if (import.meta.env.DEV) {
+    read()
+  }
+</script>

playground/optimize-deps/package.json

@@ -19,6 +19,8 @@
     "dep-linked-include": "link:./dep-linked-include",
     "dep-node-env": "file:./dep-node-env",
     "dep-not-js": "file:./dep-not-js",
+    "dep-with-builtin-module-cjs": "file:./dep-with-builtin-module-cjs",
+    "dep-with-builtin-module-esm": "file:./dep-with-builtin-module-esm",
     "dep-with-dynamic-import": "file:./dep-with-dynamic-import",
     "lodash-es": "^4.17.21",
     "nested-exclude": "file:./nested-exclude",

playground/optimize-deps/vite.config.js

@@ -62,6 +62,19 @@ module.exports = {
           return { code }
         }
       }
+    },
+    // TODO: Remove this one support for prebundling in build lands.
+    // It is expected that named importing in build doesn't work
+    // as it incurs a lot of overhead in build.
+    {
+      name: 'polyfill-named-fs-build',
+      apply: 'build',
+      enforce: 'pre',
+      load(id) {
+        if (id === '__vite-browser-external:fs') {
+          return `export default {}; export function readFileSync() {}`
+        }
+      }
     }
   ]
 }