fix(ssr): hoist export to handle cyclic import better

packages/vite/src/node/ssr/__tests__/ssrTransform.spec.ts

@@ -56,46 +56,46 @@ test('namespace import', async () => {
 test('export function declaration', async () => {
   expect(await ssrTransformSimpleCode(`export function foo() {}`))
     .toMatchInlineSnapshot(`
-      "function foo() {}
-      Object.defineProperty(__vite_ssr_exports__, "foo", { enumerable: true, configurable: true, get(){ return foo }});"
+      "Object.defineProperty(__vite_ssr_exports__, "foo", { enumerable: true, configurable: true, get(){ return foo }});
+      function foo() {}"
     `)
 })
 
 test('export class declaration', async () => {
   expect(await ssrTransformSimpleCode(`export class foo {}`))
     .toMatchInlineSnapshot(`
-      "class foo {}
-      Object.defineProperty(__vite_ssr_exports__, "foo", { enumerable: true, configurable: true, get(){ return foo }});"
+      "Object.defineProperty(__vite_ssr_exports__, "foo", { enumerable: true, configurable: true, get(){ return foo }});
+      class foo {}"
     `)
 })
 
 test('export var declaration', async () => {
   expect(await ssrTransformSimpleCode(`export const a = 1, b = 2`))
     .toMatchInlineSnapshot(`
-      "const a = 1, b = 2
-      Object.defineProperty(__vite_ssr_exports__, "a", { enumerable: true, configurable: true, get(){ return a }});
-      Object.defineProperty(__vite_ssr_exports__, "b", { enumerable: true, configurable: true, get(){ return b }});"
+      "Object.defineProperty(__vite_ssr_exports__, "a", { enumerable: true, configurable: true, get(){ return a }});
+      Object.defineProperty(__vite_ssr_exports__, "b", { enumerable: true, configurable: true, get(){ return b }});
+      const a = 1, b = 2"
     `)
 })
 
 test('export named', async () => {
   expect(
     await ssrTransformSimpleCode(`const a = 1, b = 2; export { a, b as c }`),
   ).toMatchInlineSnapshot(`
-    "const a = 1, b = 2; 
-    Object.defineProperty(__vite_ssr_exports__, "a", { enumerable: true, configurable: true, get(){ return a }});
-    Object.defineProperty(__vite_ssr_exports__, "c", { enumerable: true, configurable: true, get(){ return b }});"
+    "Object.defineProperty(__vite_ssr_exports__, "a", { enumerable: true, configurable: true, get(){ return a }});
+    Object.defineProperty(__vite_ssr_exports__, "c", { enumerable: true, configurable: true, get(){ return b }});
+    const a = 1, b = 2; "
   `)
 })
 
 test('export named from', async () => {
   expect(
     await ssrTransformSimpleCode(`export { ref, computed as c } from 'vue'`),
   ).toMatchInlineSnapshot(`
-    "const __vite_ssr_import_0__ = await __vite_ssr_import__("vue", {"importedNames":["ref","computed"]});
-
-    Object.defineProperty(__vite_ssr_exports__, "ref", { enumerable: true, configurable: true, get(){ return __vite_ssr_import_0__.ref }});
-    Object.defineProperty(__vite_ssr_exports__, "c", { enumerable: true, configurable: true, get(){ return __vite_ssr_import_0__.computed }});"
+    "Object.defineProperty(__vite_ssr_exports__, "ref", { enumerable: true, configurable: true, get(){ return __vite_ssr_import_0__.ref }});
+    Object.defineProperty(__vite_ssr_exports__, "c", { enumerable: true, configurable: true, get(){ return __vite_ssr_import_0__.computed }});
+    const __vite_ssr_import_0__ = await __vite_ssr_import__("vue", {"importedNames":["ref","computed"]});
+    "
   `)
 })
 
@@ -105,9 +105,9 @@ test('named exports of imported binding', async () => {
       `import {createApp} from 'vue';export {createApp}`,
     ),
   ).toMatchInlineSnapshot(`
-    "const __vite_ssr_import_0__ = await __vite_ssr_import__("vue", {"importedNames":["createApp"]});
-
-    Object.defineProperty(__vite_ssr_exports__, "createApp", { enumerable: true, configurable: true, get(){ return __vite_ssr_import_0__.createApp }});"
+    "Object.defineProperty(__vite_ssr_exports__, "createApp", { enumerable: true, configurable: true, get(){ return __vite_ssr_import_0__.createApp }});
+    const __vite_ssr_import_0__ = await __vite_ssr_import__("vue", {"importedNames":["createApp"]});
+    "
   `)
 })
 
@@ -129,19 +129,19 @@ test('export * from', async () => {
 test('export * as from', async () => {
   expect(await ssrTransformSimpleCode(`export * as foo from 'vue'`))
     .toMatchInlineSnapshot(`
-      "const __vite_ssr_import_0__ = await __vite_ssr_import__("vue");
-
-      Object.defineProperty(__vite_ssr_exports__, "foo", { enumerable: true, configurable: true, get(){ return __vite_ssr_import_0__ }});"
+      "Object.defineProperty(__vite_ssr_exports__, "foo", { enumerable: true, configurable: true, get(){ return __vite_ssr_import_0__ }});
+      const __vite_ssr_import_0__ = await __vite_ssr_import__("vue");
+      "
     `)
 })
 
 test('export * as from arbitrary module namespace identifier', async () => {
   expect(
     await ssrTransformSimpleCode(`export * as "arbitrary string" from 'vue'`),
   ).toMatchInlineSnapshot(`
-    "const __vite_ssr_import_0__ = await __vite_ssr_import__("vue");
-
-    Object.defineProperty(__vite_ssr_exports__, "arbitrary string", { enumerable: true, configurable: true, get(){ return __vite_ssr_import_0__ }});"
+    "Object.defineProperty(__vite_ssr_exports__, "arbitrary string", { enumerable: true, configurable: true, get(){ return __vite_ssr_import_0__ }});
+    const __vite_ssr_import_0__ = await __vite_ssr_import__("vue");
+    "
   `)
 })
 
@@ -151,8 +151,8 @@ test('export as arbitrary module namespace identifier', async () => {
       `const something = "Something";export { something as "arbitrary string" };`,
     ),
   ).toMatchInlineSnapshot(`
-    "const something = "Something";
-    Object.defineProperty(__vite_ssr_exports__, "arbitrary string", { enumerable: true, configurable: true, get(){ return something }});"
+    "Object.defineProperty(__vite_ssr_exports__, "arbitrary string", { enumerable: true, configurable: true, get(){ return something }});
+    const something = "Something";"
   `)
 })
 
@@ -162,9 +162,9 @@ test('export as from arbitrary module namespace identifier', async () => {
       `export { "arbitrary string2" as "arbitrary string" } from 'vue';`,
     ),
   ).toMatchInlineSnapshot(`
-    "const __vite_ssr_import_0__ = await __vite_ssr_import__("vue", {"importedNames":["arbitrary string2"]});
-
-    Object.defineProperty(__vite_ssr_exports__, "arbitrary string", { enumerable: true, configurable: true, get(){ return __vite_ssr_import_0__["arbitrary string2"] }});"
+    "Object.defineProperty(__vite_ssr_exports__, "arbitrary string", { enumerable: true, configurable: true, get(){ return __vite_ssr_import_0__["arbitrary string2"] }});
+    const __vite_ssr_import_0__ = await __vite_ssr_import__("vue", {"importedNames":["arbitrary string2"]});
+    "
   `)
 })
 
@@ -209,8 +209,8 @@ test('dynamic import', async () => {
     `export const i = () => import('./foo')`,
   )
   expect(result?.code).toMatchInlineSnapshot(`
-    "const i = () => __vite_ssr_dynamic_import__('./foo')
-    Object.defineProperty(__vite_ssr_exports__, "i", { enumerable: true, configurable: true, get(){ return i }});"
+    "Object.defineProperty(__vite_ssr_exports__, "i", { enumerable: true, configurable: true, get(){ return i }});
+    const i = () => __vite_ssr_dynamic_import__('./foo')"
   `)
   expect(result?.deps).toEqual([])
   expect(result?.dynamicDeps).toEqual(['./foo'])
@@ -382,11 +382,11 @@ test('should declare variable for imported super class', async () => {
         `export class B extends Foo {}`,
     ),
   ).toMatchInlineSnapshot(`
-    "const __vite_ssr_import_0__ = await __vite_ssr_import__("./dependency", {"importedNames":["Foo"]});
+    "Object.defineProperty(__vite_ssr_exports__, "B", { enumerable: true, configurable: true, get(){ return B }});
+    const __vite_ssr_import_0__ = await __vite_ssr_import__("./dependency", {"importedNames":["Foo"]});
     const Foo = __vite_ssr_import_0__.Foo;
     class A extends Foo {};
     class B extends Foo {}
-    Object.defineProperty(__vite_ssr_exports__, "B", { enumerable: true, configurable: true, get(){ return B }});
     Object.defineProperty(__vite_ssr_exports__, "default", { enumerable: true, configurable: true, value: A });"
   `)
 })
@@ -422,9 +422,9 @@ test('should handle default export variants', async () => {
       `export default class A {}\n` + `export class B extends A {}`,
     ),
   ).toMatchInlineSnapshot(`
-    "class A {};
+    "Object.defineProperty(__vite_ssr_exports__, "B", { enumerable: true, configurable: true, get(){ return B }});
+    class A {};
     class B extends A {}
-    Object.defineProperty(__vite_ssr_exports__, "B", { enumerable: true, configurable: true, get(){ return B }});
     Object.defineProperty(__vite_ssr_exports__, "default", { enumerable: true, configurable: true, value: A });"
   `)
 })
@@ -875,12 +875,12 @@ export function fn1() {
         `,
     ),
   ).toMatchInlineSnapshot(`
-    "
+    "Object.defineProperty(__vite_ssr_exports__, "fn1", { enumerable: true, configurable: true, get(){ return fn1 }});
+    Object.defineProperty(__vite_ssr_exports__, "fn2", { enumerable: true, configurable: true, get(){ return fn2 }});
+
     function fn1() {
+    };function fn2() {
     }
-    Object.defineProperty(__vite_ssr_exports__, "fn1", { enumerable: true, configurable: true, get(){ return fn1 }});;function fn2() {
-    }
-    Object.defineProperty(__vite_ssr_exports__, "fn2", { enumerable: true, configurable: true, get(){ return fn2 }});
             "
   `)
 })
@@ -981,7 +981,8 @@ export class Test {
 };`.trim()
 
   expect(await ssrTransformSimpleCode(code)).toMatchInlineSnapshot(`
-    "const __vite_ssr_import_0__ = await __vite_ssr_import__("foobar", {"importedNames":["foo","bar"]});
+    "Object.defineProperty(__vite_ssr_exports__, "Test", { enumerable: true, configurable: true, get(){ return Test }});
+    const __vite_ssr_import_0__ = await __vite_ssr_import__("foobar", {"importedNames":["foo","bar"]});
 
     if (false) {
       const foo = 'foo';
@@ -1006,8 +1007,7 @@ export class Test {
           console.log((0,__vite_ssr_import_0__.bar))
         }
       }
-    }
-    Object.defineProperty(__vite_ssr_exports__, "Test", { enumerable: true, configurable: true, get(){ return Test }});;;"
+    };;"
   `)
 })
 
@@ -1180,13 +1180,13 @@ export * as bar from './bar'
 console.log(bar)
   `),
   ).toMatchInlineSnapshot(`
-    "const __vite_ssr_import_0__ = await __vite_ssr_import__("./foo", {"importedNames":["foo"]});
+    "Object.defineProperty(__vite_ssr_exports__, "bar", { enumerable: true, configurable: true, get(){ return __vite_ssr_import_1__ }});
+    const __vite_ssr_import_0__ = await __vite_ssr_import__("./foo", {"importedNames":["foo"]});
 
 
     __vite_ssr_exports__.default = (0,__vite_ssr_import_0__.foo)();
     const __vite_ssr_import_1__ = await __vite_ssr_import__("./bar");
-
-    Object.defineProperty(__vite_ssr_exports__, "bar", { enumerable: true, configurable: true, get(){ return __vite_ssr_import_1__ }});;
+    ;
     console.log(bar)
       "
   `)

packages/vite/src/node/ssr/runtime/__tests__/fixtures/cyclic2/README.md

@@ -0,0 +1,9 @@
+Cyclic import example based on https://github.com/vitejs/vite/issues/14048#issuecomment-2354774156
+
+```mermaid
+flowchart TD
+    B(dep1.js) -->|dep1| A(index.js)
+    A -->|dep1| C(dep2.js)
+    C -->|dep2| A
+    A -->|dep1, dep2| entry.js
+```

packages/vite/src/node/ssr/runtime/__tests__/fixtures/cyclic2/package.json

@@ -0,0 +1,3 @@
+{
+  "type": "module"
+}

packages/vite/src/node/ssr/runtime/__tests__/fixtures/cyclic2/test1/dep1.js

@@ -0,0 +1 @@
+export const dep1 = { ok: true };

packages/vite/src/node/ssr/runtime/__tests__/fixtures/cyclic2/test1/dep2.js

@@ -0,0 +1,2 @@
+import { dep1 } from "./index.js"
+export const dep2 = { ok: dep1.ok }

packages/vite/src/node/ssr/runtime/__tests__/fixtures/cyclic2/test1/index.js

@@ -0,0 +1,5 @@
+import { dep1 } from './dep1.js'
+export { dep1 }
+
+import { dep2 } from './dep2.js'
+export { dep2 }

packages/vite/src/node/ssr/runtime/__tests__/fixtures/cyclic2/test2/dep1.js

@@ -0,0 +1 @@
+export const dep1 = { ok: true };

packages/vite/src/node/ssr/runtime/__tests__/fixtures/cyclic2/test2/dep2.js

@@ -0,0 +1,2 @@
+import { dep1 } from "./index.js"
+export const dep2 = { ok: dep1.ok }

packages/vite/src/node/ssr/runtime/__tests__/fixtures/cyclic2/test2/index.js

@@ -0,0 +1,2 @@
+export { dep1 } from './dep1.js'
+export { dep2 } from "./dep2.js"

packages/vite/src/node/ssr/runtime/__tests__/fixtures/cyclic2/test3/dep1.js

@@ -0,0 +1 @@
+export const dep1 = { ok: true };

packages/vite/src/node/ssr/runtime/__tests__/fixtures/cyclic2/test3/dep2.js

@@ -0,0 +1,2 @@
+import { dep1 } from "./index.js"
+export const dep2 = { ok: dep1.ok }

packages/vite/src/node/ssr/runtime/__tests__/fixtures/cyclic2/test3/index.js

@@ -0,0 +1,4 @@
+import { dep1 } from './dep1.js'
+import { dep2 } from './dep2.js'
+export { dep1 }
+export { dep2 }

packages/vite/src/node/ssr/runtime/__tests__/fixtures/cyclic2/test4/dep1.js

@@ -0,0 +1 @@
+export const dep1 = { ok: true };

packages/vite/src/node/ssr/runtime/__tests__/fixtures/cyclic2/test4/dep2.js

@@ -0,0 +1,2 @@
+import { dep1 } from "./index.js"
+export const dep2 = { ok: dep1.ok }

packages/vite/src/node/ssr/runtime/__tests__/fixtures/cyclic2/test4/index.js

@@ -0,0 +1,2 @@
+export * from './dep1.js'
+export * from './dep2.js'

packages/vite/src/node/ssr/runtime/__tests__/fixtures/cyclic2/test5/dep1.js

@@ -0,0 +1 @@
+export const dep1 = { ok: true };

packages/vite/src/node/ssr/runtime/__tests__/fixtures/cyclic2/test5/dep2.js

@@ -0,0 +1,2 @@
+import { dep1 } from "./index.js"
+export const dep2 = { ok: dep1.ok }

packages/vite/src/node/ssr/runtime/__tests__/fixtures/cyclic2/test5/index.js

@@ -0,0 +1,5 @@
+import { dep2 } from './dep2.js'
+export { dep2 }
+
+import { dep1 } from './dep1.js'
+export { dep1 }

packages/vite/src/node/ssr/runtime/__tests__/server-runtime.spec.ts

@@ -232,6 +232,30 @@ describe('module runner initialization', async () => {
     const mod = await runner.import('/fixtures/no-this/importer.js')
     expect(mod.result).toBe(undefined)
   })
+
+  it.for([
+    ['/fixtures/cyclic2/test1/index.js', true],
+    ['/fixtures/cyclic2/test2/index.js', true],
+    ['/fixtures/cyclic2/test3/index.js', true],
+    ['/fixtures/cyclic2/test4/index.js', true],
+    ['/fixtures/cyclic2/test5/index.js', false],
+  ] as const)(`cyclic %s`, async ([entry, ok], { runner }) => {
+    if (ok) {
+      const mod = await runner.import(entry)
+      expect({ ...mod }).toEqual({
+        dep1: {
+          ok: true,
+        },
+        dep2: {
+          ok: true,
+        },
+      })
+    } else {
+      await expect(() => runner.import(entry)).rejects.toMatchObject({
+        name: 'ReferenceError',
+      })
+    }
+  })
 })
 
 describe('optimize-deps', async () => {

packages/vite/src/node/ssr/ssrTransform.ts

@@ -157,11 +157,11 @@ async function ssrTransformScript(
     return importId
   }
 
-  function defineExport(position: number, name: string, local = name) {
+  function defineExport(_position: number, name: string, local = name) {
     s.appendLeft(
-      position,
-      `\nObject.defineProperty(${ssrModuleExportsKey}, ${JSON.stringify(name)}, ` +
-        `{ enumerable: true, configurable: true, get(){ return ${local} }});`,
+      fileStartIndex,
+      `Object.defineProperty(${ssrModuleExportsKey}, ${JSON.stringify(name)}, ` +
+        `{ enumerable: true, configurable: true, get(){ return ${local} }});\n`,
     )
   }
 

playground/hmr-ssr/__tests__/hmr-ssr.spec.ts

@@ -851,13 +851,16 @@ if (!isBuild) {
     editFile('self-accept-within-circular/c.js', (code) =>
       code.replace(`export const c = 'c'`, `export const c = 'cc'`),
     )
+    // it throws a same error as browser case,
+    // but it doesn't auto reload and it calls `hot.accept` called with `undefined`
+    await untilUpdated(() => el(), '')
+
+    // test reloading manually for now
+    // TODO: why clearCache not working?
+    // runner.clearCache();
+    // runner.import('/self-accept-within-circular/index')
+    await setupModuleRunner('/self-accept-within-circular/index')
     await untilUpdated(() => el(), 'cc')
-    await vi.waitFor(() => {
-      expect(serverLogs.length).greaterThanOrEqual(1)
-      // Should still keep hmr update, but it'll error on the browser-side and will refresh itself.
-      // Match on full log not possible because of color markers
-      expect(serverLogs.at(-1)!).toContain('hmr update')
-    })
   })
 
   test('hmr should not reload if no accepted within circular imported files', async (ctx) => {

playground/hmr-ssr/self-accept-within-circular/c.js

@@ -8,5 +8,5 @@ function render(content) {
 render(c)
 
 import.meta.hot?.accept((nextExports) => {
-  render(nextExports.c)
+  render(nextExports?.c)
 })

vitest.config.ts

@@ -14,7 +14,11 @@ export default defineConfig({
       './playground-temp/**/*.*',
     ],
     testTimeout: 20000,
-    isolate: false,
+    // isolate: false,
+    // TODO:
+    // importing non entry file can be broken due to cyclic import e.g.
+    //   pnpm exec tsx packages/vite/src/node/server/index.ts
+    setupFiles: ['./packages/vite/src/node/index.ts'],
   },
   esbuild: {
     target: 'node18',