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.

[stackblitz]

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]

📝 Ran ecosystem CI on c956ba2: Open

suite	result	latest scheduled
astro	❌ failure	❌ failure
nuxt	❌ failure	❌ failure
vike	❌ failure	❌ failure
vite-plugin-react-pages	❌ failure	❌ failure
vite-plugin-vue	❌ failure	✅ success
vitest	❌ failure	❌ failure

✅ 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]

📝 Ran ecosystem CI on cc138b0: Open

✅ vite-plugin-vue

[patak-dev]

I don't see a reason to limit a external URL to only be http. Users could be using ipfs:// for example. The original function was added as part of bigger features so there is no history about having to restrict it: fbf7ab4#diff-071a32aedd2ea59472ebb69fb456e818b103d1a332da632e12c2d54395938ad1R10

[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.

vite/packages/vite/src/node/plugins/resolve.ts

Line 344 in ed5ecd9

return options.idOnly ? id : { id, external: true }

virtual: seems to be used in many places (GitHub search result).

I think we can fix this by either

1. implement Support external in dev #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

[patak-dev]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI on 01f7432: Open

suite	result	latest scheduled
astro	❌ failure	✅ success
nuxt	❌ failure	❌ failure
qwik	❌ failure	❌ failure
vite-plugin-pwa	❌ failure	✅ success
vitest	❌ failure	❌ failure

✅ 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 withastro/astro#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.