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

fix(hmr): dedupe virtual modules in module graph #10144

Merged
merged 9 commits into from Sep 19, 2022

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Sep 16, 2022

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.

@bluwy bluwy added feat: hmr p3-downstream-blocker 🔨 Blocking the downstream ecosystem to work properly (priority) labels Sep 16, 2022
@patak-dev
Copy link
Member

patak-dev commented Sep 17, 2022

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
Copy link
Member Author

bluwy commented Sep 17, 2022

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
Copy link
Member

patak-dev commented Sep 17, 2022

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:

this.idToModuleMap.set(resolvedId, mod)

Akryum added a commit to histoire-dev/histoire that referenced this pull request Sep 17, 2022
@bluwy
Copy link
Member Author

bluwy commented Sep 17, 2022

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
Copy link
Member

patak-dev commented Sep 17, 2022

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
Copy link
Member Author

bluwy commented Sep 17, 2022

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!

@patak-dev patak-dev added this to the 3.2 milestone Sep 19, 2022
@bluwy
Copy link
Member Author

bluwy commented Sep 19, 2022

/ecosystem-ci run

@vite-ecosystem-ci
Copy link

vite-ecosystem-ci bot commented Sep 19, 2022

📝 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
Copy link
Member Author

bluwy commented Sep 19, 2022

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

@patak-dev patak-dev removed this from the 3.2 milestone Sep 19, 2022
@patak-dev patak-dev merged commit 71f08e7 into vitejs:main Sep 19, 2022
13 checks passed
@bluwy bluwy deleted the fix-hmr-virtual-modules branch Sep 19, 2022
@matthewp
Copy link
Contributor

matthewp commented Sep 19, 2022

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
Copy link

Akryum commented Oct 2, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: hmr p3-downstream-blocker 🔨 Blocking the downstream ecosystem to work properly (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HMR broken in certain cases when using the Storyblok integration
4 participants