fix(ssr): preserve require for external node (#11057)

sapphi-red · web-flow · commit 1ec017670f0f · 2022-11-30T05:46:27.000+01:00
diff --git a/packages/vite/src/node/optimizer/esbuildDepPlugin.ts b/packages/vite/src/node/optimizer/esbuildDepPlugin.ts
@@ -263,7 +263,10 @@ module.exports = Object.create(new Proxy({}, {
 
 // esbuild doesn't transpile `require('foo')` into `import` statements if 'foo' is externalized
 // https://github.com/evanw/esbuild/issues/566#issuecomment-735551834
-export function esbuildCjsExternalPlugin(externals: string[]): Plugin {
+export function esbuildCjsExternalPlugin(
+  externals: string[],
+  platform: 'node' | 'browser'
+): Plugin {
   return {
     name: 'cjs-external',
     setup(build) {
@@ -279,7 +282,8 @@ export function esbuildCjsExternalPlugin(externals: string[]): Plugin {
       })
 
       build.onResolve({ filter }, (args) => {
-        if (args.kind === 'require-call') {
+        // preserve `require` for node because it's more accurate than converting it to import
+        if (args.kind === 'require-call' && platform !== 'node') {
           return {
             path: args.path,
             namespace: cjsExternalFacadeNamespace
diff --git a/packages/vite/src/node/optimizer/index.ts b/packages/vite/src/node/optimizer/index.ts
@@ -578,7 +578,7 @@ export async function runOptimizeDeps(
 
   const plugins = [...pluginsFromConfig]
   if (external.length) {
-    plugins.push(esbuildCjsExternalPlugin(external))
+    plugins.push(esbuildCjsExternalPlugin(external, platform))
   }
   plugins.push(esbuildDepPlugin(flatIdDeps, external, config, ssr))
 
diff --git a/playground/ssr-noexternal/__tests__/serve.ts b/playground/ssr-noexternal/__tests__/serve.ts
@@ -0,0 +1,52 @@
+// this is automatically detected by playground/vitestSetup.ts and will replace
+// the default e2e test serve behavior
+
+import path from 'node:path'
+import kill from 'kill-port'
+import { hmrPorts, isBuild, ports, rootDir } from '~utils'
+
+export const port = ports['ssr-noexternal']
+
+export async function serve(): Promise<{ close(): Promise<void> }> {
+  if (isBuild) {
+    // build first
+    const { build } = await import('vite')
+    // server build
+    await build({
+      root: rootDir,
+      logLevel: 'silent',
+      build: {
+        ssr: 'src/entry-server.js'
+      }
+    })
+  }
+
+  await kill(port)
+
+  const { createServer } = await import(path.resolve(rootDir, 'server.js'))
+  const { app, vite } = await createServer(
+    rootDir,
+    isBuild,
+    hmrPorts['ssr-noexternal']
+  )
+
+  return new Promise((resolve, reject) => {
+    try {
+      const server = app.listen(port, () => {
+        resolve({
+          // for test teardown
+          async close() {
+            await new Promise((resolve) => {
+              server.close(resolve)
+            })
+            if (vite) {
+              await vite.close()
+            }
+          }
+        })
+      })
+    } catch (e) {
+      reject(e)
+    }
+  })
+}
diff --git a/playground/ssr-noexternal/__tests__/ssr-noexternal.spec.ts b/playground/ssr-noexternal/__tests__/ssr-noexternal.spec.ts
@@ -0,0 +1,10 @@
+import { expect, test } from 'vitest'
+import { port } from './serve'
+import { page } from '~utils'
+
+const url = `http://localhost:${port}`
+
+test('message from require-external-cjs', async () => {
+  await page.goto(url)
+  expect(await page.textContent('.require-external-cjs')).toMatch('foo')
+})
diff --git a/playground/ssr-noexternal/external-cjs/import.mjs b/playground/ssr-noexternal/external-cjs/import.mjs
@@ -0,0 +1 @@
+throw new Error('shouldnt be loaded')
diff --git a/playground/ssr-noexternal/external-cjs/package.json b/playground/ssr-noexternal/external-cjs/package.json
@@ -0,0 +1,9 @@
+{
+  "name": "@vitejs/external-cjs",
+  "private": true,
+  "version": "0.0.0",
+  "exports": {
+    "require": "./require.cjs",
+    "import": "./import.mjs"
+  }
+}
diff --git a/playground/ssr-noexternal/external-cjs/require.cjs b/playground/ssr-noexternal/external-cjs/require.cjs
@@ -0,0 +1 @@
+module.exports = 'foo'
diff --git a/playground/ssr-noexternal/index.html b/playground/ssr-noexternal/index.html
@@ -0,0 +1,11 @@
+<!DOCTYPE html>
+<html lang="en">
+  <head>
+    <meta charset="UTF-8" />
+    <meta name="viewport" content="width=device-width, initial-scale=1.0" />
+    <title>Vite App</title>
+  </head>
+  <body>
+    <div id="app"><!--app-html--></div>
+  </body>
+</html>
diff --git a/playground/ssr-noexternal/package.json b/playground/ssr-noexternal/package.json
@@ -0,0 +1,17 @@
+{
+  "name": "@vitejs/test-ssr-noexternal",
+  "private": true,
+  "version": "0.0.0",
+  "type": "module",
+  "scripts": {
+    "dev": "node server",
+    "build": "vite build --ssr src/entry-server.js",
+    "serve": "NODE_ENV=production node server",
+    "debug": "node --inspect-brk server"
+  },
+  "dependencies": {
+    "@vitejs/external-cjs": "file:./external-cjs",
+    "@vitejs/require-external-cjs": "file:./require-external-cjs",
+    "express": "^4.18.2"
+  }
+}
diff --git a/playground/ssr-noexternal/require-external-cjs/main.js b/playground/ssr-noexternal/require-external-cjs/main.js
@@ -0,0 +1 @@
+module.exports = require('@vitejs/external-cjs')
diff --git a/playground/ssr-noexternal/require-external-cjs/package.json b/playground/ssr-noexternal/require-external-cjs/package.json
@@ -0,0 +1,10 @@
+{
+  "name": "@vitejs/require-external-cjs",
+  "type": "commonjs",
+  "private": true,
+  "version": "0.0.0",
+  "main": "main.js",
+  "dependencies": {
+    "@vitejs/external-cjs": "file:../external-cjs"
+  }
+}
diff --git a/playground/ssr-noexternal/server.js b/playground/ssr-noexternal/server.js
@@ -0,0 +1,87 @@
+import fs from 'node:fs'
+import path from 'node:path'
+import { fileURLToPath } from 'node:url'
+import express from 'express'
+
+const __dirname = path.dirname(fileURLToPath(import.meta.url))
+
+const isTest = process.env.VITEST
+
+export async function createServer(
+  root = process.cwd(),
+  isProd = process.env.NODE_ENV === 'production',
+  hmrPort
+) {
+  const resolve = (p) => path.resolve(__dirname, p)
+
+  const indexProd = isProd
+    ? fs.readFileSync(resolve('index.html'), 'utf-8')
+    : ''
+
+  const app = express()
+
+  /**
+   * @type {import('vite').ViteDevServer}
+   */
+  let vite
+  if (!isProd) {
+    vite = await (
+      await import('vite')
+    ).createServer({
+      root,
+      logLevel: isTest ? 'error' : 'info',
+      server: {
+        middlewareMode: true,
+        watch: {
+          // During tests we edit the files too fast and sometimes chokidar
+          // misses change events, so enforce polling for consistency
+          usePolling: true,
+          interval: 100
+        },
+        hmr: {
+          port: hmrPort
+        }
+      },
+      appType: 'custom'
+    })
+    app.use(vite.middlewares)
+  }
+
+  app.use('*', async (req, res) => {
+    try {
+      const url = req.originalUrl
+
+      let template, render
+      if (!isProd) {
+        // always read fresh template in dev
+        template = fs.readFileSync(resolve('index.html'), 'utf-8')
+        template = await vite.transformIndexHtml(url, template)
+        render = (await vite.ssrLoadModule('/src/entry-server.js')).render
+      } else {
+        template = indexProd
+        // @ts-ignore
+        render = (await import('./dist/entry-server.js')).render
+      }
+
+      const appHtml = await render(url)
+
+      const html = template.replace(`<!--app-html-->`, appHtml)
+
+      res.status(200).set({ 'Content-Type': 'text/html' }).end(html)
+    } catch (e) {
+      !isProd && vite.ssrFixStacktrace(e)
+      console.log(e.stack)
+      res.status(500).end(e.stack)
+    }
+  })
+
+  return { app, vite }
+}
+
+if (!isTest) {
+  createServer().then(({ app }) =>
+    app.listen(5173, () => {
+      console.log('http://localhost:5173')
+    })
+  )
+}
diff --git a/playground/ssr-noexternal/src/entry-server.js b/playground/ssr-noexternal/src/entry-server.js
@@ -0,0 +1,9 @@
+import requireExternalCjs from '@vitejs/require-external-cjs'
+
+export async function render(url) {
+  let html = ''
+
+  html += `\n<p class="require-external-cjs">message from require-external-cjs: ${requireExternalCjs}</p>`
+
+  return html + '\n'
+}
diff --git a/playground/ssr-noexternal/vite.config.js b/playground/ssr-noexternal/vite.config.js
@@ -0,0 +1,22 @@
+import module from 'node:module'
+import { defineConfig } from 'vite'
+
+export default defineConfig({
+  ssr: {
+    noExternal: ['@vitejs/require-external-cjs'],
+    external: ['@vitejs/external-cjs'],
+    optimizeDeps: {
+      disabled: false
+    }
+  },
+  build: {
+    target: 'esnext',
+    minify: false,
+    rollupOptions: {
+      external: ['@vitejs/external-cjs']
+    },
+    commonjsOptions: {
+      include: []
+    }
+  }
+})
diff --git a/playground/test-utils.ts b/playground/test-utils.ts
@@ -24,10 +24,11 @@ export const ports = {
   'legacy/client-and-ssr': 9523,
   'ssr-deps': 9600,
   'ssr-html': 9601,
-  'ssr-pug': 9602,
-  'ssr-react': 9603,
-  'ssr-vue': 9604,
-  'ssr-webworker': 9605,
+  'ssr-noexternal': 9602,
+  'ssr-pug': 9603,
+  'ssr-react': 9604,
+  'ssr-vue': 9605,
+  'ssr-webworker': 9606,
   'css/postcss-caching': 5005,
   'css/postcss-plugins-different-dir': 5006,
   'css/dynamic-import': 5007
@@ -36,9 +37,10 @@ export const hmrPorts = {
   'optimize-missing-deps': 24680,
   'ssr-deps': 24681,
   'ssr-html': 24682,
-  'ssr-pug': 24683,
-  'ssr-react': 24684,
-  'ssr-vue': 24685
+  'ssr-noexternal': 24683,
+  'ssr-pug': 24684,
+  'ssr-react': 24685,
+  'ssr-vue': 24686
 }
 
 const hexToNameMap: Record<string, string> = {}
diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml

Original file line number	Diff line number	Diff line change
`@@ -578,7 +578,7 @@ export async function runOptimizeDeps(`
`578`	`578`
`579`	`579`	`const plugins = [...pluginsFromConfig]`
`580`	`580`	`if (external.length) {`
`581`		`- plugins.push(esbuildCjsExternalPlugin(external))`
	`581`	`+ plugins.push(esbuildCjsExternalPlugin(external, platform))`
`582`	`582`	`}`
`583`	`583`	`plugins.push(esbuildDepPlugin(flatIdDeps, external, config, ssr))`
`584`	`584`
Original file line number	Diff line number	Diff line change
`@@ -0,0 +1 @@`
	`1`	`+module.exports = require('@vitejs/external-cjs')`