Skip to content

fix: correctly resolve hmr dep ids and fallback to url #18840

Merged
patak-cat merged 10 commits into
vitejs:mainfrom
patricklx:hmr-virtual
Jan 23, 2025
Merged

fix: correctly resolve hmr dep ids and fallback to url #18840
patak-cat merged 10 commits into
vitejs:mainfrom
patricklx:hmr-virtual

Conversation

@patricklx
Copy link
Copy Markdown
Contributor

@patricklx patricklx commented Nov 29, 2024

Description

fixes #12912
fixes #16375

this is the most basic to get it working. There are more issues regarding this, like having multiple modules in the module graph. one for 0virtual:file and another for /@id/__00__/virtual:file

@patricklx patricklx changed the title fix hot accept virtual module fix: hot accept virtual module Nov 29, 2024
@sapphi-red
Copy link
Copy Markdown
Member

Thank you for the PR!
I don't have much context for the linked issue and PRs, but my understandings are:

  • import.meta.hot.accept currently expects the deps argument to be a URL.
  • for a module with an id starting with \0, the url property of the module node for that module starts with \0
  • writing import.meta.hot.accept(urlForTheVirtualModule) doesn't work, because urlForTheVirtualModule starts with \0 instead of / and thus toAbsolutePath appends some string

I think there are two ways to fix the issue.

The first is to make import.meta.hot.accept to expect the deps argument to be an ID instead of a URL. I think this is more intuitive. But given that we have been expecting URLs, it's a breaking change. Maybe simply falling back to treating as a URL when resolving as an id fails would be fine.

The second is to make the url property of the module node to always start with /. Since it's a "URL", I expect it to always be a (non-relative) path part of the URL, and thus start with /. This fix would probably be more simple than the first one.

IIUC this PR makes import.meta.hot.accept to expect the deps arugument to be a URL for relative specifiers and to be an id for non-relative specifiers. I think that is confusing and better to expect only URLs or only ids.

@sapphi-red sapphi-red added feat: hmr p3-minor-bug An edge case that only affects very specific usage (priority) labels Dec 6, 2024
@patricklx
Copy link
Copy Markdown
Contributor Author

patricklx commented Dec 6, 2024

hi, alright, I think it should accept Ids, since the imports can be transformed based on imported and specifier. i think meta.hot.accept should do the same.

is it necessary a breaking change? I did not find that its documented to use url for deps.

@patricklx patricklx changed the title fix: hot accept virtual module fix: hot accept dep ids and fallback to url Dec 6, 2024
@patricklx
Copy link
Copy Markdown
Contributor Author

patricklx commented Dec 6, 2024

@sapphi-red this is now passing and still uses urls, but tries first to resolve the module via resolveId and then uses the resolved module.url.

on meta.hot.accept:
to be more precise:

  • import.meta.hot.accept currently expects the deps argument to be a URL at runtime and id at build time.

@patricklx patricklx changed the title fix: hot accept dep ids and fallback to url fix: correctly resolve dep ids and fallback to url Dec 6, 2024
@patricklx patricklx changed the title fix: correctly resolve dep ids and fallback to url fix: correctly resolve hmr dep ids and fallback to url Dec 6, 2024
@patricklx
Copy link
Copy Markdown
Contributor Author

@sapphi-red does this look okay now?

Copy link
Copy Markdown
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

Thank you for the change!

Comment thread packages/vite/src/node/plugins/importAnalysis.ts Outdated
Comment thread packages/vite/src/node/plugins/importAnalysis.ts
Comment thread packages/vite/src/node/plugins/importAnalysis.ts
Co-authored-by: 翠 / green <green@sapphi.red>
@sapphi-red

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@patricklx
Copy link
Copy Markdown
Contributor Author

I looked at vitest failure. vitest is currently failing on main

hi-ogawa
hi-ogawa previously approved these changes Dec 10, 2024
@vite-ecosystem-ci

This comment was marked as outdated.

@sapphi-red sapphi-red added this to the 6.1 milestone Dec 11, 2024
Comment thread packages/vite/src/node/plugins/importAnalysis.ts Outdated
@sapphi-red
Copy link
Copy Markdown
Member

@patricklx Would you also describe where you needed to use import.meta.hot.accept(virtualDeps)?

Co-authored-by: 翠 / green <green@sapphi.red>
@patricklx
Copy link
Copy Markdown
Contributor Author

@patricklx Would you also describe where you needed to use import.meta.hot.accept(virtualDeps)?

I have a vite plugin that generates virtual files which can also change depending on other files on the filesystem.
currently it's impossible to hot accept them.

@sapphi-red
Copy link
Copy Markdown
Member

@patricklx Would you also describe where you needed to use import.meta.hot.accept(virtualDeps)?

I have a vite plugin that generates virtual files which can also change depending on other files on the filesystem. currently it's impossible to hot accept them.

Is self-accepting on the virtual module side (i.e. call import.meta.accept(cb) in the virtual module) not an option?

@patak-cat
Copy link
Copy Markdown
Member

Hmm, it seems rakkas and vitepress is passing a URL

Maybe we could add back the URL fallback in the next minor if we're clear that this is just for backward compat and we'll move to a warning on Vite v7.

@patricklx
Copy link
Copy Markdown
Contributor Author

So, should i add back the fallback then?

@sapphi-red
Copy link
Copy Markdown
Member

Yeah. Would you add back the fallback with a comment that this is just for backward compat and will remove it in Vite 7?

@patricklx
Copy link
Copy Markdown
Contributor Author

Yeah. Would you add back the fallback with a comment that this is just for backward compat and will remove it in Vite 7?

I added the change

sapphi-red
sapphi-red previously approved these changes Dec 18, 2024
Copy link
Copy Markdown
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

Thank you

@sapphi-red

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as duplicate.

@vite-ecosystem-ci

This comment was marked as outdated.

@sapphi-red sapphi-red dismissed their stale review December 18, 2024 03:58

The fallback does not seem to be working

Comment thread packages/vite/src/node/plugins/importAnalysis.ts Outdated
@sapphi-red

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@sapphi-red
Copy link
Copy Markdown
Member

/ecosystem-ci run

@vite-ecosystem-ci

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@vite-ecosystem-ci
Copy link
Copy Markdown

📝 Ran ecosystem CI on d17d16f: Open

suite result latest scheduled
histoire failure failure
laravel failure success
marko failure success
redwoodjs failure failure
remix failure failure
vike failure failure
vite-plugin-svelte success failure
waku failure failure

analogjs, astro, ladle, nuxt, previewjs, quasar, qwik, rakkas, storybook, sveltekit, unocss, vite-environment-examples, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-vue, vite-setup-catalogue, vitepress, vitest, vuepress

Copy link
Copy Markdown
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

Thank you 💚

@patricklx
Copy link
Copy Markdown
Contributor Author

@sapphi-red anything left todo here?

@patak-cat
Copy link
Copy Markdown
Member

@patricklx things look good! thanks! we'll merge this PR once we start the 6.1 beta (soon).

@patak-cat patak-cat merged commit b84498b into vitejs:main Jan 23, 2025
@patricklx patricklx deleted the hmr-virtual branch January 23, 2025 16:00
moonlitusun pushed a commit to moonlitusun/vite that referenced this pull request May 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feat: hmr p3-minor-bug An edge case that only affects very specific usage (priority) trigger: preview

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Support resolve import.meta.hot.accept dep Fails to hmr accept virtual modules

4 participants