fix(hmr): dedupe virtual modules in module graph

[bluwy]

Description

Fix withastro/astro#4727

In the module graph, virtual:foo that resolves to \0virtual:foo could be represented as both /@id/__x00__virtual:foo and \0virtual:foo in the module graph (two separate modules). This is because in some cases we use unwrapId and some not.

• This PR calls unwrapId in the module graph to ensure they are deduped.
• new: This PR calls unwrapId in the import analysis so that urls passed to the module graph are already unwrapped.
• I also refactored unwrapId and added wrapId (not related to the bug, but I refactored along)

Additional context

Note: The below questions has been addressed in the GitHub comments below

• It looks like \0virtual:foo is being treated as a "url" in module graph terms, which doesn't sound right. I tried to fix this by passing /@id/__x00__virtual:foo as URLs but SSR seems to always fail to load and transform for some reason.
• I tried to avoid using unwrapId in the module graph since it feels a bit hacky/catch-all, but there's many place where we're not properly unwrap the id that might further clutter the code.
• Perhaps the above change could be done in the id <-> url re-definition we discussed before.

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

[patak-dev]

So good that you did the refactoring, that will simplify discussions.

What I'm worried about doing the unwrapping at resolveUrl is the same that we discussed here: #9124 (comment)

Supporting both is comfortable, you don't need users to think about what space their URLs are (normally if you are in a middleware you are browser space, and if you are in a plugin you are in rollup space). But also then we can't have proper decode/encode escape hatches (these are implemented right now, but maybe we should?). Users can't have a /@fs folder on their system, because we don't pre-escape them as you would do for special chars like: %%, \\. It could be for example __vite_escaped_@id. Or since all these are at the start, maybe just something like /@fs/@fs. If we want this to be an option, maybe ssrLoadModule shouldn't do any unwrapping.

This was in the context of the same problem but for ssrLoadModule, where we do accept both types of URL. So if we do this at resolveUrl which is a public API, we are going in the opposite direction of allowing us to do proper escaping later.
I think it would be better to find where is resolveUrl being called with a browser URL (wrapped) instead of a rollup URL (unwrapped) and fix it there so we have the chance to make ssrLoadModule work only at the rollup URL space in Vite 4.

[bluwy]

I think it would be better to find where is resolveUrl being called with a browser URL (wrapped) instead of a rollup URL (unwrapped) and fix it there so we have the chance to make ssrLoadModule work only at the rollup URL space in Vite 4.

Interesting, I thought the term "url" and "id" meant "browser URL" and "rollup URL" respectively. In that path specifiers in browsers is called "url", while in Rollup it's called "id". Since in the module graph, we have urlToModuleMap and idToModuleMap properties.

• transformRequest: expects a Rollup-space URL
• ssrLoadModules: it supports both a Rollup-space or Browser-space URL

I also thought that we want ssrLoadModule to only support browser-space URL (or just "url"), since the parameter name is called url. But I think it's nice to support both "url" and "id" for convenience and backwards-compat.

I'm not so sure about having transformRequest support rollup-space URL (or "id"), since it expands on the scope of a "request" 🤔

---

Back to this PR and resolveUrl, since the parameter name is called url, I thought we'd want to always receive a browser-space URL (or "url"), so it receiving /@id/__x00__virtual:foo is expected I believe.

I also pushed a commit to fix this logic since I was incorrectly unwrapping the resolved URL where it shouldn't.

[patak-dev]

I think it would be better to find where is resolveUrl being called with a browser URL (wrapped) instead of a rollup URL (unwrapped) and fix it there so we have the chance to make ssrLoadModule work only at the rollup URL space in Vite 4.

Interesting, I thought the term "url" and "id" meant "browser URL" and "rollup URL" respectively. In that path specifiers in browsers is called "url", while in Rollup it's called "id". Since in the module graph, we have urlToModuleMap and idToModuleMap properties.

url is actually being used for both "browser URL" and for "rollup URL", depending on the context. It is quite confusing, and I think it should be noted as you wrote here but that isn't currently the case.

What we have is:

• url <-> browser URL (what we get from the browser in the request)
• url <-> rollup URL (it should be called id, what plugins and transformRequest expect)
• id <-> it should be called resolvedId

• transformRequest: expects a Rollup-space URL
• ssrLoadModules: it supports both a Rollup-space or Browser-space URL

I also thought that we want ssrLoadModule to only support browser-space URL (or just "url"), since the parameter name is called url. But I think it's nice to support both "url" and "id" for convenience and backwards-compat.

It is named url but it actually means "rollup URL" (id in a future ideal world)

I'm not so sure about having transformRequest support rollup-space URL (or "id"), since it expands on the scope of a "request" 🤔

It is the other way around. You can see here in the TransformMiddleware. As soon as we get the request, we unwrap it (going back to having a \0) so we can pass a Rollup URL to transformRequest (id in that ideal future).

Current use of url in Vite's plugins and the module graph is confusing. For me, it should be called:

• urlToModuleMap (current) -> idToModuleMap
• idToModuleMap (current) -> resolvedIdToModuleMap

See here:

vite/packages/vite/src/node/server/moduleGraph.ts

Line 194 in cda361c

this.idToModuleMap.set(resolvedId, mod)

[bluwy]

Ah I see your point now, it's... confusing but I think we know that now 😄

I guess another thing to note for the module graph resolveUrl, the url (or rollup URL like you mention), it could receive /@fs/blabla too, so it's technically still receiving the browser URL, and we may need to handle unwrapping this too?

So back to

I think it would be better to find where is resolveUrl being called with a browser URL (wrapped) instead of a rollup URL (unwrapped) and fix it there so we have the chance to make ssrLoadModule work only at the rollup URL space in Vite 4.

I still think resolveUrl should still work in the browser-space URL because of how it's being used. APIs like ensureEntryFromUrl also expect a browser-space URL and I think this make sense instead of redefining it as a Rollup-space URL.

Instead I think we should try to find where Rollup-space URLs are passed and prevent that further forward as this distinction wasn't clear in the past.

Re transformRequest supporting Rollup URL, if we want to support this, I think resolveUrl could still stick with browser-space URL, similar to how ssrLoadModule works today too. (But I'm still not sure if transformRequest should support Rollup URL even if it's partially supported, e.g. the unwrapped id)

[patak-dev]

Ah I see your point now, it's... confusing but I think we know that now 😄

And I think we're still not there... maybe we should get in a call on Monday 😅

I guess another thing to note for the module graph resolveUrl, the url (or rollup URL like you mention), it could receive /@fs/blabla too, so it's technically still receiving the browser URL, and we may need to handle unwrapping this too?

I think that /@fs/ counts like a Rollup-space URL.

So back to

I think it would be better to find where is resolveUrl being called with a browser URL (wrapped) instead of a rollup URL (unwrapped) and fix it there so we have the chance to make ssrLoadModule work only at the rollup URL space in Vite 4.

I still think resolveUrl should still work in the browser-space URL because of how it's being used. APIs like ensureEntryFromUrl also expect a browser-space URL and I think this make sense instead of redefining it as a Rollup-space URL.

But this isn't the case, ensureEntryFromUrl expects a Rollup-space URL, and if there is a place where we are calling that with a Browser-space URL we should change that.

Instead I think we should try to find where Rollup-space URLs are passed and prevent that further forward as this distinction wasn't clear in the past.

This is a big change, as we are using Rollup-space URLs everywhere now (except when writing an import path that is meant to be read by the browser). IIUC, what we should make sure of is that we use Rollup-space URL everywhere and we only go to Browser-space on the limits of the server when the browser is going to read these ids (this is why we convert to Rollup-space URL in the transform middleware plugin)

Re transformRequest supporting Rollup URL, if we want to support this, I think resolveUrl could still stick with browser-space URL, similar to how ssrLoadModule works today too. (But I'm still not sure if transformRequest should support Rollup URL even if it's partially supported, e.g. the unwrapped id)

transformRequest only supports Rollup-space URL now, and I think it should remain like that. To me, Browser-space URL is an encoding that is done only when handling a path to the browser, everything else internally expects a Rollup-space URL (an id with \0 already, so rollup compatible plugins work as expected for example... they know nothing about our internal encoding of \0 or the addition of /@id/ so the browser doesn't explode)

[bluwy]

I think that /@fs/ counts like a Rollup-space URL.

Perhaps you're right, it looks like the resolve plugin handles this when passed as an id too.

---

I think I understand your point now and I could implement that too, but maybe we should get Evan's thoughts first so we don't accidentally re-define what url and id means currently. Happy to chat on Monday too!

[bluwy]

/ecosystem-ci run

[vite-ecosystem-ci]

📝 Ran ecosystem CI: Open

suite	result
astro	✅ success
iles	✅ success
ladle	✅ success
laravel	❌ failure
marko	✅ success
nuxt-framework	✅ success
rakkas	✅ success
storybook	❌ failure
svelte	✅ success
vite-plugin-ssr	✅ success
vite-setup-catalogue	✅ success
vitepress	✅ success
vitest	✅ success
windicss	✅ success

[bluwy]

Looks like this PR passes ecosystem-ci. Laravel and Storybook is currently failing on main

[matthewp]

I recommend updating terminology a bit. I'd go with:

• specifier - the unresolved string that users put into their code.
• id or Rollup id - The fully resolved id created by running the resolveId hook.
• pathname or Browser pathname or something like that - Don't call these URLs, they are just the pathname part of a URL. A URL includes a protocol. I think Vite does already support http(s) URLs so it's good to get the terminology here right.

[Akryum]

HMR for virtual modules still don't work when resolving with \0virtual:module-id.