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

Imported CSS getting duplicated in dev server #5857

Closed
1 task
mayank99 opened this issue Jan 13, 2023 · 15 comments
Closed
1 task

Imported CSS getting duplicated in dev server #5857

mayank99 opened this issue Jan 13, 2023 · 15 comments
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: dev Related to `astro dev` CLI command (scope) feat: styling Related to styles (scope) requires refactor Bug, may take longer as fixing either requires refactors, breaking changes, or considering tradeoffs

Comments

@mayank99
Copy link
Contributor

mayank99 commented Jan 13, 2023

What version of astro are you using?

1.9.2

Are you using an SSR adapter? If so, which one?

None

What package manager are you using?

npm

What operating system are you using?

Windows / Stackblitz

Describe the Bug

Original description

When importing a css file (local or npm. @import or import. doesn't matter), the styles get duplicated anywhere from 2-4 times.

Doesn't seem to affect prod build, and doesn't seem to affect styles defined in the same file.

Possibly related issues: #5817, #5834, #5715

When importing a stylesheet into a cascade layer, the styles get incorrectly duplicated outside the layer. See comment below: #5857 (comment)

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-fkhp2c-kkhdqq

Participation

  • I am willing to submit a pull request for this issue.
@mayank99

This comment was marked as outdated.

@natemoo-re
Copy link
Member

The original issue here is fixed by #5917.

The @layer bug seems like an upstream Vite issue.

@mayank99
Copy link
Contributor Author

just for clarity, i did not touch the devSourcemaps option. maybe another e2e fixture without that option would help?

i can open an issue in vite for the layer issue, but i can't seem to reproduce it in vanilla vite

@gvkhna
Copy link

gvkhna commented Jan 20, 2023

just for clarity, i did not touch the devSourcemaps option. maybe another e2e fixture without that option would help?

i can open an issue in vite for the layer issue, but i can't seem to reproduce it in vanilla vite

I believe the issue here is that with devSourcemaps it was creating double styles, as in the style tag inline and also the style src referenced. Which caused an issue with layer directive because styles were duplicated in the page. Considering that, I don’t think it would be repro without devSourcemaps.

@mayank99
Copy link
Contributor Author

but it is reproducible in my two stackblitz links above

@gvkhna
Copy link

gvkhna commented Jan 20, 2023

O yes, that makes sense. Astro is doing the same thing irrespective of devSourcemaps I believe. But the issue would be fixed without double styles. That makes sense why you can’t reproduce in vanilla vite. I’m pretty sure it’s the same issue but I would have to take a closer look when this PR is merged, can you try with the PR?

@natemoo-re
Copy link
Member

natemoo-re commented Jan 21, 2023

just for clarity, i did not touch the devSourcemaps option. maybe another e2e fixture without that option would help?

Yep, I will add another test to confirm that your first repro doesn't regress.

i can open an issue in vite for the layer issue, but i can't seem to reproduce it in vanilla vite

For this one, I'm seeing some strange output that leads me to believe this is an upstream issue... But I suppose it's possible there's something happening on our end? I'll have to look into this more if you can't repro in plain Vite.

@layer A, B;
@media (A){
  @layer vite--anon-layer-4076b8fe-0{
    body {
      font-family: system-ui;
      display: grid;
      place-items: center;
      background-color: tomato;
    }
  }
}
@layer B {
  :where(body) {
    background-color: hotpink;
  }
}

@natemoo-re
Copy link
Member

OK, just looked into this again as I was getting #5917 ready to merge. It looks like this is a separate issue that has to do with how Astro inlines imports from styles, but the import remains in the module graph. Probably going to have to consult with @bluwy on how we can address this.

@mayank99
Copy link
Contributor Author

mayank99 commented Jan 23, 2023

@natemoo-re So, testing on 2.0.0-beta.4 (which contains #5917), i found that instead of duplicating 4 times, the css was duplicated 2 times (updated stackblitz).

that's an improvement! i'd guess the one duplication left is because of the module graph thing you mentioned.

when using layers, i dont see the duplication in dev (see updated stackblitz), but if i had to guess, it's because:
1. vite is producing an invalid rule (shown in your comment, happens in both dev/prod)
2. astro is stripping out the layer when doing its own import (in dev, not prod)

i will open an issue in vite for 1st point, since i understand how to repro it now.

@mayank99
Copy link
Contributor Author

update: actually the layer thing was partly because of invalid syntax on my end 🤦

there should be no space between layer and the parentheses:

- @import '../styles.css' layer (A);
+ @import '../styles.css' layer(A);

see final updated stackblitz: https://stackblitz.com/edit/github-fkhp2c-kkhdqq

the duplication happens only in dev now, not prod, and the layer gets stripped only in astro, not vite.

dev prod

sorry about all the confusion! let me know if i can clarify anything.

@matthewp
Copy link
Contributor

So what's going here is:

  1. We add <style> tags not just your Astro styles but for any imported dependencies, so that if that dependency changes it can receive an HMR update.
  2. Using @layer makes this not a real dependency, but we have no way of knowing that. This all relies on a postcss plugin that tracks dependencies, see https://www.npmjs.com/package/postcss-import#user-content-dependency-message-support

So unfortunately there isn't a way to really do what you'd like us to do and not treat this as a dependency.

Going to leave this open for now, but I don't see any possible solution here. We could revisit why we add <style> tags for dependency but that's such an old decision that I can't imagine going down that road.

@matthewp matthewp removed their assignment Feb 10, 2023
@mayank99
Copy link
Contributor Author

why are <style> tags added at all (even for non-layered imports)?

SSR/FOUC is not really a concern in the dev server, so i think vite should handle all of it. and prod seems to behave correctly after #5917

@lilnasy
Copy link
Contributor

lilnasy commented Nov 21, 2023

Issue still exists with [email protected]. Extra stylesheet highlighted in devtools.

image

@lilnasy lilnasy added feat: styling Related to styles (scope) feat: dev Related to `astro dev` CLI command (scope) labels Nov 21, 2023
@lilnasy lilnasy added the requires refactor Bug, may take longer as fixing either requires refactors, breaking changes, or considering tradeoffs label Nov 22, 2023
@ascorbic
Copy link
Contributor

ascorbic commented Oct 2, 2024

I can't reproduce this in [email protected], so I'll close this.
image

@ascorbic ascorbic closed this as completed Oct 2, 2024
@mayank99
Copy link
Contributor Author

mayank99 commented Oct 2, 2024

Huh, it does appear to be fixed in 4.15 at least in my initial testing, which is great news! 🚀

I'll test against some more repos and report back if there is still an issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: dev Related to `astro dev` CLI command (scope) feat: styling Related to styles (scope) requires refactor Bug, may take longer as fixing either requires refactors, breaking changes, or considering tradeoffs
Projects
None yet
Development

No branches or pull requests

6 participants