diff --git a/packages/vite/src/client/bundledDevHmrClient.ts b/packages/vite/src/client/bundledDevHmrClient.ts index e75e603b7a5d6f..a8cc428234a818 100644 --- a/packages/vite/src/client/bundledDevHmrClient.ts +++ b/packages/vite/src/client/bundledDevHmrClient.ts @@ -16,13 +16,18 @@ export interface RolldownRuntimeLike { loadExports(id: string): unknown } +interface PropagationBoundary { + boundary: string + acceptedVia: string + isWithinCircularImport: boolean +} + type HmrUpdate = | { type: 'noop' } | { type: 'full-reload'; reason: string } | { type: 'boundaries' - /** `[boundary, acceptedVia]` pairs */ - boundaries: [string, string][] + boundaries: PropagationBoundary[] updateSet: string[] } @@ -69,7 +74,7 @@ export class BundledDevHMRClient extends HMRClient { changedIds: string[], opts?: { firstInvalidatedBy?: string }, ): HmrUpdate { - const boundaries: [string, string][] = [] + const boundaries: PropagationBoundary[] = [] const updateSet = new Set() const traversedModules = new Set() for (const changed of changedIds) { @@ -95,7 +100,7 @@ export class BundledDevHMRClient extends HMRClient { id: string, stack: string[], updateSet: Set, - boundaries: [string, string][], + boundaries: PropagationBoundary[], firstInvalidatedBy: string | undefined, traversedModules: Set, ): HmrUpdate | undefined { @@ -109,7 +114,11 @@ export class BundledDevHMRClient extends HMRClient { } } if (this.isSelfAccepted(id)) { - boundaries.push([id, id]) + boundaries.push({ + boundary: id, + acceptedVia: id, + isWithinCircularImport: this.isNodeWithinCircularImports(id, stack), + }) return } const parents = this.runtime @@ -122,28 +131,107 @@ export class BundledDevHMRClient extends HMRClient { } } for (const parent of parents) { + const subChain = [...stack, parent] if (this.acceptsDep(parent, id)) { - boundaries.push([parent, id]) + boundaries.push({ + boundary: parent, + acceptedVia: id, + isWithinCircularImport: this.isNodeWithinCircularImports( + parent, + subChain, + ), + }) continue } - if (stack.includes(parent)) { - return { - type: 'full-reload', - reason: `circular import chain between \`${id}\` and \`${parent}\``, - } + if (!stack.includes(parent)) { + const fullReload = this.bubble( + parent, + subChain, + updateSet, + boundaries, + firstInvalidatedBy, + traversedModules, + ) + if (fullReload) return fullReload } - const fullReload = this.bubble( - parent, - [...stack, parent], - updateSet, - boundaries, - firstInvalidatedBy, - traversedModules, - ) - if (fullReload) return fullReload } } + /** + * 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 + * @param traversedModules The set of modules that have traversed + */ + private isNodeWithinCircularImports( + node: string, + nodeChain: string[], + currentChain: string[] = [node], + traversedModules = new Set(), + ): boolean { + // To help visualize how each parameter works, 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. + + if (traversedModules.has(node)) { + return false + } + traversedModules.add(node) + + for (const importer of this.runtime.getImporters(node)) { + // 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 + // 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(), + ] + this.logger.debug( + `circular imports detected: ${importChain.join(' -> ')}`, + ) + return true + } + + // Continue recursively + if (!currentChain.includes(importer)) { + const result = this.isNodeWithinCircularImports( + importer, + nodeChain, + currentChain.concat(importer), + traversedModules, + ) + if (result) return result + } + } + return false + } + handlePush(payload: BundledDevUpdatePayload): void { this.applyQueue = this.applyQueue .then(() => this.applyPush(payload)) @@ -247,22 +335,41 @@ export class BundledDevHMRClient extends HMRClient { } // collect callbacks before the caches are removed - const applies = update.boundaries.map(([boundary, acceptedVia]) => ({ - boundary, - acceptedVia, - callbacks: - this.hotModulesMap - .get(boundary) - ?.callbacks.filter((c) => c.deps.includes(acceptedVia)) ?? [], - })) + const applies = update.boundaries.map( + ({ boundary, acceptedVia, isWithinCircularImport }) => ({ + boundary, + acceptedVia, + isWithinCircularImport, + callbacks: + this.hotModulesMap + .get(boundary) + ?.callbacks.filter((c) => c.deps.includes(acceptedVia)) ?? [], + }), + ) for (const id of update.updateSet) { this.runtime.removeModuleCache(id) } - for (const { boundary, acceptedVia, callbacks } of applies) { - this.runtime.initModule(acceptedVia) - const fresh = this.runtime.loadExports(acceptedVia) + for (const { + boundary, + acceptedVia, + isWithinCircularImport, + callbacks, + } of applies) { + let fresh: unknown + try { + this.runtime.initModule(acceptedVia) + fresh = this.runtime.loadExports(acceptedVia) + } catch (err) { + if (isWithinCircularImport) { + this.requestFullReload( + `${acceptedVia} failed to apply HMR as it's within a circular import. Reloading page to reset the execution order.`, + ) + return + } + throw err + } try { this.currentFirstInvalidatedBy = firstInvalidatedBy for (const { deps, fn } of callbacks) { @@ -284,16 +391,19 @@ export class BundledDevHMRClient extends HMRClient { } private toUpdatePayload( - boundaries: [string, string][], + boundaries: PropagationBoundary[], firstInvalidatedBy: string | undefined, ): UpdatePayload { - const updates: Update[] = boundaries.map(([boundary, acceptedVia]) => ({ - type: 'js-update', - path: boundary, - acceptedPath: acceptedVia, - timestamp: Date.now(), - firstInvalidatedBy, - })) + const updates: Update[] = boundaries.map( + ({ boundary, acceptedVia, isWithinCircularImport }) => ({ + type: 'js-update', + path: boundary, + acceptedPath: acceptedVia, + timestamp: Date.now(), + isWithinCircularImport, + firstInvalidatedBy, + }), + ) return { type: 'update', updates } } diff --git a/playground/hmr/__tests__/hmr.spec.ts b/playground/hmr/__tests__/hmr.spec.ts index bb63a04b841a59..7e17b2363aedf1 100644 --- a/playground/hmr/__tests__/hmr.spec.ts +++ b/playground/hmr/__tests__/hmr.spec.ts @@ -351,20 +351,15 @@ if (!isBuild) { .toMatch('child updated') }) - // bundled dev: the invalidate inside the circular import ends in a full - // page reload instead of a hot update - test.skipIf(isBundledDev)( - 'invalidate in circular dep should be hot updated if possible', - async () => { - const el = page.locator('.invalidation-circular-deps-handled') - await expect.poll(() => el.textContent()).toMatch('child') - editFile( - 'invalidation-circular-deps/invalidate-handled-in-circle/child.js', - (code) => code.replace('child', 'child updated'), - ) - await expect.poll(() => el.textContent()).toMatch('child updated') - }, - ) + test('invalidate in circular dep should be hot updated if possible', async () => { + const el = page.locator('.invalidation-circular-deps-handled') + await expect.poll(() => el.textContent()).toMatch('child') + editFile( + 'invalidation-circular-deps/invalidate-handled-in-circle/child.js', + (code) => code.replace('child', 'child updated'), + ) + await expect.poll(() => el.textContent()).toMatch('child updated') + }) // bundled dev: file changes do not run the `hotUpdate` plugin hook, so the // plugin's custom events never reach the page @@ -1196,29 +1191,27 @@ if (!isBuild) { ) }) - // bundled dev: - // 1. `unexpected no error` is shown instead of `expected error`, - // `_value` in `mod-c` is initialzed with `var` and is hoisted to the top of the module, - // whereas in unbundled dev, `_value` is still in TDZ, which causes a `ReferenceError`. - // - // 2. an edit inside the circular import chain triggers a full - // page reload instead of a hot update - test.skipIf(isBundledDev)( - 'hmr should not reload if no accepted within circular imported files', - async () => { - await page.goto(viteTestUrl + '/circular/index.html') - const el = page.locator('.circular') - await expect - .poll(() => 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) ->`), + test('hmr should not reload if no accepted within circular imported files', async () => { + await page.goto(viteTestUrl + '/circular/index.html') + const el = page.locator('.circular') + await expect + .poll(() => el.textContent()) + .toBe( + isBundledDev + ? // bundled-dev: `unexpected no error` is shown instead of `expected error`. + // Vite uses `output.topLevelVar = true` by default in bundled dev, which causes the circular import to be `undefined` instead of throwing a TDZ error, + // (see https://rolldown.rs/guide/troubleshooting#avoid-relying-on-temporal-dead-zone-tdz-errors), + // whereas in unbundled dev, it follows TDZ and shows `expected error`. + 'mod-a -> mod-b -> mod-c -> undefined (unexpected no error)' + : 'mod-a -> mod-b -> mod-c -> mod-a (expected error)', ) - await expect - .poll(() => el.textContent()) - .toBe('mod-a -> mod-b (edited) -> mod-c -> mod-a (expected error)') - }, - ) + editFile('circular/mod-b.js', (code) => + code.replace(`mod-b ->`, `mod-b (edited) ->`), + ) + await expect + .poll(() => el.textContent()) + .toBe('mod-a -> mod-b (edited) -> mod-c -> mod-a (expected error)') + }) // bundled dev: editing an asset file does not push an update to the page test.skipIf(isBundledDev)('not inlined assets HMR', async () => {