feat(hmr): check boundary outside the circular imports

packages/vite/src/client/client.ts

@@ -147,51 +147,26 @@ const hmrClient = new HMRClient(
   },
   transport,
   isBundleMode
-    ? async function importUpdatedModule({
-        url,
-        acceptedPath,
-        isWithinCircularImport,
-      }) {
-        const importPromise = import(base + url!).then(() =>
+    ? async function importUpdatedModule({ url, acceptedPath }) {
+        return await import(base + url!).then(() =>
           // @ts-expect-error globalThis.__rolldown_runtime__
           globalThis.__rolldown_runtime__.loadExports(acceptedPath),
         )
-        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 importUpdatedModule({
         acceptedPath,
         timestamp,
         explicitImportRequired,
-        isWithinCircularImport,
       }) {
         const [acceptedPathWithoutQuery, query] = acceptedPath.split(`?`)
-        const importPromise = import(
+        return await 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
       },
 )
 transport.connect!(createHMRHandler(handleMessage))

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

@@ -71,7 +71,6 @@ export interface HmrContext {
 interface PropagationBoundary {
   boundary: EnvironmentModuleNode & { type: 'js' | 'css' }
   acceptedVia: EnvironmentModuleNode
-  isWithinCircularImport: boolean
 }
 
 export interface HotChannelClient {
@@ -643,9 +642,15 @@ export function updateModules(
   // Modules could be empty if a root module is invalidated via import.meta.hot.invalidate()
   let needFullReload: HasDeadEnd = modules.length === 0
 
+  let hasSkippedBoundary = false
+
   for (const mod of modules) {
     const boundaries: PropagationBoundary[] = []
-    const hasDeadEnd = propagateUpdate(mod, traversedModules, boundaries)
+    const [hasDeadEnd, skippedBoundary] = propagateUpdate(
+      mod,
+      traversedModules,
+      boundaries,
+    )
 
     environment.moduleGraph.invalidateModule(
       mod,
@@ -658,42 +663,49 @@ export function updateModules(
       continue
     }
 
+    hasSkippedBoundary ||= skippedBoundary
     if (hasDeadEnd) {
       needFullReload = hasDeadEnd
       continue
     }
 
-    // If import.meta.hot.invalidate was called already on that module for the same update,
-    // it means any importer of that module can't hot update. We should fallback to full reload.
-    if (
-      firstInvalidatedBy &&
-      boundaries.some(
-        ({ acceptedVia }) =>
-          normalizeHmrUrl(acceptedVia.url) === firstInvalidatedBy,
-      )
-    ) {
-      needFullReload = 'circular import invalidate'
-      continue
-    }
-
     updates.push(
-      ...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,
-          firstInvalidatedBy,
-        }),
-      ),
+      ...boundaries.map(({ boundary, acceptedVia }) => ({
+        type: `${boundary.type}-update` as const,
+        timestamp,
+        path: normalizeHmrUrl(boundary.url),
+        acceptedPath: normalizeHmrUrl(acceptedVia.url),
+        explicitImportRequired:
+          boundary.type === 'js'
+            ? isExplicitImportRequired(acceptedVia.url)
+            : false,
+        firstInvalidatedBy,
+      })),
     )
   }
 
+  // Warn when circular imports cause HMR boundaries to be skipped
+  if (hasSkippedBoundary) {
+    if (needFullReload) {
+      environment.logger.warn(
+        colors.yellow(
+          `hmr boundary skipped due to circular imports, page will reload. ` +
+            `To debug and break the circular import, use ${colors.dim('`vite --debug hmr`')} to log the circular import path.`,
+        ),
+        { timestamp: true },
+      )
+    } else if (traversedModules.size > 10) {
+      environment.logger.warn(
+        colors.yellow(
+          `hmr boundary skipped due to circular imports, ` +
+            `${traversedModules.size} modules will re-execute. ` +
+            `To debug and break the circular import, use ${colors.dim('`vite --debug hmr`')} to log the circular import path.`,
+        ),
+        { timestamp: true },
+      )
+    }
+  }
+
   // html file cannot be hot updated because it may be used as the template for a top-level request response.
   const isClientHtmlChange =
     file.endsWith('.html') &&
@@ -758,9 +770,9 @@ function propagateUpdate(
   traversedModules: Set<EnvironmentModuleNode>,
   boundaries: PropagationBoundary[],
   currentChain: EnvironmentModuleNode[] = [node],
-): HasDeadEnd {
+): [HasDeadEnd, boundarySkipped: boolean] {
   if (traversedModules.has(node)) {
-    return false
+    return [false, false]
   }
   traversedModules.add(node)
 
@@ -773,53 +785,58 @@ function propagateUpdate(
         node.id,
       )}`,
     )
-    return false
+    return [false, false]
   }
 
+  const isWithinCircularImports = isNodeWithinCircularImports(
+    node,
+    currentChain,
+  )
+
+  let boundarySkipped = false
   if (node.isSelfAccepting) {
-    // isSelfAccepting is only true for js and css
-    const boundary = node as EnvironmentModuleNode & { type: 'js' | 'css' }
-    boundaries.push({
-      boundary,
-      acceptedVia: boundary,
-      isWithinCircularImport: isNodeWithinCircularImports(node, currentChain),
-    })
-    return false
+    if (!isWithinCircularImports) {
+      // isSelfAccepting is only true for js and css
+      const boundary = node as EnvironmentModuleNode & { type: 'js' | 'css' }
+      boundaries.push({ boundary, acceptedVia: boundary })
+      return [false, false]
+    } else {
+      boundarySkipped = true
+    }
   }
 
   // A partially accepted module with no importers is considered self accepting,
   // because the deal is "there are parts of myself I can't self accept if they
   // are used outside of me".
   // 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) {
+  if (node.acceptedHmrExports && !isWithinCircularImports) {
     // acceptedHmrExports is only true for js and css
     const boundary = node as EnvironmentModuleNode & { type: 'js' | 'css' }
-    boundaries.push({
-      boundary,
-      acceptedVia: boundary,
-      isWithinCircularImport: isNodeWithinCircularImports(node, currentChain),
-    })
+    boundaries.push({ boundary, acceptedVia: boundary })
   } else {
+    if (node.acceptedHmrExports && isWithinCircularImports) {
+      boundarySkipped = true
+    }
     if (!node.importers.size) {
-      return true
+      return [true, boundarySkipped]
     }
   }
 
   for (const importer of node.importers) {
     const subChain = currentChain.concat(importer)
 
     if (importer.acceptedHmrDeps.has(node)) {
-      // acceptedHmrDeps has value only for js and css
-      const boundary = importer as EnvironmentModuleNode & {
-        type: 'js' | 'css'
+      if (!isNodeWithinCircularImports(importer, subChain)) {
+        // acceptedHmrDeps has value only for js and css
+        const boundary = importer as EnvironmentModuleNode & {
+          type: 'js' | 'css'
+        }
+        boundaries.push({ boundary, acceptedVia: node })
+        continue
+      } else {
+        boundarySkipped = true
       }
-      boundaries.push({
-        boundary,
-        acceptedVia: node,
-        isWithinCircularImport: isNodeWithinCircularImports(importer, subChain),
-      })
-      continue
     }
 
     if (node.id && node.acceptedHmrExports && importer.importedBindings) {
@@ -832,14 +849,20 @@ function propagateUpdate(
       }
     }
 
-    if (
-      !currentChain.includes(importer) &&
-      propagateUpdate(importer, traversedModules, boundaries, subChain)
-    ) {
-      return true
+    if (!currentChain.includes(importer)) {
+      const [hasDeadEnd, skipped] = propagateUpdate(
+        importer,
+        traversedModules,
+        boundaries,
+        subChain,
+      )
+      boundarySkipped ||= skipped
+      if (hasDeadEnd) {
+        return [true, boundarySkipped]
+      }
     }
   }
-  return false
+  return [false, boundarySkipped]
 }
 
 /**

packages/vite/types/hmrPayload.d.ts

@@ -36,8 +36,6 @@ export interface Update {
   /** @internal */
   explicitImportRequired?: boolean
   /** @internal */
-  isWithinCircularImport?: boolean
-  /** @internal */
   firstInvalidatedBy?: string
   /** @internal */
   invalidates?: string[]

playground/hmr/__tests__/hmr.spec.ts

@@ -268,7 +268,7 @@ if (!isBuild) {
       'invalidation-circular-deps/circular-invalidate/child.js',
       (code) => code.replace('child', 'child updated'),
     )
-    await page.waitForEvent('load')
+    // hmr.ts which is outside the circular chain applies the update
     await expect
       .poll(() => page.textContent('.invalidation-circular-deps'))
       .toMatch('child updated')
@@ -1060,8 +1060,7 @@ if (!isBuild) {
       .poll(() => page.textContent('.self-accept-within-circular'))
       .toBe('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
+    // The boundary within the circular chain (c.js) is skipped, but the outer boundary (a.js) handles it
     expect(serverLogs.at(-1)!).toContain('hmr update')
   })
 
@@ -1079,6 +1078,50 @@ if (!isBuild) {
       .toBe('mod-a -> mod-b (edited) -> mod-c -> mod-a (expected error)')
   })
 
+  test('hmr warns when boundary is skipped due to circular imports and page reloads', async () => {
+    await page.goto(viteTestUrl + '/circular-boundary-no-outer/index.html')
+    const el = await page.$('.circular-boundary-no-outer')
+    await expect.poll(() => el.textContent()).toBe('a:b')
+    serverLogs.length = 0
+    editFile('circular-boundary-no-outer/a.js', (code) =>
+      code.replace(`export const a = 'a:'`, `export const a = 'a2:'`),
+    )
+    await expect
+      .poll(() => page.textContent('.circular-boundary-no-outer'))
+      .toBe('a2:b')
+    await expect
+      .poll(() => serverLogs)
+      .toStrictEqual(
+        expect.arrayContaining([
+          expect.stringMatching(
+            /hmr boundary skipped due to circular imports.*page will reload/,
+          ),
+        ]),
+      )
+  })
+
+  test('hmr warns when boundary is skipped due to circular imports and many modules re-execute', async () => {
+    await page.goto(viteTestUrl + '/circular-boundary-many-modules/index.html')
+    const el = await page.$('.circular-boundary-many-modules')
+    await expect.poll(() => el.textContent()).toBe('a:b')
+    serverLogs.length = 0
+    editFile('circular-boundary-many-modules/a.js', (code) =>
+      code.replace(`export const value = 'a:'`, `export const value = 'a2:'`),
+    )
+    await expect
+      .poll(() => page.textContent('.circular-boundary-many-modules'))
+      .toBe('a2:b')
+    await expect
+      .poll(() => serverLogs)
+      .toStrictEqual(
+        expect.arrayContaining([
+          expect.stringMatching(
+            /hmr boundary skipped due to circular imports.*modules will re-execute/,
+          ),
+        ]),
+      )
+  })
+
   test('not inlined assets HMR', async () => {
     await page.goto(viteTestUrl)
     const el = await page.$('#logo-no-inline')

playground/hmr/circular-boundary-many-modules/a.js

@@ -0,0 +1,7 @@
+import { b } from './b'
+
+export const value = 'a:' + b
+
+if (import.meta.hot) {
+  import.meta.hot.accept()
+}

playground/hmr/circular-boundary-many-modules/b.js

@@ -0,0 +1,3 @@
+import './a'
+
+export const b = 'b'

playground/hmr/circular-boundary-many-modules/index.html

@@ -0,0 +1,2 @@
+<script type="module" src="index.js"></script>
+<div class="circular-boundary-many-modules"></div>

playground/hmr/circular-boundary-many-modules/index.js

@@ -0,0 +1,7 @@
+import { value } from './outer'
+
+document.querySelector('.circular-boundary-many-modules').textContent = value
+
+if (import.meta.hot) {
+  import.meta.hot.accept()
+}

playground/hmr/circular-boundary-many-modules/m1.js

@@ -0,0 +1,2 @@
+import { value } from './m2'
+export { value }

playground/hmr/circular-boundary-many-modules/m2.js

@@ -0,0 +1,2 @@
+import { value } from './m3'
+export { value }

playground/hmr/circular-boundary-many-modules/m3.js

@@ -0,0 +1,2 @@
+import { value } from './m4'
+export { value }

playground/hmr/circular-boundary-many-modules/m4.js

@@ -0,0 +1,2 @@
+import { value } from './m5'
+export { value }

playground/hmr/circular-boundary-many-modules/m5.js

@@ -0,0 +1,2 @@
+import { value } from './m6'
+export { value }

playground/hmr/circular-boundary-many-modules/m6.js

@@ -0,0 +1,2 @@
+import { value } from './m7'
+export { value }