fix: collect virtual module CSS in vite dev #2039
Conversation
🦋 Changeset detectedLatest commit: d1a23f5 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
katywings
added
bug
commit: |
✅ Deploy Preview for solid-start-landing-page ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
katywings
changed the title
katywings
changed the title
| try { | ||
| const res = await vite.fetchModule(file, importer); | ||
| if (!("id" in res)) return; | ||
| return vite.moduleGraph.getModuleById(res.id); |
There was a problem hiding this comment.
much cleaner! 🥇
but this is removing the fallback vite.fetchModuleByUrl and relying only in the moduleid- is this not needed or am I missing something else? 🤔
There was a problem hiding this comment.
relying only in the moduleid
The new line const res = await vite.fetchModule(file, importer); resolves the raw file url. But it's result doesn't include all fields from the module, that's the reason why I have to also call vite.moduleGraph.getModuleById on the later line. The logic here is:
- Resolve raw file url to module id via
vite.fetchModule - Get missing module fields via
vite.moduleGraph.getModuleById
removing the fallback
vite.fetchModuleByUrl
fetchModuleByUrl - is this a typo 🤔? Afaik this API does not exist, but I assume you mean getModuleByUrl / ensureEntryFromUrl?
So to "quickly" comment my thoughts about those:
Removed ensureEntryFromUrl
The new vite.fetchModule call internally already calls ensureEntryFromUrl (https://github.com/vitejs/vite/blob/964c718a382ff46ec1f906d7d6bc3f135a6dcd3f/packages/vite/src/node/ssr/fetchModule.ts#L83), so we don't have to do that manually anymore.
Removed getModuleByUrl
nodePath = resolve(nodePath);
node = await vite.moduleGraph.getModuleByUrl(file);
- This old logic was already broken, because
getModuleByUrlwas called withfileinstead ofnodePath🙈. - But assuming that the old logic somehow was correct: getModuleByUrl basically is a small subset of ensureEntryFromUrl.
Removed getModuleById
- The old logic also partially already was broken to begin with
- But assuming that the old logic somehow was correct:
ensureEntryFromUrlinternally calls _resolveUrl and_resolveUrluses _resolveId to get the id from the url.
Extra words: To be completely honest, deleting all that old logic and replacing it with the simple two lines didn't exactly feel good, especially because we do not have lot's of tests around this. I tried my best to test the new logic, but there's certainly the chance that I missed something. So there is the risk that with the new logic some unexpected cases might pop up where CSS is not server rendered during the alpha. This however gives us the opportunity to then include those cases in the CSS fixture and bring back the few actually needed parts of the old logic, if necessary. What do you think of this plan @atilafassina (and @Brendonovich)?
There was a problem hiding this comment.
thanks for the explanation!!
I agree with your reasoning: removing what looks wrong/unnecessary/redundant and watching for edge cases
Changes
\0virtual:module.cssto/@id/__x00__virtual:module.cssin manifest, to fix dynamic import of virtual modulesdata-vite-dev-idattributes for style tags of virtual modulesFixes tested with
Other information
Before change 2+3: https://github.com/user-attachments/assets/630e827b-5241-403f-9a70-5d4ccbb0db90
After: https://github.com/user-attachments/assets/7d3af2e8-5211-4f05-b8de-9f14ef153549
Before change 4: https://github.com/user-attachments/assets/e8e59e0b-800f-42ab-b8df-f452ea8d0de7
After: https://github.com/user-attachments/assets/a19627dd-232e-41b1-b72f-f54714b5df42