-
-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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(runner): replace runner path normalization with fetchModule
resolve
#18361
base: main
Are you sure you want to change the base?
Conversation
Run & review this pull request in StackBlitz Codeflow. |
bb8c402
to
5e8e7ec
Compare
fetchModule
fetchModule
|
packages/vite/src/node/ssr/runtime/__tests__/fixtures/dynamic-import.js
Outdated
Show resolved
Hide resolved
packages/vite/src/node/ssr/runtime/__tests__/fixtures/dynamic-import.js
Outdated
Show resolved
Hide resolved
/ecosystem-ci run |
📝 Ran ecosystem CI on
✅ analogjs, histoire, ladle, laravel, marko, nuxt, previewjs, quasar, qwik, rakkas, redwoodjs, storybook, vite-environment-examples, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress, vuepress |
/ecosystem-ci run |
📝 Ran ecosystem CI on
✅ analogjs, histoire, ladle, laravel, marko, nuxt, previewjs, quasar, qwik, rakkas, redwoodjs, storybook, unocss, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-vue, vite-setup-catalogue, vitepress, vuepress |
fetchModule
fetchModule
fetchModule
fetchModule
resolve
runner.executeId('file:///C:/Users/name/dev/index.js') This is a valid Node.js code: await import('file:///C:/Users/name/dev/index.js') |
I agree that |
Description
Not so confident that this is a proper fix nor the issue needs fix, but I'm thinking about what would require to support #18223 and hopefully it helps deciding the fate of the issue (bug or wontfix).
One thing I noticed that it looks possible to dedupe/normalize
/abs-path-to/src/etc -> /src/etc
on server side (fetchModule
) and module identity is still kept intact on runner side since they are keyed by resolved id. ForFetchModuleResult.cache
detection, I replacedgetModuleByUrl
withensureEntryFromUrl
and I'm wondering what's the downside of this.todo
file://
supportfile://
support. It's still possible to use custom pluginresolveId
to resolvefile://
and I added tests for this usage.