-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
fix(hmr): register css deps as type: asset
#20391
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
fix(hmr): register css deps as type: asset
#20391
Conversation
| if (url.startsWith('data:')) { | ||
| depModules.add(moduleGraph.createFileOnlyEntry(file)) | ||
| } else { | ||
| depModules.add(await moduleGraph.ensureEntryFromUrl(url)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const isClientHtmlChange = | ||
| file.endsWith('.html') && environment.name === 'client' | ||
|
|
||
| if (needFullReload || isClientHtmlChange) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is added for cases that was covered by
https://github.com/vitejs/vite/pull/20391/files#diff-863416a9b0769ffb1d7e36b6840ce6d0d84170a80a5c4ce4539464b0ebcbd137L801-L813
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the intent of this change with respect to the old code, but I just wondered what if html raw import. I think new code forces this to be full reload on html change, but previously the parent module can hmr with the code like this?
import html from "./some-file.html?raw";
export function MyComponent() {
console.log(html)
return ...
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've pushed a commit to fix that case (9eea422). The downside is that if the index.html is imported (import html from './index.html?raw'), the full-reload won't happen even if it should. But I think that's fine as that was the previous behavior and this is really an edge case.
| // additionally check for CSS importers, since a PostCSS plugin like | ||
| // Tailwind JIT may register any file as a dependency to a CSS file. | ||
| for (const importer of node.importers) { | ||
| if (isCSSRequest(importer.url) && !currentChain.includes(importer)) { | ||
| propagateUpdate( | ||
| importer, | ||
| traversedModules, | ||
| boundaries, | ||
| currentChain.concat(importer), | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This part is no longer needed. type=asset module is a separate module from type=js, so updateModules is called separately for CSS files.
|
/ecosystem-ci run |
commit: |
|
📝 Ran ecosystem CI on
✅ ladle, laravel, analogjs, vite-plugin-react, astro, storybook, marko, vite-plugin-svelte, vite-setup-catalogue, vite-environment-examples, react-router, qwik, rakkas, nuxt, quasar, vite-plugin-cloudflare, sveltekit, unocss, vite-plugin-pwa, waku, vitest, vuepress, vitepress |
|
I need to check the plugin-vue failure. |
|
It was a bug in plugin-vue: vitejs/vite-plugin-vue#625 |
| const isClientHtmlChange = | ||
| file.endsWith('.html') && environment.name === 'client' | ||
|
|
||
| if (needFullReload || isClientHtmlChange) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the intent of this change with respect to the old code, but I just wondered what if html raw import. I think new code forces this to be full reload on html change, but previously the parent module can hmr with the code like this?
import html from "./some-file.html?raw";
export function MyComponent() {
console.log(html)
return ...
}| depModules.add(await moduleGraph.ensureEntryFromUrl(url)) | ||
| } | ||
| } | ||
| depModules.add(moduleGraph.createFileOnlyEntry(file)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! I just explored the case in vitejs/vite-plugin-react#616, so let me share what I found. Client hot update looks like this when changing server component:
before
🔷 [hotUpdate] client [
{
environment: 'client',
url: '/src/routes/root.tsx',
id: '/home/hiroshi/code/others/vite-plugin-react/packages/plugin-rsc/examples/basic/src/routes/root.tsx',
file: '/home/hiroshi/code/others/vite-plugin-react/packages/plugin-rsc/examples/basic/src/routes/root.tsx',
type: 'js',
info: undefined,
meta: undefined,
importers: [ '/src/styles.css?direct', '/src/styles.css' ],
after
🔷 [hotUpdate] client [
{
environment: 'client',
url: '/@fs//home/hiroshi/code/others/vite-plugin-react/packages/plugin-rsc/examples/basic/src/routes/root.tsx',
id: null,
file: '/home/hiroshi/code/others/vite-plugin-react/packages/plugin-rsc/examples/basic/src/routes/root.tsx',
type: 'asset',
info: undefined,
meta: undefined,
importers: [ '/src/styles.css?direct', '/src/styles.css' ],
In my case, I already had filter based on id ctx.modules.map((mod) => mod.id).filter((v) => v !== null), so id: null also helped not hitting my custom css hot update handling.
The double slash in url: '/@fs//...' looks odd, but I'd assume it's essentially a placeholder value, so it shouldn't matter much.
hi-ogawa
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes an issue where CSS dependencies were being incorrectly registered as JavaScript modules in the module graph, causing mixed dependencies and HMR problems. The fix ensures that CSS dependencies (like files watched via addWatchFile) are now properly registered with type: 'asset' instead of type: 'js'.
- Changed module type from 'js' | 'css' to include 'asset' type for better categorization
- Updated HMR logic to handle asset modules correctly and trigger appropriate reload behavior
- Added specific handling for HTML file changes to trigger full page reloads when needed
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| packages/vite/src/node/server/moduleGraph.ts | Core changes to support 'asset' module type and ensure file-only modules are created as assets |
| packages/vite/src/node/server/mixedModuleGraph.ts | Type definition update to include 'asset' type |
| packages/vite/src/node/server/hmr.ts | HMR logic updates to handle asset modules and HTML file changes properly |
| packages/vite/src/node/plugins/css.ts | Simplified CSS dependency handling to use createFileOnlyEntry for all imports |
| playground/tailwind/* | Test case additions to verify HMR behavior with HTML file changes |
| playground/assets/* | Test case additions to verify raw HTML import HMR functionality |
Description
This PR is a revised version of #18972.
The dependencies registered by
this.addWatchFilein CSS is now added to the module graph withtype: 'asset'. In past, it was added withtype: 'js'. This made the JS dep and non-JS dep to be mixed and caused other issues (#3929, #4127, #9512)fixes #9512
fixes #19786
fixes #19563
refs #8356