feat(hmr): reload for circular imports only if error (#15118)

vitejs · Jan 8, 2024 · 6ace32b · 6ace32b
1 parent ebc37f6
commit 6ace32b
Show file tree

Hide file tree

Showing 4 changed files with 58 additions and 31 deletions.
diff --git a/packages/vite/src/client/client.ts b/packages/vite/src/client/client.ts
@@ -138,16 +138,27 @@ const hmrClient = new HMRClient(console, async function importUpdatedModule({
   acceptedPath,
   timestamp,
   explicitImportRequired,
+  isWithinCircularImport,
 }) {
   const [acceptedPathWithoutQuery, query] = acceptedPath.split(`?`)
-  return await import(
+  const importPromise = import(
     /* @vite-ignore */
     base +
       acceptedPathWithoutQuery.slice(1) +
       `?${explicitImportRequired ? 'import&' : ''}t=${timestamp}${
         query ? `&${query}` : ''
       }`
   )
+  if (isWithinCircularImport) {
+    importPromise.catch(() => {
+      console.info(
+        `[hmr] ${acceptedPath} failed to apply HMR as it's within a circular import. Reloading page to reset the execution order. ` +
+          `To debug and break the circular import, you can run \`vite --debug hmr\` to log the circular dependency path if a file change triggered it.`,
+      )
+      pageReload()
+    })
+  }
+  return await importPromise
 })
 
 async function handleMessage(payload: HMRPayload) {

diff --git a/packages/vite/src/node/server/hmr.ts b/packages/vite/src/node/server/hmr.ts
@@ -45,6 +45,12 @@ export interface HmrContext {
   server: ViteDevServer
 }
 
+interface PropagationBoundary {
+  boundary: ModuleNode
+  acceptedVia: ModuleNode
+  isWithinCircularImport: boolean
+}
+
 export function getShortName(file: string, root: string): string {
   return file.startsWith(withTrailingSlash(root))
     ? path.posix.relative(root, file)
@@ -143,7 +149,8 @@ export async function handleHMRUpdate(
   updateModules(shortFile, hmrContext.modules, timestamp, server)
 }
 
-type HasDeadEnd = boolean | string
+type HasDeadEnd = boolean
+
 export function updateModules(
   file: string,
   modules: ModuleNode[],
@@ -157,7 +164,7 @@ export function updateModules(
   let needFullReload: HasDeadEnd = false
 
   for (const mod of modules) {
-    const boundaries: { boundary: ModuleNode; acceptedVia: ModuleNode }[] = []
+    const boundaries: PropagationBoundary[] = []
     const hasDeadEnd = propagateUpdate(mod, traversedModules, boundaries)
 
     moduleGraph.invalidateModule(mod, invalidatedModules, timestamp, true)
@@ -172,16 +179,19 @@ export function updateModules(
     }
 
     updates.push(
-      ...boundaries.map(({ boundary, acceptedVia }) => ({
-        type: `${boundary.type}-update` as const,
-        timestamp,
-        path: normalizeHmrUrl(boundary.url),
-        explicitImportRequired:
-          boundary.type === 'js'
-            ? isExplicitImportRequired(acceptedVia.url)
-            : undefined,
-        acceptedPath: normalizeHmrUrl(acceptedVia.url),
-      })),
+      ...boundaries.map(
+        ({ boundary, acceptedVia, isWithinCircularImport }) => ({
+          type: `${boundary.type}-update` as const,
+          timestamp,
+          path: normalizeHmrUrl(boundary.url),
+          acceptedPath: normalizeHmrUrl(acceptedVia.url),
+          explicitImportRequired:
+            boundary.type === 'js'
+              ? isExplicitImportRequired(acceptedVia.url)
+              : false,
+          isWithinCircularImport,
+        }),
+      ),
     )
   }
 
@@ -258,7 +268,7 @@ function areAllImportsAccepted(
 function propagateUpdate(
   node: ModuleNode,
   traversedModules: Set<ModuleNode>,
-  boundaries: { boundary: ModuleNode; acceptedVia: ModuleNode }[],
+  boundaries: PropagationBoundary[],
   currentChain: ModuleNode[] = [node],
 ): HasDeadEnd {
   if (traversedModules.has(node)) {
@@ -279,9 +289,11 @@ function propagateUpdate(
   }
 
   if (node.isSelfAccepting) {
-    boundaries.push({ boundary: node, acceptedVia: node })
-    const result = isNodeWithinCircularImports(node, currentChain)
-    if (result) return result
+    boundaries.push({
+      boundary: node,
+      acceptedVia: node,
+      isWithinCircularImport: isNodeWithinCircularImports(node, currentChain),
+    })
 
     // additionally check for CSS importers, since a PostCSS plugin like
     // Tailwind JIT may register any file as a dependency to a CSS file.
@@ -305,9 +317,11 @@ function propagateUpdate(
   // Also, the imported module (this one) must be updated before the importers,
   // so that they do get the fresh imported module when/if they are reloaded.
   if (node.acceptedHmrExports) {
-    boundaries.push({ boundary: node, acceptedVia: node })
-    const result = isNodeWithinCircularImports(node, currentChain)
-    if (result) return result
+    boundaries.push({
+      boundary: node,
+      acceptedVia: node,
+      isWithinCircularImport: isNodeWithinCircularImports(node, currentChain),
+    })
   } else {
     if (!node.importers.size) {
       return true
@@ -328,9 +342,11 @@ function propagateUpdate(
     const subChain = currentChain.concat(importer)
 
     if (importer.acceptedHmrDeps.has(node)) {
-      boundaries.push({ boundary: importer, acceptedVia: node })
-      const result = isNodeWithinCircularImports(importer, subChain)
-      if (result) return result
+      boundaries.push({
+        boundary: importer,
+        acceptedVia: node,
+        isWithinCircularImport: isNodeWithinCircularImports(importer, subChain),
+      })
       continue
     }
 
@@ -369,7 +385,7 @@ function isNodeWithinCircularImports(
   nodeChain: ModuleNode[],
   currentChain: ModuleNode[] = [node],
   traversedModules = new Set<ModuleNode>(),
-): HasDeadEnd {
+): boolean {
   // To help visualize how each parameters work, imagine this import graph:
   //
   // A -> B -> C -> ACCEPTED -> D -> E -> NODE
@@ -420,7 +436,7 @@ function isNodeWithinCircularImports(
             importChain.map((m) => colors.dim(m.url)).join(' -> '),
         )
       }
-      return 'circular imports'
+      return true
     }
 
     // Continue recursively

diff --git a/packages/vite/types/hmrPayload.d.ts b/packages/vite/types/hmrPayload.d.ts
@@ -20,10 +20,10 @@ export interface Update {
   path: string
   acceptedPath: string
   timestamp: number
-  /**
-   * @experimental internal
-   */
-  explicitImportRequired?: boolean | undefined
+  /** @internal */
+  explicitImportRequired: boolean
+  /** @internal */
+  isWithinCircularImport: boolean
 }
 
 export interface PrunePayload {

diff --git a/playground/hmr/__tests__/hmr.spec.ts b/playground/hmr/__tests__/hmr.spec.ts
@@ -887,9 +887,9 @@ if (import.meta.hot) {
       'cc',
     )
     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('page reload')
-    expect(serverLogs.at(-1)!).toContain('(circular imports)')
+    expect(serverLogs.at(-1)!).toContain('hmr update')
   })
 
   test('hmr should not reload if no accepted within circular imported files', async () => {