feat(hmr): improve circular import updates (#14867)

docs/guide/troubleshooting.md

@@ -147,13 +147,9 @@ If you are running Vite with WSL2, Vite cannot watch file changes in some condit
 
 ### A full reload happens instead of HMR
 
-If HMR is not handled by Vite or a plugin, a full reload will happen.
+If HMR is not handled by Vite or a plugin, a full reload will happen as it's the only way to refresh the state.
 
-Also if there is a dependency loop, a full reload will happen. To solve this, try removing the loop.
-
-### High number of HMR updates in console
-
-This can be caused by a circular dependency. To solve this, try breaking the loop.
+If HMR is handled but it is within a circular dependency, a full reload will also happen to recover the execution order. To solve this, try breaking the loop. You can run `vite --debug hmr` to log the circular dependency path if a file change triggered it.
 
 ## Build
 

packages/vite/src/node/server/hmr.ts

@@ -158,13 +158,7 @@ export function updateModules(
     const boundaries: { boundary: ModuleNode; acceptedVia: ModuleNode }[] = []
     const hasDeadEnd = propagateUpdate(mod, traversedModules, boundaries)
 
-    moduleGraph.invalidateModule(
-      mod,
-      invalidatedModules,
-      timestamp,
-      true,
-      boundaries.map((b) => b.boundary),
-    )
+    moduleGraph.invalidateModule(mod, invalidatedModules, timestamp, true)
 
     if (needFullReload) {
       continue
@@ -280,6 +274,9 @@ function propagateUpdate(
 
   if (node.isSelfAccepting) {
     boundaries.push({ boundary: node, acceptedVia: node })
+    if (isNodeWithinCircularImports(node, currentChain)) {
+      return true
+    }
 
     // additionally check for CSS importers, since a PostCSS plugin like
     // Tailwind JIT may register any file as a dependency to a CSS file.
@@ -304,6 +301,9 @@ function propagateUpdate(
   // so that they do get the fresh imported module when/if they are reloaded.
   if (node.acceptedHmrExports) {
     boundaries.push({ boundary: node, acceptedVia: node })
+    if (isNodeWithinCircularImports(node, currentChain)) {
+      return true
+    }
   } else {
     if (!node.importers.size) {
       return true
@@ -322,8 +322,12 @@ function propagateUpdate(
 
   for (const importer of node.importers) {
     const subChain = currentChain.concat(importer)
+
     if (importer.acceptedHmrDeps.has(node)) {
       boundaries.push({ boundary: importer, acceptedVia: node })
+      if (isNodeWithinCircularImports(importer, subChain)) {
+        return true
+      }
       continue
     }
 
@@ -337,12 +341,82 @@ function propagateUpdate(
       }
     }
 
-    if (currentChain.includes(importer)) {
-      // circular deps is considered dead end
+    if (
+      !currentChain.includes(importer) &&
+      propagateUpdate(importer, traversedModules, boundaries, subChain)
+    ) {
+      return true
+    }
+  }
+  return false
+}
+
+/**
+ * Check importers recursively if it's an import loop. An accepted module within
+ * an import loop cannot recover its execution order and should be reloaded.
+ *
+ * @param node The node that accepts HMR and is a boundary
+ * @param nodeChain The chain of nodes/imports that lead to the node.
+ *   (The last node in the chain imports the `node` parameter)
+ * @param currentChain The current chain tracked from the `node` parameter
+ */
+function isNodeWithinCircularImports(
+  node: ModuleNode,
+  nodeChain: ModuleNode[],
+  currentChain: ModuleNode[] = [node],
+) {
+  // To help visualize how each parameters work, imagine this import graph:
+  //
+  // A -> B -> C -> ACCEPTED -> D -> E -> NODE
+  //      ^--------------------------|
+  //
+  // ACCEPTED: the node that accepts HMR. the `node` parameter.
+  // NODE    : the initial node that triggered this HMR.
+  //
+  // This function will return true in the above graph, which:
+  // `node`         : ACCEPTED
+  // `nodeChain`    : [NODE, E, D, ACCEPTED]
+  // `currentChain` : [ACCEPTED, C, B]
+  //
+  // It works by checking if any `node` importers are within `nodeChain`, which
+  // means there's an import loop with a HMR-accepted module in it.
+
+  for (const importer of node.importers) {
+    // Node may import itself which is safe
+    if (importer === node) continue
+
+    // Check circular imports
+    const importerIndex = nodeChain.indexOf(importer)
+    if (importerIndex > -1) {
+      // Log extra debug information so users can fix and remove the circular imports
+      if (debugHmr) {
+        // Following explanation above:
+        // `importer`                    : E
+        // `currentChain` reversed       : [B, C, ACCEPTED]
+        // `nodeChain` sliced & reversed : [D, E]
+        // Combined                      : [E, B, C, ACCEPTED, D, E]
+        const importChain = [
+          importer,
+          ...[...currentChain].reverse(),
+          ...nodeChain.slice(importerIndex, -1).reverse(),
+        ]
+        debugHmr(
+          colors.yellow(`circular imports detected: `) +
+            importChain.map((m) => colors.dim(m.url)).join(' -> '),
+        )
+      }
       return true
     }
 
-    if (propagateUpdate(importer, traversedModules, boundaries, subChain)) {
+    // Continue recursively
+    if (
+      !currentChain.includes(importer) &&
+      isNodeWithinCircularImports(
+        importer,
+        nodeChain,
+        currentChain.concat(importer),
+      )
+    ) {
       return true
     }
   }

packages/vite/src/node/server/moduleGraph.ts

@@ -153,8 +153,6 @@ export class ModuleGraph {
     timestamp: number = Date.now(),
     isHmr: boolean = false,
     /** @internal */
-    hmrBoundaries: ModuleNode[] = [],
-    /** @internal */
     softInvalidate = false,
   ): void {
     const prevInvalidationState = mod.invalidationState
@@ -199,14 +197,6 @@ export class ModuleGraph {
     mod.ssrModule = null
     mod.ssrError = null
 
-    // https://github.com/vitejs/vite/issues/3033
-    // Given b.js -> c.js -> b.js (arrow means top-level import), if c.js self-accepts
-    // and refetches itself, the execution order becomes c.js -> b.js -> c.js. The import
-    // order matters here as it will fail. The workaround for now is to not hmr invalidate
-    // b.js so that c.js refetches the already cached b.js, skipping the import loop.
-    if (hmrBoundaries.includes(mod)) {
-      return
-    }
     mod.importers.forEach((importer) => {
       if (!importer.acceptedHmrDeps.has(mod)) {
         // If the importer statically imports the current module, we can soft-invalidate the importer
@@ -220,7 +210,6 @@ export class ModuleGraph {
           seen,
           timestamp,
           isHmr,
-          undefined,
           shouldSoftInvalidateImporter,
         )
       }

playground/hmr/__tests__/hmr.spec.ts

@@ -877,7 +877,25 @@ if (import.meta.hot) {
     editFile('self-accept-within-circular/c.js', (code) =>
       code.replace(`export const c = 'c'`, `export const c = 'cc'`),
     )
-    await untilUpdated(() => el.textContent(), 'cc')
+    await untilUpdated(
+      () => page.textContent('.self-accept-within-circular'),
+      'cc',
+    )
+  })
+
+  test('hmr should not reload if no accepted within circular imported files', async () => {
+    await page.goto(viteTestUrl + '/circular/index.html')
+    const el = await page.$('.circular')
+    expect(await el.textContent()).toBe(
+      'mod-a -> mod-b -> mod-c -> mod-a (expected error)',
+    )
+    editFile('circular/mod-b.js', (code) =>
+      code.replace(`mod-b ->`, `mod-b (edited) ->`),
+    )
+    await untilUpdated(
+      () => el.textContent(),
+      'mod-a -> mod-b (edited) -> mod-c -> mod-a (expected error)',
+    )
   })
 
   test('assets HMR', async () => {

playground/hmr/circular/index.js

@@ -0,0 +1,7 @@
+import { msg } from './mod-a'
+
+document.querySelector('.circular').textContent = msg
+
+if (import.meta.hot) {
+  import.meta.hot.accept()
+}

playground/hmr/circular/mod-a.js

@@ -0,0 +1,5 @@
+export const value = 'mod-a'
+
+import { value as _value } from './mod-b'
+
+export const msg = `mod-a -> ${_value}`

playground/hmr/circular/mod-b.js

@@ -0,0 +1,3 @@
+import { value as _value } from './mod-c'
+
+export const value = `mod-b -> ${_value}`

playground/hmr/circular/mod-c.js

@@ -0,0 +1,11 @@
+import { value as _value } from './mod-a'
+
+// Should error as `_value` is not defined yet within the circular imports
+let __value
+try {
+  __value = `${_value} (unexpected no error)`
+} catch {
+  __value = 'mod-a (expected error)'
+}
+
+export const value = `mod-c -> ${__value}`

playground/hmr/hmr.ts

@@ -5,6 +5,7 @@ import './invalidation/parent'
 import './file-delete-restore'
 import './optional-chaining/parent'
 import './intermediate-file-delete'
+import './circular'
 import logo from './logo.svg'
 import { msg as softInvalidationMsg } from './soft-invalidation'
 

playground/hmr/index.html

@@ -37,3 +37,4 @@
 <button class="intermediate-file-delete-increment">1</button>
 <div class="intermediate-file-delete-display"></div>
 <image id="logo"></image>
+<div class="circular"></div>