fix: external URLs don't have to start with http(s)

[asivery]

Description

Right now, when building, vite checks if the base provided is an external URL by checking if it starts with https:// or http://. This doesn't have to be the case. When working with electron, for example, one can create a custom protocol, then load the page with it. Right now setting the base to sandbox:// causes a broken app in electron. This PR fixes it.

[bolt-new-by-stackblitz[bot]]

Run & review this pull request in StackBlitz Codeflow.

[DaveFlashNL]

yeah, cos an external url can also be a local file in electron right, so if it's hardcoded for http(s) it would break on 'file:///', right?

[patak-dev]

/ecosystem-ci run

[vite-ecosystem-ci[bot]]

📝 Ran ecosystem CI on c956ba2: Open

suite	result	latest scheduled
astro	:x: failure	:x: failure
nuxt	:x: failure	:x: failure
vike	:x: failure	:x: failure
vite-plugin-react-pages	:x: failure	:x: failure
vite-plugin-vue	:x: failure	:white_check_mark: success
vitest	:x: failure	:x: failure

:white_check_mark: analogjs, histoire, ladle, laravel, marko, previewjs, quasar, qwik, rakkas, remix, sveltekit, unocss, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-svelte, vite-setup-catalogue, vitepress

[patak-dev]

/ecosystem-ci run vite-plugin-vue

[vite-ecosystem-ci[bot]]

📝 Ran ecosystem CI on cc138b0: Open

:white_check_mark: vite-plugin-vue

[sapphi-red]

I think file:// URLs should not be externalized as it was working like that.

One thing to point out is that IIUC this will make non-enforce: pre plugins not able to handle ids with protocols (e.g. virtual-module:, virtual:, custom:). This line would set external: true. https://github.com/vitejs/vite/blob/ed5ecd90fdddcd6a6bdce1858735ae47a6a1e2da/packages/vite/src/node/plugins/resolve.ts#L344 virtual: seems to be used in many places (GitHub search result).

I think we can fix this by either

1. implement https://github.com/vitejs/vite/issues/6582
2. add a fallback plugin that runs after other plugins and externalizes any id with protocol

[patak-dev]

Good call with file://, that shouldn't be considered external.

Could you expand on the virtual: issue? The regex will only match ids with xxx://, no?

[sapphi-red]

The regex will only match ids with xxx://, no?

My bad, forgot that the regex includes // 😅

[sapphi-red]

Just noticed that file:// URLs doesn't work in Vite now 🫠 Probably want to support it in future though.

[patak-dev]

/ecosystem-ci run

[vite-ecosystem-ci[bot]]

📝 Ran ecosystem CI on cc138b0: Open

suite	result	latest scheduled
astro	:x: failure	:white_check_mark: success
nuxt	:x: failure	:x: failure
qwik	:x: failure	:x: failure
vite-plugin-react	:x: failure	:white_check_mark: success
vite-plugin-vue	:x: failure	:white_check_mark: success
vitest	:x: failure	:x: failure

:white_check_mark: analogjs, histoire, ladle, laravel, marko, previewjs, quasar, rakkas, remix, sveltekit, unocss, vike, vite-plugin-pwa, vite-plugin-react-swc, vite-plugin-svelte, vite-setup-catalogue, vitepress

[patak-dev]

/ecosystem-ci run

[vite-ecosystem-ci[bot]]

📝 Ran ecosystem CI on 01f7432: Open

suite	result	latest scheduled
astro	:x: failure	:white_check_mark: success
nuxt	:x: failure	:x: failure
qwik	:x: failure	:x: failure
vite-plugin-pwa	:x: failure	:white_check_mark: success
vitest	:x: failure	:x: failure

:white_check_mark: analogjs, histoire, ladle, laravel, marko, previewjs, quasar, rakkas, remix, sveltekit, unocss, vike, vite-plugin-react, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress

[patak-dev]

The vite-plugin-pwa looks like a fluke due to a failed sharp install. @bluwy, the Astro fail seems legit. It seems there is a test for file:// in Astro's CI. So if this is expected to work, we could keep file:// as no external for now.

[bluwy]

I don't know if this would fix it, but Astro has a file:// plugin here, and I'm guessing it's a matter of marking it enforce: 'pre'. Let me see if I can do that without breaking anything.

[patak-dev]

here

It seems that Astro decided to make file://path URLs equivalent to path though. If Vite resolves them as external by default and frameworks start to add plugins to change them to regular paths it could get confusing. I don't know what is the best here though, we could argue for both. Maybe we could dig more into why Astro decided making them no external was the right call?

[bluwy]

For me (and also mentioned in the meeting), I think it would be nice if Vite can support it in the future, and this is also more of a stop point. file:// urls wouldn't really work normally otherwise, and they shouldn't be as different to absolute paths. @matthewp added it in https://github.com/withastro/astro/pull/9407, maybe he can fill in more details if this should be supported in core or not.

[patak-dev]

If we want to do this, for now maybe we shouldn't change how file:// works (by making them external). We can modify this PR to keep returning false for file:// from isExternal() for now.

[bluwy]

I think either way works for me, but since this will be merged in the next major, it shouldn't have a big impact in practice.

[matthewp]

We did it this way because we use URLs almost exclusively and it was easier just to pass the URL as a module specifier rather than convert it to a path. I'm always unsure if paths needed to be converted to forward slashes, or if they can start with C:\, etc. so this just seemed easier. I know normalizePath is maybe what we should be using instead.

I don't think I understand the logic of why file:// should be externalized by default. I guess I sort of understand why you're doing it that way for http(s), although I don't know if I'd necessarily do it that way if it were me, but files are just files and starting with file:// doesn't signal to me that this is some external thing.

[bluwy]

In our meeting notes, we decided to not externalize file://. However in https://github.com/vitejs/vite/pull/18422 we decided to support file:// in client and ssr. So I think we can merge the PR as is now?

[sapphi-red]

I think we should merge #18422 before this PR in that case. I guess it might be confusing when debugging later on finding that plugins handling fileURLs not working for some commits.

[sapphi-red]

/ecosystem-ci run

[vite-ecosystem-ci[bot]]

📝 Ran ecosystem CI on 062908a: Open

suite	result	latest scheduled
astro	:x: failure	:white_check_mark: success
redwoodjs	:x: failure	:x: failure
remix	:x: failure	:x: failure
sveltekit	:x: failure	:x: failure
vike	:x: failure	:x: failure
vite-environment-examples	:x: failure	:white_check_mark: success
vitest	:x: failure	:x: failure

:white_check_mark: analogjs, histoire, ladle, laravel, marko, nuxt, previewjs, quasar, qwik, rakkas, storybook, unocss, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress, vuepress

[vite-ecosystem-ci[bot]]

📝 Ran ecosystem CI on 062908a: Open

suite	result	latest scheduled
redwoodjs	:x: failure	:x: failure
remix	:x: failure	:x: failure
sveltekit	:x: failure	:x: failure
vike	:x: failure	:x: failure
vite-environment-examples	:x: failure	:white_check_mark: success
vitest	:x: failure	:x: failure

:white_check_mark: analogjs, astro, histoire, ladle, laravel, marko, nuxt, previewjs, quasar, qwik, rakkas, storybook, unocss, vite-plugin-pwa, vite-plugin-react, vite-plugin-react-swc, vite-plugin-svelte, vite-plugin-vue, vite-setup-catalogue, vitepress, vuepress