Skip to content
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

feat!: support file:// resolution #18422

Merged
merged 7 commits into from
Oct 28, 2024

Conversation

hi-ogawa
Copy link
Collaborator

@hi-ogawa hi-ogawa commented Oct 22, 2024

Description

Copying #18421 on top of main since its base branch #18361 is concerned with a different aspect of runner implementation.

The rational of supporting file:// is briefly mentioned here #18361 (comment). I think the current situation is:

  • Node supports import "file://...
  • Node and Vite supports import "/abs-path.js" on Unix, but for Windows, import "C:/abs-path.js" fails.
  • For Vite, /@fs can be used to workaround.
  • For Vite 6 module runner specifically (i.e. dev ssr), import "file://... works.
  • User can use a plugin resolveId to resolve file://.

And the necessary decision is probably a one of

  • keep it as is
  • remove file:// support from Vite 6 module runner (this is partly done in PR 18361)
  • support file:// for ssr dev/build (this PR)
  • support file:// for both client and ssr dev/build

@hi-ogawa
Copy link
Collaborator Author

/ecosystem-ci run

@vite-ecosystem-ci

This comment was marked as outdated.

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 4e0b6fd: Open

suite result latest scheduled
astro failure failure
remix failure failure
sveltekit failure failure
vike failure failure
vite-environment-examples success failure
vite-plugin-svelte failure failure
vitest failure failure

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

@hi-ogawa hi-ogawa changed the title feat(ssr): support file:// resolve for server consumer (clean branch) feat(ssr): support file:// resolve Oct 23, 2024
@hi-ogawa hi-ogawa changed the title feat(ssr): support file:// resolve feat(ssr): support file:// resolution Oct 23, 2024
@hi-ogawa hi-ogawa marked this pull request as ready for review October 23, 2024 05:58
@sapphi-red
Copy link
Member

sapphi-red commented Oct 24, 2024

I'm for supporting fileURLs in both client and SSR dev/build.

  • Node and Vite supports import "/abs-path.js" on Unix, but for Windows, import "C:/abs-path.js" fails.

To clarify this part, I wrote down a support matrix.

type example Node Deno Bun Vite
file URL import "file://..."
absolute path (linux) import "/home/foo.mjs"
absolute path (windows) import "C:\\Users\\foo.mjs"
normalized absolute path (windows) import "C:/Users/foo.mjs"
protocol relative URL (windows) import "/C:/Users/foo.mjs"
drive relative file URL (windows) import "/Users/foo.mjs"

@hi-ogawa
Copy link
Collaborator Author

Thanks for the summary! I didn't know the last two are possible on Windows. It looks like most of us agree with supporting this everywhere, so I'll go ahead removing ssr check and try adding tests for client dev/build.

@hi-ogawa hi-ogawa changed the title feat(ssr): support file:// resolution feat!: support file:// resolution Oct 25, 2024
@hi-ogawa
Copy link
Collaborator Author

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

📝 Ran ecosystem CI on 582719f: Open

suite result latest scheduled
astro failure failure
nuxt success failure
redwoodjs failure failure
remix failure failure
sveltekit failure failure
vike failure failure
vite-plugin-svelte failure failure
vitest failure failure

analogjs, histoire, ladle, laravel, marko, previewjs, quasar, qwik, rakkas, storybook, unocss, vite-environment-examples, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-vue, vite-setup-catalogue, vitepress, vuepress

@sapphi-red sapphi-red changed the title feat!: support file:// resolution feat: support file:// resolution Oct 28, 2024
@sapphi-red
Copy link
Member

I removed ! from the PR title as I don't think this is a breaking change. But let me know if it's a breaking change.

@sapphi-red sapphi-red added the p3-significant High priority enhancement (priority) label Oct 28, 2024
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.

Thanks!

@hi-ogawa
Copy link
Collaborator Author

I removed ! from the PR title as I don't think this is a breaking change. But let me know if it's a breaking change.

Doesn't it have a similar effect as #17369 (comment)? Users will need enforce: true for user plugins to intercept file:// if they want to do something different from what this PR does for whatever reason.

@sapphi-red
Copy link
Member

That's true. Yeah, let's add it back and add this PR to https://github.com/vitejs/vite/blob/main/docs/guide/migration.md#advanced.

@sapphi-red sapphi-red changed the title feat: support file:// resolution feat!: support file:// resolution Oct 28, 2024
@patak-dev patak-dev merged commit 6a7e313 into vitejs:main Oct 28, 2024
16 checks passed
@hi-ogawa hi-ogawa deleted the feat-resolve-file-url-2 branch October 29, 2024 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change p3-significant High priority enhancement (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants