Merge branch '08-11-fix_rsc_remove_non-optimized_server_cjs_warning' into 08-11-refactor_rsc_remove_server_optimizedeps.include

Author: hi-ogawa

packages/plugin-rsc/e2e/basic.test.ts

@@ -58,12 +58,20 @@ test.describe('dev-non-optimized-cjs', () => {
     )
   })
 
-  const f = useFixture({ root: 'examples/basic', mode: 'dev' })
+  const f = useFixture({
+    root: 'examples/basic',
+    mode: 'dev',
+    cliOptions: {
+      env: {
+        DEBUG: 'vite-rsc:cjs',
+      },
+    },
+  })
 
   test('show warning', async ({ page }) => {
     await page.goto(f.url())
-    expect(f.proc().stderr()).toContain(
-      `Found non-optimized CJS dependency in 'ssr' environment.`,
+    expect(f.proc().stderr()).toMatch(
+      /non-optimized CJS dependency in 'ssr' environment.*@vitejs\/test-dep-cjs\/index.js/,
     )
   })
 })

packages/plugin-rsc/examples/react-router/cf/vite.config.ts

@@ -36,21 +36,6 @@ export default defineConfig({
         },
       ],
     }),
-    {
-      name: 'no-server-optimize-deps',
-      configEnvironment: {
-        order: 'post',
-        handler(name) {
-          if (name === 'ssr' || name === 'rsc') {
-            return {
-              optimizeDeps: {
-                noDiscovery: true,
-              },
-            }
-          }
-        },
-      },
-    },
   ],
   environments: {
     client: {
@@ -60,14 +45,12 @@ export default defineConfig({
     },
     ssr: {
       optimizeDeps: {
-        // include: ['react-router > cookie', 'react-router > set-cookie-parser'],
-        // exclude: ['react-router'],
+        exclude: ['react-router'],
       },
     },
     rsc: {
       optimizeDeps: {
-        // include: ['react-router > cookie', 'react-router > set-cookie-parser'],
-        // exclude: ['react-router'],
+        exclude: ['react-router'],
       },
     },
   },

packages/plugin-rsc/src/plugin.ts

@@ -32,7 +32,11 @@ import {
 } from './transforms'
 import { generateEncryptionKey, toBase64 } from './utils/encryption-utils'
 import { createRpcServer } from './utils/rpc'
-import { normalizeViteImportAnalysisUrl, prepareError } from './vite-utils'
+import {
+  cleanUrl,
+  normalizeViteImportAnalysisUrl,
+  prepareError,
+} from './vite-utils'
 import { cjsModuleRunnerPlugin } from './plugins/cjs'
 import { evalValue, parseIdQuery } from './plugins/utils'
 
@@ -959,9 +963,12 @@ function vitePluginUseClient(
         let importId: string
         let referenceKey: string
         const packageSource = packageSources.get(id)
-        if (!packageSource && id.includes('?v=')) {
-          assert(this.environment.mode === 'dev')
-          // If non package source `?v=<hash>` reached here, this is a client boundary
+        if (
+          !packageSource &&
+          this.environment.mode === 'dev' &&
+          id.includes('/node_modules/')
+        ) {
+          // If non package source reached here (often with ?v=... query), this is a client boundary
           // created by a package imported on server environment, which breaks the
           // expectation on dependency optimizer on browser. Directly copying over
           // "?v=<hash>" from client optimizer in client reference can make a hashed
@@ -978,9 +985,7 @@ function vitePluginUseClient(
               `[vite-rsc] detected an internal client boundary created by a package imported on rsc environment`,
             )
           }
-          importId = `/@id/__x00__virtual:vite-rsc/client-in-server-package-proxy/${encodeURIComponent(
-            id.split('?v=')[0]!,
-          )}`
+          importId = `/@id/__x00__virtual:vite-rsc/client-in-server-package-proxy/${encodeURIComponent(cleanUrl(id))}`
           referenceKey = importId
         } else if (packageSource) {
           if (this.environment.mode === 'dev') {
@@ -1223,8 +1228,10 @@ function vitePluginUseServer(
         let normalizedId_: string | undefined
         const getNormalizedId = () => {
           if (!normalizedId_) {
-            if (id.includes('?v=')) {
-              assert(this.environment.mode === 'dev')
+            if (
+              this.environment.mode === 'dev' &&
+              id.includes('/node_modules/')
+            ) {
               const ignored =
                 useServerPluginOptions.ignoredPackageWarnings?.some(
                   (pattern) =>
@@ -1238,8 +1245,8 @@ function vitePluginUseServer(
                 )
               }
               // module runner has additional resolution step and it's not strict about
-              // module identity of `import(id)` like browser, so we simply strip it off.
-              id = id.split('?v=')[0]!
+              // module identity of `import(id)` like browser, so we simply strip queries such as `?v=`.
+              id = cleanUrl(id)
             }
             if (config.command === 'build') {
               normalizedId_ = hashString(path.relative(config.root, id))

packages/plugin-rsc/src/plugins/cjs.ts

@@ -5,15 +5,12 @@ import path from 'node:path'
 import fs from 'node:fs'
 import * as esModuleLexer from 'es-module-lexer'
 import { transformCjsToEsm } from '../transforms/cjs'
+import { createDebug } from '@hiogawa/utils'
+
+const debug = createDebug('vite-rsc:cjs')
 
 export function cjsModuleRunnerPlugin(): Plugin[] {
-  // use-sync-external-store is known to work fine so don't show warning
-  const warnedPackages = new Set<string>([
-    'use-sync-external-store',
-    'react',
-    'react-dom',
-    '@vitejs/plugin-rsc',
-  ])
+  const warnedPackages = new Set<string>()
 
   return [
     {
@@ -52,9 +49,8 @@ export function cjsModuleRunnerPlugin(): Plugin[] {
           // warning once per package
           const packageKey = extractPackageKey(id)
           if (!warnedPackages.has(packageKey)) {
-            this.warn(
-              `Found non-optimized CJS dependency in '${this.environment.name}' environment. ` +
-                `It is recommended to add the dependency to 'environments.${this.environment.name}.optimizeDeps.include'.`,
+            debug(
+              `non-optimized CJS dependency in '${this.environment.name}' environment: ${id}`,
             )
             warnedPackages.add(packageKey)
           }
@@ -65,9 +61,10 @@ export function cjsModuleRunnerPlugin(): Plugin[] {
           // TODO: can we use cjs-module-lexer to properly define named exports?
           // for re-exports, we need to eagerly transform dependencies though.
           // https://github.com/nodejs/node/blob/f3adc11e37b8bfaaa026ea85c1cf22e3a0e29ae9/lib/internal/modules/esm/translators.js#L382-L409
-          output.append(
-            `\n;__vite_ssr_exportAll__(module.exports);\nexport default module.exports;\n`,
-          )
+          output.append(`
+;__vite_ssr_exportAll__(module.exports);
+export default module.exports;
+`)
           return {
             code: output.toString(),
             map: output.generateMap({ hires: 'boundary' }),

packages/plugin-rsc/src/plugins/utils.ts

@@ -19,3 +19,9 @@ export function parseIdQuery(id: string): {
   const query = Object.fromEntries(new URLSearchParams(rawQuery))
   return { filename, query }
 }
+
+// https://github.com/vitejs/vite/blob/946831f986cb797009b8178659d2b31f570c44ff/packages/vite/src/shared/utils.ts#L31-L34
+const postfixRE = /[?#].*$/
+export function cleanUrl(url: string): string {
+  return url.replace(postfixRE, '')
+}

packages/plugin-rsc/src/transforms/cjs.test.ts

@@ -1,7 +1,8 @@
-import { parseAstAsync } from 'vite'
+import { createServer, createServerModuleRunner, parseAstAsync } from 'vite'
 import { describe, expect, it } from 'vitest'
 import { debugSourceMap } from './test-utils'
 import { transformCjsToEsm } from './cjs'
+import path from 'node:path'
 
 describe(transformCjsToEsm, () => {
   async function testTransform(input: string) {
@@ -83,4 +84,51 @@ if (true) {
       "
     `)
   })
+
+  it('e2e', async () => {
+    const server = await createServer({
+      configFile: false,
+      logLevel: 'error',
+      root: path.join(import.meta.dirname, 'fixtures/cjs'),
+      plugins: [
+        {
+          name: 'cjs-module-runner-transform',
+          async transform(code, id) {
+            if (id.endsWith('.cjs')) {
+              const ast = await parseAstAsync(code)
+              const { output } = transformCjsToEsm(code, ast)
+              output.append(`
+;__vite_ssr_exportAll__(module.exports);
+export default module.exports;
+`)
+              return {
+                code: output.toString(),
+                map: output.generateMap({ hires: 'boundary' }),
+              }
+            }
+          },
+        },
+      ],
+    })
+    const runner = createServerModuleRunner(server.environments.ssr, {
+      hmr: false,
+    })
+    const mod = await runner.import('/entry.mjs')
+    expect(mod).toMatchInlineSnapshot(`
+      {
+        "depDefault": {
+          "a": "a",
+          "b": "b",
+        },
+        "depNamespace": {
+          "a": "a",
+          "b": "b",
+          "default": {
+            "a": "a",
+            "b": "b",
+          },
+        },
+      }
+    `)
+  })
 })

packages/plugin-rsc/src/transforms/fixtures/cjs/dep1.cjs

@@ -0,0 +1,2 @@
+exports.a = 'a'
+exports.b = 'b'

packages/plugin-rsc/src/transforms/fixtures/cjs/dep2.cjs

@@ -0,0 +1,2 @@
+exports.a = 'a'
+exports.b = 'b'

packages/plugin-rsc/src/transforms/fixtures/cjs/entry.mjs

@@ -0,0 +1,3 @@
+import depDefault from './dep1.cjs'
+import * as depNamespace from './dep2.cjs'
+export { depDefault, depNamespace }