Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Jan 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion packages/vite/src/client/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
64 changes: 40 additions & 24 deletions packages/vite/src/node/server/hmr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -142,7 +148,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[],
Expand All @@ -156,7 +163,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)
Expand All @@ -171,16 +178,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,
}),
),
)
}

Expand Down Expand Up @@ -257,7 +267,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)) {
Expand All @@ -278,9 +288,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.
Expand All @@ -304,9 +316,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
Expand All @@ -327,9 +341,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
}

Expand Down Expand Up @@ -368,7 +384,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
Expand Down Expand Up @@ -419,7 +435,7 @@ function isNodeWithinCircularImports(
importChain.map((m) => colors.dim(m.url)).join(' -> '),
)
}
return 'circular imports'
return true
}

// Continue recursively
Expand Down
8 changes: 4 additions & 4 deletions packages/vite/types/hmrPayload.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions playground/hmr/__tests__/hmr.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 () => {
Expand Down