Skip to content

fix(module-runner): allow already resolved id as entry #19768

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

Merged

Conversation

hi-ogawa
Copy link
Collaborator

@hi-ogawa hi-ogawa commented Apr 1, 2025

Description

I thought ensuring entry module resolveId makes sense, but that's causing inconsistency with how already resolved virtual module id is handled in other places. I think we can remove this to match transformRequest behavior in general. Otherwise, I'm not sure how to fix #19283.

I didn't add a test case for #19283, but confirmed the repro is fixed https://stackblitz.com/edit/github-i1t4t7n4?file=repro.js (EDIT: added in "virtual module hmr")

@hi-ogawa
Copy link
Collaborator Author

hi-ogawa commented Apr 1, 2025

/ecosystem-ci run

Copy link

pkg-pr-new bot commented Apr 1, 2025

Open in StackBlitz

npm i https://pkg.pr.new/vite@19768

commit: 22c17f2

@vite-ecosystem-ci

This comment was marked as outdated.

@vite-ecosystem-ci

This comment was marked as outdated.

@hi-ogawa hi-ogawa marked this pull request as ready for review April 1, 2025 03:25
@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 385baa8: Open

suite result latest scheduled
vike success failure
unocss success failure
storybook success failure
nuxt success failure
vite-plugin-vue success failure
laravel failure success
sveltekit failure success
vite-environment-examples success failure
vite-plugin-svelte failure failure
vitest failure success
vitepress success failure
vuepress success failure
quasar success failure

analogjs, astro, marko, vite-plugin-react-swc, vite-plugin-pwa, react-router, previewjs, vite-plugin-cloudflare, qwik, histoire, vite-plugin-react, rakkas, ladle, waku, vite-setup-catalogue

@sapphi-red sapphi-red added the p3-minor-bug An edge case that only affects very specific usage (priority) label Apr 7, 2025
Copy link
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.

I think #19283 should be fixed. On the other hand, I'm not sure about #19764. I prefer to be not fixed and keep the behavior as-is. But that'd require a complex refactor (probably, the normalization for browsers, which is also done for the module runner, needs to be removed) and I think it's not the right time to do that. To sum up, I think we can go with this PR. But I think we should have a test for #19283 as that's the reason why we are changing the behavior here.

@patak-dev patak-dev merged commit e2e11b1 into vitejs:main Apr 9, 2025
26 of 27 checks passed
@hi-ogawa hi-ogawa deleted the fix-ssr-allow-already-resolved-id-as-entry branch April 9, 2025 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: ssr p3-minor-bug An edge case that only affects very specific usage (priority) trigger: preview
Projects
None yet
4 participants