Skip to content
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
55 changes: 55 additions & 0 deletions packages/vite/src/node/server/__tests__/hmr.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import path from 'node:path'
import { expect, onTestFinished, test } from 'vitest'
import { handleHMRUpdate } from '../hmr'
import { type ServerOptions, createServer } from '../index'

async function testRestartDuringHotUpdate(serverOptions: ServerOptions = {}) {
let hookCalls = 0
const server = await createServer({
configFile: false,
root: import.meta.dirname,
logLevel: 'silent',
server: {
middlewareMode: true,
ws: false,
...serverOptions,
},
plugins: [
{
name: 'restart-during-hot-update',
async hotUpdate() {
if (hookCalls++ === 0) {
await server.restart()
throw new Error('hot update interrupted by restart')
}
},
},
],
})
onTestFinished(async () => {
await server.close()
})

await expect(
handleHMRUpdate(
'update',
path.join(import.meta.dirname, 'fixture.js'),
server,
),
).resolves.toBeUndefined()
}

test('cancels HMR when the server restarts during a hot update', async () => {
await testRestartDuringHotUpdate()
})

test('does not schedule stale HMR with a custom environment handler', async () => {
let hotUpdateEnvironmentsCalls = 0
await testRestartDuringHotUpdate({
async hotUpdateEnvironments(server, hmr) {
hotUpdateEnvironmentsCalls++
await Promise.all(Object.values(server.environments).map(hmr))
},
})
expect(hotUpdateEnvironmentsCalls).toBe(0)
})
20 changes: 15 additions & 5 deletions packages/vite/src/node/server/hmr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -416,7 +416,11 @@ export async function handleHMRUpdate(
const { config } = server
const mixedModuleGraph = ignoreDeprecationWarnings(() => server.moduleGraph)

const environments = Object.values(server.environments)
const environmentSnapshot = server.environments
const environments = Object.values(environmentSnapshot)
// A plugin hook may restart the server, replacing the environments and
// invalidating this HMR transaction.
const isStale = () => server.environments !== environmentSnapshot
const shortFile = getShortName(file, config.root)

const isConfig = file === config.configFile
Expand Down Expand Up @@ -509,14 +513,13 @@ export async function handleHMRUpdate(
const clientHotUpdateOptions = hotMap.get(clientEnvironment)!.options
const ssrHotUpdateOptions = hotMap.get(ssrEnvironment)?.options
try {
for (const plugin of getSortedHotUpdatePlugins(
server.environments.client,
)) {
for (const plugin of getSortedHotUpdatePlugins(clientEnvironment)) {
if (plugin.hotUpdate) {
const filteredModules = await getHookHandler(plugin.hotUpdate).call(
clientContext,
clientHotUpdateOptions,
)
if (isStale()) return
if (filteredModules) {
clientHotUpdateOptions.modules = filteredModules
// Invalidate the hmrContext to force compat modules to be updated
Expand Down Expand Up @@ -552,6 +555,7 @@ export async function handleHMRUpdate(
const filteredModules = await getHookHandler(
plugin.handleHotUpdate!,
).call(contextForHandleHotUpdate, mixedHmrContext)
if (isStale()) return
if (filteredModules) {
mixedHmrContext.modules = filteredModules
clientHotUpdateOptions.modules =
Expand Down Expand Up @@ -590,7 +594,8 @@ export async function handleHMRUpdate(
}
}
} catch (error) {
hotMap.get(server.environments.client)!.error = error
if (isStale()) return
hotMap.get(clientEnvironment)!.error = error
}

for (const environment of environments) {
Expand All @@ -604,17 +609,20 @@ export async function handleHMRUpdate(
context,
hot.options,
)
if (isStale()) return
if (filteredModules) {
hot.options.modules = filteredModules
}
}
}
} catch (error) {
if (isStale()) return
hot.error = error
}
}

async function hmr(environment: DevEnvironment) {
if (isStale()) return
Comment on lines 624 to +625

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sapphi-red IIUC this is the main stale check we need to support hotUpdateEnvironments right? The rest of the stale checks are only early-bails for performance?

I don't mind the other checks, but it being sprinkled around I'd like to understand in what cases we need the check for.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is the one for hotUpdateEnvironments. I think other places are needed to bail out hotUpdate / handleHotUpdate for cases like below technically:

handleHotUpdate({ server }) {
  this.environment.name in server.environments
}

try {
const { options, error } = hotMap.get(environment)!
if (error) {
Expand Down Expand Up @@ -654,6 +662,8 @@ export async function handleHMRUpdate(
}
}

if (isStale()) return

const hotUpdateEnvironments =
server.config.server.hotUpdateEnvironments ??
((server, hmr) => {
Expand Down