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

Style-only changes in .astro files don't use HMR consistently #9262

Closed
1 task
cmalven opened this issue Dec 1, 2023 · 8 comments · Fixed by #10166
Closed
1 task

Style-only changes in .astro files don't use HMR consistently #9262

cmalven opened this issue Dec 1, 2023 · 8 comments · Fixed by #10166
Assignees
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 Dec 1, 2023

Astro Info

Astro                    v3.6.4
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

Related to #9216

Normally, if I make a style-only change in a .astro file (such as changing a background-color property and save the file, the styles in the page will update without a refresh.

However, if I make a style change in one file, and then make another style change in another file, that second change will cause a refresh, and so on for each consecutive change in a different file.

You can see this in the linked repo by doing the following:

  • Change the background-color in /src/layouts/Layout.astro and save
  • Change the color in /src/pages/index.astro and save
  • Page will refresh

IMPORTANT: This issue is not about full HRM support in Astro - it applies only to style-only changes in a .astro file

What's the expected result?

Any number of style-only changes in .astro files - in any order - should not cause a full page refresh.

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 Dec 1, 2023
@natemoo-re
Copy link
Member

Thanks for opening a new issue and continuing to provide great reproductions! I will look into this one.

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

I've started work on refactoring our HMR logic to make this behavior much more consistent!

#9356

@bluwy bluwy self-assigned this Jan 11, 2024
@bluwy
Copy link
Member

bluwy commented Jan 17, 2024

I can confirm that this is fixed by #9706 now, but I'll continue to work on Nate's improvements so we get better style change detection. For now I'll close this as fixed.

@bluwy bluwy closed this as completed Jan 17, 2024
@linear linear bot reopened this Jan 17, 2024
@github-actions github-actions bot added the needs triage Issue needs to be triaged label Jan 17, 2024
@bluwy bluwy closed this as completed Jan 17, 2024
@cmalven
Copy link
Author

cmalven commented Jan 19, 2024

Thanks all for your work on this. I just tested using the reduced test case repo I created for this (https://github.com/cmalven/astro-css-hmr) with an updated version of Astro and the immediate problem was indeed fixed, but now I've discovered on another project that any updates made in imported, external .css files will still trigger a full refresh. Not sure if this is related or not - I can create a new issue if necessary. To be clear, I'm not saying this was caused by the Astro updates - it more likely has been the case for awhile and just wasn't fixed by these latest updates.

I've updated the linked repo to demonstrate the issue with external files.

@bluwy
Copy link
Member

bluwy commented Jan 22, 2024

No problem @cmalven! This does seem like an issue still so I'll re-open it. Will take a look when possible

@bluwy bluwy reopened this Jan 22, 2024
@Gazook89
Copy link

I chatted via discord (https://discord.com/channels/830184174198718474/1198010724753100933/1199102951248109698) about an issue with style changes in .astro components that I was hoping would be fixed by #9712, but it doesn't seem like that fixed the issue. I think this thread is likely related, but let me know if you think I should open a new Issue.

Before #9712, when changing styles of an .astro component, astro would not trigger a refresh in the preview. As an easy workaround, I could make my style change and add a small change to the component itself, such as by adding another html node, and then saving. This would refresh the page with both the new node and the style change.

Now, after updating to 4.2.2 (post #9712), if I do the same steps above the page refreshes but only adds the new node-- it does not reflect the style change. I can only get a refresh with the new style by shutting the dev server down and starting it again.

I don't have (or haven't noticed) any issue with normal .css or .less files, or even any issue with .astro files in /layouts/ or other places-- only .astro files in my /components/ folder.

@lilnasy lilnasy removed the needs triage Issue needs to be triaged label Jan 23, 2024
@bluwy
Copy link
Member

bluwy commented Jan 24, 2024

@cmalven it seems like that's caused by #9712, the only way to reliably fix it I think is to revert the PR, or wait till Vite 4.1 releases which should give us the feature we need to isolate CSS updates.

@Gazook89 I can't seem to reproduce that. The styles under components/ are working for me. Can you provide a repro (e.g. https://astro.new) to debug further?

Trying to judge the severity of the issue before reverting the PR, but if it's not too bad, it might be worth waiting for Vite 4.1 to release first (in around 1 week), so we can get a more robust HMR altogether.

@cmalven
Copy link
Author

cmalven commented Jan 24, 2024

Thanks @bluwy. For what it's worth on my end, I don't know that external CSS files ever seemed to reliably trigger an HMR update, even before #9712 but at least now CSS updates within .astro files seem to update correctly. So I guess I'd be in favor of keeping #9712 until Vite 4.1 can help properly resolve things.

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
5 participants