feat(ssr): tolerate circular imports (#3950)

Co-authored-by: Greg Fairbanks <gregfa@zillowgroup.com>
Co-authored-by: Ray Thurne Void <ray.thurne.void@gmail.com>

packages/playground/ssr-react/__tests__/ssr-react.spec.ts

@@ -1,4 +1,4 @@
-import { editFile, getColor, isBuild, untilUpdated } from '../../testUtils'
+import { editFile, untilUpdated } from '../../testUtils'
 import { port } from './serve'
 import fetch from 'node-fetch'
 
@@ -46,3 +46,10 @@ test('client navigation', async () => {
   )
   await untilUpdated(() => page.textContent('h1'), 'changed')
 })
+
+test(`circular dependecies modules doesn't throw`, async () => {
+  await page.goto(url)
+  expect(await page.textContent('.circ-dep-init')).toMatch(
+    'circ-dep-init-a circ-dep-init-b'
+  )
+})

packages/playground/ssr-react/src/add.js

@@ -0,0 +1,9 @@
+import { multiply } from './multiply'
+
+export function add(a, b) {
+  return a + b
+}
+
+export function addAndMultiply(a, b, c) {
+  return multiply(add(a, b), c)
+}

packages/playground/ssr-react/src/circular-dep-init/README.md

@@ -0,0 +1 @@
+This test aim to find out wherever the modules with circular dependencies are correctly initialized

packages/playground/ssr-react/src/circular-dep-init/circular-dep-init.js

@@ -0,0 +1,2 @@
+export * from './module-a'
+export { getValueAB } from './module-b'

packages/playground/ssr-react/src/circular-dep-init/module-a.js

@@ -0,0 +1 @@
+export const valueA = 'circ-dep-init-a'

packages/playground/ssr-react/src/circular-dep-init/module-b.js

@@ -0,0 +1,8 @@
+import { valueA } from './circular-dep-init'
+
+export const valueB = 'circ-dep-init-b'
+export const valueAB = valueA.concat(` ${valueB}`)
+
+export function getValueAB() {
+  return valueAB
+}

packages/playground/ssr-react/src/forked-deadlock/README.md

@@ -0,0 +1,45 @@
+This test aim to check for a particular type of circular dependency that causes tricky deadlocks, **deadlocks with forked imports stack**
+
+```
+A -> B means: B is imported by A and B has A in its stack
+A ... B means: A is waiting for B to ssrLoadModule()
+
+H -> X ... Y
+H -> X -> Y ... B
+H -> A ... B
+H -> A -> B ... X
+```
+
+### Forked deadlock description:
+```
+[X] is waiting for [Y] to resolve
+ ↑                  ↳ is waiting for [A] to resolve
+ │                                    ↳ is waiting for [B] to resolve
+ │                                                      ↳ is waiting for [X] to resolve
+ └────────────────────────────────────────────────────────────────────────┘
+```
+
+This may seems a traditional deadlock, but the thing that makes this special is the import stack of each module:
+```
+[X] stack:
+	[H]
+```
+```
+[Y] stack:
+	[X]
+	[H]
+```
+```
+[A] stack:
+	[H]
+```
+```
+[B] stack:
+	[A]
+	[H]
+```
+Even if `[X]` is imported by `[B]`, `[B]` is not in `[X]`'s stack because it's imported by `[H]` in first place then it's stack is only composed by `[H]`. `[H]` **forks** the imports **stack** and this make hard to be found.
+
+### Fix description
+Vite, when imports `[X]`, should check whether `[X]` is already pending and if it is, it must check that, when it was imported in first place, the stack of `[X]` doesn't have any module in common with the current module; in this case `[B]` has the module `[H]` is common with `[X]` and i can assume that a deadlock is going to happen.
+

packages/playground/ssr-react/src/forked-deadlock/common-module.js

@@ -0,0 +1,10 @@
+import { stuckModuleExport } from './stuck-module'
+import { deadlockfuseModuleExport } from './deadlock-fuse-module'
+
+/**
+ * module H
+ */
+export function commonModuleExport() {
+  stuckModuleExport()
+  deadlockfuseModuleExport()
+}

packages/playground/ssr-react/src/forked-deadlock/deadlock-fuse-module.js

@@ -0,0 +1,8 @@
+import { fuseStuckBridgeModuleExport } from './fuse-stuck-bridge-module'
+
+/**
+ * module A
+ */
+export function deadlockfuseModuleExport() {
+  fuseStuckBridgeModuleExport()
+}

packages/playground/ssr-react/src/forked-deadlock/fuse-stuck-bridge-module.js

@@ -0,0 +1,8 @@
+import { stuckModuleExport } from './stuck-module'
+
+/**
+ * module C
+ */
+export function fuseStuckBridgeModuleExport() {
+  stuckModuleExport()
+}

packages/playground/ssr-react/src/forked-deadlock/middle-module.js

@@ -0,0 +1,8 @@
+import { deadlockfuseModuleExport } from './deadlock-fuse-module'
+
+/**
+ * module Y
+ */
+export function middleModuleExport() {
+  void deadlockfuseModuleExport
+}

packages/playground/ssr-react/src/forked-deadlock/stuck-module.js

@@ -0,0 +1,8 @@
+import { middleModuleExport } from './middle-module'
+
+/**
+ * module X
+ */
+export function stuckModuleExport() {
+  middleModuleExport()
+}

packages/playground/ssr-react/src/multiply.js

@@ -0,0 +1,9 @@
+import { add } from './add'
+
+export function multiply(a, b) {
+  return a * b
+}
+
+export function multiplyAndAdd(a, b, c) {
+  return add(multiply(a, b), c)
+}

packages/playground/ssr-react/src/pages/About.jsx

@@ -1,3 +1,12 @@
+import { addAndMultiply } from '../add'
+import { multiplyAndAdd } from '../multiply'
+
 export default function About() {
-  return <h1>About</h1>
+  return (
+    <>
+      <h1>About</h1>
+      <div>{addAndMultiply(1, 2, 3)}</div>
+      <div>{multiplyAndAdd(1, 2, 3)}</div>
+    </>
+  )
 }

packages/playground/ssr-react/src/pages/Home.jsx

@@ -1,3 +1,17 @@
+import { addAndMultiply } from '../add'
+import { multiplyAndAdd } from '../multiply'
+import { commonModuleExport } from '../forked-deadlock/common-module'
+import { getValueAB } from '../circular-dep-init/circular-dep-init'
+
 export default function Home() {
-  return <h1>Home</h1>
+  commonModuleExport()
+
+  return (
+    <>
+      <h1>Home</h1>
+      <div>{addAndMultiply(1, 2, 3)}</div>
+      <div>{multiplyAndAdd(1, 2, 3)}</div>
+      <div className="circ-dep-init">{getValueAB()}</div>
+    </>
+  )
 }

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

@@ -11,7 +11,7 @@ test('default import', async () => {
       )
     ).code
   ).toMatchInlineSnapshot(`
-    "const __vite_ssr_import_0__ = __vite_ssr_import__(\\"vue\\")
+    "const __vite_ssr_import_0__ = await __vite_ssr_import__(\\"vue\\");
     console.log(__vite_ssr_import_0__.default.bar)"
   `)
 })
@@ -26,7 +26,7 @@ test('named import', async () => {
       )
     ).code
   ).toMatchInlineSnapshot(`
-    "const __vite_ssr_import_0__ = __vite_ssr_import__(\\"vue\\")
+    "const __vite_ssr_import_0__ = await __vite_ssr_import__(\\"vue\\");
     function foo() { return __vite_ssr_import_0__.ref(0) }"
   `)
 })
@@ -41,7 +41,7 @@ test('namespace import', async () => {
       )
     ).code
   ).toMatchInlineSnapshot(`
-    "const __vite_ssr_import_0__ = __vite_ssr_import__(\\"vue\\")
+    "const __vite_ssr_import_0__ = await __vite_ssr_import__(\\"vue\\");
     function foo() { return __vite_ssr_import_0__.ref(0) }"
   `)
 })
@@ -50,24 +50,24 @@ test('export function declaration', async () => {
   expect((await ssrTransform(`export function foo() {}`, null, null)).code)
     .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 }});"
   `)
 })
 
 test('export class declaration', async () => {
   expect((await ssrTransform(`export class foo {}`, null, null)).code)
     .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 }});"
   `)
 })
 
 test('export var declaration', async () => {
   expect((await ssrTransform(`export const a = 1, b = 2`, null, null)).code)
     .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 }});"
   `)
 })
 
@@ -77,8 +77,8 @@ test('export named', async () => {
       .code
   ).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 }});"
   `)
 })
 
@@ -87,10 +87,10 @@ test('export named from', async () => {
     (await ssrTransform(`export { ref, computed as c } from 'vue'`, null, null))
       .code
   ).toMatchInlineSnapshot(`
-    "const __vite_ssr_import_0__ = __vite_ssr_import__(\\"vue\\")
+    "const __vite_ssr_import_0__ = await __vite_ssr_import__(\\"vue\\");
 
-    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 }});"
   `)
 })
 
@@ -104,27 +104,35 @@ test('named exports of imported binding', async () => {
       )
     ).code
   ).toMatchInlineSnapshot(`
-    "const __vite_ssr_import_0__ = __vite_ssr_import__(\\"vue\\")
+    "const __vite_ssr_import_0__ = await __vite_ssr_import__(\\"vue\\");
 
-    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 }});"
   `)
 })
 
 test('export * from', async () => {
-  expect((await ssrTransform(`export * from 'vue'`, null, null)).code)
-    .toMatchInlineSnapshot(`
-    "const __vite_ssr_import_0__ = __vite_ssr_import__(\\"vue\\")
-
-    __vite_ssr_exportAll__(__vite_ssr_import_0__)"
+  expect(
+    (
+      await ssrTransform(
+        `export * from 'vue'\n` + `export * from 'react'`,
+        null,
+        null
+      )
+    ).code
+  ).toMatchInlineSnapshot(`
+    "const __vite_ssr_import_0__ = await __vite_ssr_import__(\\"vue\\");
+    __vite_ssr_exportAll__(__vite_ssr_import_0__);
+    const __vite_ssr_import_1__ = await __vite_ssr_import__(\\"react\\");
+    __vite_ssr_exportAll__(__vite_ssr_import_1__);"
   `)
 })
 
 test('export * as from', async () => {
   expect((await ssrTransform(`export * as foo from 'vue'`, null, null)).code)
     .toMatchInlineSnapshot(`
-    "const __vite_ssr_import_0__ = __vite_ssr_import__(\\"vue\\")
+    "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__ }});"
   `)
 })
 
@@ -146,7 +154,7 @@ test('dynamic import', async () => {
       .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 }});"
   `)
 })
 
@@ -160,7 +168,7 @@ test('do not rewrite method definition', async () => {
       )
     ).code
   ).toMatchInlineSnapshot(`
-    "const __vite_ssr_import_0__ = __vite_ssr_import__(\\"vue\\")
+    "const __vite_ssr_import_0__ = await __vite_ssr_import__(\\"vue\\");
     class A { fn() { __vite_ssr_import_0__.fn() } }"
   `)
 })
@@ -175,7 +183,7 @@ test('do not rewrite catch clause', async () => {
       )
     ).code
   ).toMatchInlineSnapshot(`
-    "const __vite_ssr_import_0__ = __vite_ssr_import__(\\"./dependency\\")
+    "const __vite_ssr_import_0__ = await __vite_ssr_import__(\\"./dependency\\");
     try {} catch(error) {}"
   `)
 })
@@ -191,7 +199,7 @@ test('should declare variable for imported super class', async () => {
       )
     ).code
   ).toMatchInlineSnapshot(`
-    "const __vite_ssr_import_0__ = __vite_ssr_import__(\\"./dependency\\")
+    "const __vite_ssr_import_0__ = await __vite_ssr_import__(\\"./dependency\\");
     const Foo = __vite_ssr_import_0__.Foo;
     class A extends Foo {}"
   `)
@@ -209,12 +217,12 @@ test('should declare variable for imported super class', async () => {
       )
     ).code
   ).toMatchInlineSnapshot(`
-    "const __vite_ssr_import_0__ = __vite_ssr_import__(\\"./dependency\\")
+    "const __vite_ssr_import_0__ = await __vite_ssr_import__(\\"./dependency\\");
     const Foo = __vite_ssr_import_0__.Foo;
     class A extends Foo {}
     class B extends Foo {}
-    Object.defineProperty(__vite_ssr_exports__, \\"default\\", { enumerable: true, value: A })
-    Object.defineProperty(__vite_ssr_exports__, \\"B\\", { enumerable: true, configurable: true, get(){ return B }})"
+    Object.defineProperty(__vite_ssr_exports__, \\"default\\", { enumerable: true, value: A });
+    Object.defineProperty(__vite_ssr_exports__, \\"B\\", { enumerable: true, configurable: true, get(){ return B }});"
   `)
 })
 
@@ -246,7 +254,7 @@ test('should handle default export variants', async () => {
   ).toMatchInlineSnapshot(`
     "function foo() {}
     foo.prototype = Object.prototype;
-    Object.defineProperty(__vite_ssr_exports__, \\"default\\", { enumerable: true, value: foo })"
+    Object.defineProperty(__vite_ssr_exports__, \\"default\\", { enumerable: true, value: foo });"
   `)
   // default named classes
   expect(
@@ -260,8 +268,8 @@ test('should handle default export variants', async () => {
   ).toMatchInlineSnapshot(`
     "class A {}
     class B extends A {}
-    Object.defineProperty(__vite_ssr_exports__, \\"default\\", { enumerable: true, value: A })
-    Object.defineProperty(__vite_ssr_exports__, \\"B\\", { enumerable: true, configurable: true, get(){ return B }})"
+    Object.defineProperty(__vite_ssr_exports__, \\"default\\", { enumerable: true, value: A });
+    Object.defineProperty(__vite_ssr_exports__, \\"B\\", { enumerable: true, configurable: true, get(){ return B }});"
   `)
 })
 
@@ -288,7 +296,7 @@ test('overwrite bindings', async () => {
       )
     ).code
   ).toMatchInlineSnapshot(`
-    "const __vite_ssr_import_0__ = __vite_ssr_import__(\\"vue\\")
+    "const __vite_ssr_import_0__ = await __vite_ssr_import__(\\"vue\\");
     const a = { inject: __vite_ssr_import_0__.inject }
     const b = { test: __vite_ssr_import_0__.inject }
     function c() { const { test: inject } = { test: true }; console.log(inject) }