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

CSS changes do not always correctly update using HMR #9216

Closed
1 task
cmalven opened this issue Nov 28, 2023 · 10 comments · Fixed by #9219
Closed
1 task

CSS changes do not always correctly update using HMR #9216

cmalven opened this issue Nov 28, 2023 · 10 comments · Fixed by #9219
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority) feat: hmr Related to HMR (scope) feat: styling Related to styles (scope)

Comments

@cmalven
Copy link

cmalven commented Nov 28, 2023

Astro Info

Astro                    v3.6.2
Node                     v18.17.0
System                   macOS (arm64)
Package Manager          npm
Output                   static
Adapter                  none
Integrations             none

If this issue only occurs in one browser, which browser is a problem?

No response

Describe the Bug

  • Start the development server and open the site in your browser
  • Open /src/layouts/Layout.astro
  • Update the background-color property in the styles

We're keeping track of a counter value using Javascript, loading a heavy image, and doing some heavy work in JS when the page loads. These are all here purely to demonstrate more clearly the improper behavior that is occurring around style updates.

Updating the color of the background (or any style change in this file) causes the counter to reset and often the image to reload, demonstrating that the styles aren't being applied purely via HMR injection.

This becomes especially noticeable on larger projects. While this demo focuses on refreshing styles within a single file, I've found Astro to generally be very hit-or-miss about when it will update styles via HMR and when it will refresh the entire page, and I haven't been able to track down anything consistent that determines when this will and won't happen.

Incidentally, I just tried using the Astro 4.0 beta and I see the same issue there.

What's the expected result?

CSS updates should occur without the page reloading or the current Javascript state for the page being reset.

Link to Minimal Reproducible Example

https://github.com/cmalven/astro-css-hmr

Participation

  • I am willing to submit a pull request for this issue.
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Nov 28, 2023
@lilnasy
Copy link
Contributor

lilnasy commented Nov 28, 2023

HMR for astro files is not supported. There is no straightforward way of implementing it considering that the astro templating language is server-render only, there is no "module" on the client to replace.

Closing as duplicate of #8916

@lilnasy lilnasy closed this as not planned Won't fix, can't repro, duplicate, stale Nov 28, 2023
@cmalven
Copy link
Author

cmalven commented Nov 28, 2023

Unless there is some confusion here, what I'm hearing is that HMR for CSS changes is not supported in Astro files? If accurate, that's a dealbreaker for me and represents a pretty significant downgrade in DX vs just about every other frontend framework. Correct me if I'm misunderstanding.

@cmalven
Copy link
Author

cmalven commented Nov 28, 2023

Also want to be clear here - this bug report is exclusively related to CSS changes in Astro files, not changes to HTML or <script>. For that reason, I don't see how this is a duplicate of #8916, which is specific to templating changes in .astro files.

@lilnasy
Copy link
Contributor

lilnasy commented Nov 28, 2023

That is correct, unfortunately. We could probably do CSS as you mention, but it would still be a new feature, which we track in roadmap discussions.

@cmalven
Copy link
Author

cmalven commented Nov 28, 2023

Thanks for the clarification @lilnasy. I really enjoy Astro otherwise and look forward to revisiting it if this ever happens.

@natemoo-re
Copy link
Member

natemoo-re commented Nov 28, 2023

I don't know, @lilnasy. We definitely supported live changes for <style> block inside .astro files at one point. I remember fixing a few issues around it.

I can reproduce this locally. It's possible that this was a regression that we just didn't catch until now.

For some additional context, we tend to be very quick to close HMR-related issues because they are typically a misunderstanding of what "HMR" actually entails. In this case, though, updates to styles in Astro files should be applied without reloading the page and this is a valid issue.

@natemoo-re natemoo-re reopened this Nov 28, 2023
@natemoo-re natemoo-re added feat: hmr Related to HMR (scope) - P4: important Violate documented behavior or significantly impacts performance (priority) feat: styling Related to styles (scope) labels Nov 28, 2023
@github-actions github-actions bot removed the needs triage Issue needs to be triaged label Nov 28, 2023
@cmalven
Copy link
Author

cmalven commented Nov 28, 2023

Thanks for looking into it @natemoo-re. I understand the desire to limit HMR issues, and I wasn't sure that was the most accurate terminology for what this is, but my issue is definitely specific to live-refreshing styles and not the entire component.

@natemoo-re
Copy link
Member

Thanks for your patience @cmalven, understood!

After a bit more digging...

I originally implemented this particular style HMR logic in #4125, which was released in [email protected]. It's possible that Vite changes overtime subtly broke our logic and our E2E HMR tests didn't catch them for some reason.

@natemoo-re
Copy link
Member

Ah ha! I think I found the source of the inconsistency.

If you check our HMR logic, we correctly detect when a change only changes a style, but we don't filter the affected modules properly.

if (isStyleOnlyChange && file === ctx.file) continue;

In this case, we're filtering out the Layout.astro file but not the index.astro page file, which causes Vite to reload the browser since it thinks the index.astro page needs to be re-rendered.

This fix is going to be better handling for style-only changes that actually crawl the module graph to determine which files actually need to be invalidated.

@cmalven
Copy link
Author

cmalven commented Dec 1, 2023

@natemoo-re Thank you so much for your work on this. Unfortunately, after this fix was released and I updated to the latest astro in my project, I found pretty quickly that the fix only seems to resolve a specific version of this situation. I can still trigger a page refresh if I make style-only changes in different .astro files one after another.

I've updated my reduced case repo to reflect this and created a new issue for it, but feel free to kill that one and reopen this one if it makes more sense.

#9262

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P4: important Violate documented behavior or significantly impacts performance (priority) feat: hmr Related to HMR (scope) feat: styling Related to styles (scope)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants