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

Fix duplicated Astro and Vite injected styles #8706

Merged
merged 2 commits into from
Sep 29, 2023
Merged

Conversation

bluwy
Copy link
Member

@bluwy bluwy commented Sep 29, 2023

Changes

Fix #8637

The issue was that <style data-astro-dev-id="..." and <style data-vite-dev-id="..." have different IDs that they don't match and deduplicate correctly, causing duplicated styles. The main issue is that we're using importedModule.url instead of importedModule.id.

But I made a complete cleanup of CSS HMR, relying on Vite to handle it entirely. A lot of HMR handling was added by #4157 before. It's possible to take this new approach today because Vite natively treats data-vite-dev-id as FOUC-prevention injected styles.

Testing

Existing CSS and CSS HMR tests should still work. Updated them where necessary for the slightly different markup. I also tested the #8637 repro manually.

Docs

n/a. internal change and bug fix.

@changeset-bot
Copy link

changeset-bot bot commented Sep 29, 2023

🦋 Changeset detected

Latest commit: d07e2c8

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

@github-actions github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Sep 29, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Sep 29, 2023

⚖️ Bundle Size Check

Latest commit: d07e2c8

File Old Size New Size Change
hmr 765 B 40 B - NaN undefined

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test file seems to duplicate the same test in css.test.js

const totalExpectedStyles = 8;
const totalExpectedStyles = 7;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no more <style data-astro-dev-id as it's merged into <style data-vite-dev-id, so this number is reduced by one.

Comment on lines -19 to -20
// Vue `link` styles need to be manually refreshed in Firefox
import.meta.hot.on('vite:beforeUpdate', async (payload) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I manually tested this and it seems to not apply anymore, so I removed it. The rest of this file relates to our CSS handling, which is no longer needed as Vite handles all now, so pretty much the entire file is empty and not need to be loaded anymore.

Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing, thanks!

@bluwy bluwy merged commit 3458081 into main Sep 29, 2023
14 checks passed
@bluwy bluwy deleted the fix-duplicated-styles branch September 29, 2023 13:55
@astrobot-houston astrobot-houston mentioned this pull request Sep 29, 2023
@bluwy bluwy mentioned this pull request Oct 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: astro Related to the core `astro` package (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vite injected styles and Astro injected styles are conflicting
2 participants