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

vite: support HMR #925

Closed
wants to merge 1 commit into from
Closed

Conversation

wmertens
Copy link
Contributor

With this change, hot reload works in Qwik and presumably anything else using Vite.

The problem was that a change in css.ts didn't force a regen of the CSS.

@changeset-bot
Copy link

changeset-bot bot commented Nov 22, 2022

🦋 Changeset detected

Latest commit: b21f3b7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@vanilla-extract/vite-plugin Minor

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

@endigma
Copy link
Contributor

endigma commented Feb 20, 2023

What's pending here? This would majorly help us.

@mattcompiles
Copy link
Contributor

Hey @wmertens 👋

This PR implies that we don't currently support HMR in Vite but that's not true. Are you able to provide a reproduction of the issue that's being resolved here? When testing locally, HMR seems to work fine so I'm not sure why it doesn't work in your scenario?

Also sorry, I know this PR has been open for a while.

@wsow4
Copy link

wsow4 commented Apr 3, 2023

@mattcompiles
I can confirm the bug exists. HMR works in general, but not in every case. Sometimes the page reloads, but you don't see the changes you made in CSS.

The problem is server.moduleGraph.getModulesByFile(virtualId) sometimes returns more than one module and each of that module should be invalidated, but currently the plugin invalidates only the first one.

@MicMonen
Copy link

MicMonen commented Apr 3, 2023

Waiting on this as well.

@mattcompiles
Copy link
Contributor

@wsow4 Thanks for jumping in, I don't doubt you. Just without a reproduction to test against how can we say something has been resolved? If someone provides a reproduction of this issue we will definitely try get it resolved ASAP.

@wmertens
Copy link
Contributor Author

wmertens commented Apr 4, 2023

@mattcompiles Try changing the text color in https://stackblitz.com/edit/qwik-starter-r2tt19?file=src/routes/styles.css.ts - it only works when you restart the server

Note that it's not doing hot reload, just reload.

@mattcompiles
Copy link
Contributor

@wmertens Thanks bud. I'll take a look.

@askoufis
Copy link
Contributor

askoufis commented Apr 17, 2023

@mattcompiles Try changing the text color in https://stackblitz.com/edit/qwik-starter-r2tt19?file=src/routes/styles.css.ts - it only works when you restart the server

Note that it's not doing hot reload, just reload.

@wmertens Implementing the fix mentioned by @wsow4 where we invalidate all modules returned by server.moduleGraph.getModulesByFile(virtualId) results in at least a page reload with correct styles when updating a .css.ts file in your reproduction. I'll make a separate PR for that change.

I think the crux of the issue here is that the Vanilla Extract vite plugin handles HMR for vanilla modules, but it does not handle HMR for plain .css files, which are what's being emitted due to logic within the plugin that detects specific plugins (namely the qwik plugin).

If I'm understanding it correctly, you've effectively implemented HMR for emitted CSS. IMO this shouldn't be the responsibility of the Vanilla Extract vite plugin, but rather the qwik vite plugin. In fact, I'm assuming it already has some kind of support for CSS HMR already, so maybe what's needed is some extra logic to handle CSS emitted by vanilla.

@wmertens
Copy link
Contributor Author

@askoufis no, this PR doesn't implement CSS hot reload, it only re-parses the .css.ts file when it should be, but your solution is more elegant. Closing.

@wmertens wmertens closed this Apr 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants