fix(vite-dev-server): ensure assets are correctly reloaded #24965
Conversation
|
Thanks for taking the time to open a PR!
|
| if (mod.file === supportFilePath) { | ||
| debug('handleHotUpdate - support compile success') | ||
| devServerEvents.emit('dev-server:compile:success') |
There was a problem hiding this comment.
I was under the impression that devServerEvents.emit('dev-server:compile:success') is similar to server.ws.send( type: 'full-reload' }) as it destroys the AUT and creates a new one.
Seems like we have two mechanisms here for causing a reload, the full-reload and the dev-server:compile:success event. Do we know why the dev-server:compile:success wasn't working in this case?
There was a problem hiding this comment.
I think dev-server:compile:success just causes the spec to re-run (using any updated code via HMR). full-reload is a Vite client API which tells Vite to actually go and get the latest resources in their entirety (since HMR isn't working for the stylesheets).
So this change basically means we aren't using HMR for Vite anymore, but doing a full reload. This seems more correct imo - fresh slate every test WDYT?
HMR will be useful if we have a "workbench" mode one day like Storybook... but not so much for tests.
There was a problem hiding this comment.
This change does fix the linked issue but I realized that we are going to have this same issue inside of specs. To reproduce, move the import '../../src/styles.css' from the support file into a spec file (App.cy.ts from the reproduction).
The update of a spec relies on the devServerEvents.emit('dev-server:compile:success') rather than the ws reload so it causes the same problem.
I was messing around with it and I was able to fix this by changing return [] to return, not sure the implications of this. It just feels strange that we have a system for rerunning a spec when something changes dev-server:compile:success which should work how we want it to since it destroys the AUT and should trigger a refresh but it doesn't work for some reason..
There was a problem hiding this comment.
Hmmm I think the problem might be Vite caching, then? 🤔
dev-server:compile:success triggers a re-run but has nothing to do with fetching the latest data.
Right now it's
- HMR -> get latest data (spec, component) ->
dev-server:compile:success->rerun().
So the problem is Vite - I think it's cache is not getting updated, so it uses the stale one. I think for reliability we should probably just be using full-reload for Vite exclusively? It's more in line with the "fresh slate" and "test isolation" philosophy, for sure.
What do you think?
There was a problem hiding this comment.
I think you are right @ZachJW34, I will double down and find out why the HMR trigger doesn't work.
There was a problem hiding this comment.
I investigated. I think we are NOT using HMR what-so-ever right now. We refetch all assets every time the spec re-runs.
According to the Vite HMR docs.
Return an empty array and perform complete custom HMR handling by sending custom events to the client
We currently return an empty array, but we don't do any custom HMR (like import.meta.hot...). The assets are rebuilt (the JS modules in memory) but we don't do any HMR updates. We just emit the dev-server:compile:success, which casuse Cypress to re-run the spec, which re-requests the assets.
We have a few options to fix this.
- Just be to change
return []toreturn. This seems to work just fine. - use the new
reloadModuleAPI.
/**
* Triggers HMR for a module in the module graph. You can use the `server.moduleGraph`
* API to retrieve the module to be reloaded. If `hmr` is false, this is a no-op.
*/
reloadModule(module: ModuleNode): Promise<void>
I tried this the new reloadModule, I got it working with if (mod.file.endsWith('css')) { reloadModule(mod) }. Although this probably isn't what we want - it's more for forcing a virtual module to reload. It's also not available until Vite 3.2.0 - which seems to NOT be a straight forward upgrade :(
There was a problem hiding this comment.
I think we probably haven't implemented the Vite Dev Server ideally - we might be able to better leverage HMR. For now, WDYT of my fix? It seems in line with how Vite recommends using handleHotModule, and it seems to work great.
There was a problem hiding this comment.
I did some poking around of Vite's HMR and I can definitely see how return [] could cause some cache issues. Looks like returning an empty array essentially tells Vite that there is no new module and the previous module isn't invalidated. Maybe this is only a problem with virtual modules which is why we are seeing this with Tailwind.
I'm cool with the fix you have here! Not sure why the vite-dev-server tests crapped out but I'll rerun them.
There was a problem hiding this comment.
Maybe we move to reloadModule once we update to Vite 3.2. That's supposed to be for virtual modules, like Tailwind (I assume this is how it's implemented, anyway).
3ae70b3 to
58c3fd1
Compare
* do not use custom HMR * remove old code
User facing changelog
Fix a bug where updating a component would not trigger the
supportFileto reload imported stylesheets. Now we do afull-reloadfor Vite, to ensure we re-run the spec correctly with the latest styles.Additional details
I don't think we even want a partial hot reload - it's more consistent with our testing philosophy to always get a refresh slate, so a full reload makes sense. The perf diff is negligible, and only in interactive mode - it won't impact CI runs at all.
Steps to test
You can grab a binary and test it with the reproduction provided in the linked issue. Here's the binaries: e1d766c#comments
How has the user experience changed?
Correctly reloads latest stylesheets, even if it's import in
support/component.js. Before this wouldn't work - the latest styles wouldn't load, and you would be stuck with an unstyled component.simplescreenrecorder-2022-12-06_10.36.01.mp4
PR Tasks
cypress-documentation?type definitions?