withastro / astro

The web framework for content-driven websites. ⭐️ Star to support our work!
https://astro.build
Other
45.38k stars 2.37k forks source link

Vite injected styles and Astro injected styles are conflicting #8637

Closed marceloverdijk closed 11 months ago

marceloverdijk commented 11 months ago

Astro Info

Astro                    v3.1.2
Node                     v18.17.1
System                   macOS (x64)
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

Yes I also noticed (partially) duplicated css.

E.g. I have a span like:

<span class="badge bg-primary bg-opacity-10 text-primary">Hotel</span>

and the generated css contains:

L28048:

.bg-primary {
  --bs-bg-opacity: 1;
  background-color: rgba(var(--bs-primary-rgb), var(--bs-bg-opacity)) !important;
}

L28116:

.bg-opacity-10 {
  --bs-bg-opacity: 0.1;
}

L34947:

.bg-primary {
  --bs-bg-opacity: 1;
  background-color: rgba(var(--bs-primary-rgb), var(--bs-bg-opacity)) !important;
}

The .bg-primary is for some reason duplicated, but the bg-opacity-10 not. The problem now is that the --bs-bg-opacity is overwritten again, causing a styling issue.

This new issue is created after discussion here with @bluwy

What's the expected result?

Correct - non duplicate - styling.

Link to Minimal Reproducible Example

https://github.com/marceloverdijk/astro-7508

Participation

github-actions[bot] commented 11 months ago

Hello @marceloverdijk. Please provide a minimal reproduction using a GitHub repository or StackBlitz. Issues marked with needs repro will be closed if they have no activity within 3 days.

sarvex commented 11 months ago

This is the GitHub repo https://github.com/sarvex/afraid-aurora containing the code

The problem is seen with the current NodeJS version 20.7. with LTS 18.18, the problem is not there

marceloverdijk commented 11 months ago

Thx @sarvex for sharing you repo. Note that I used Node v18.17.1 and with that version I had the issue as well.

sarvex commented 11 months ago

@marceloverdijk there is something weird going on... The problem shows up on the local windows machine but is absent under WSL Ubuntu. The problem does not show up on Stackblitz

marceloverdijk commented 11 months ago

On my side as well. This particular example mentioned above happens for me in 1 project but in another project. So it is very unclear where it is coming from to be honest.

@bluwy also noticed this behavior (see discussion in #7508 )

marceloverdijk commented 11 months ago

I also found this issue: https://github.com/withastro/astro/issues/6975 Where @matthewp mentions Closing as ordering issues are not fixable. Order can not be guaranteed after bundling. Isn't this scary as order in css output is important...

ematipico commented 11 months ago

I also found this issue: https://github.com/withastro/astro/issues/6975 Where @matthewp mentions Closing as ordering issues are not fixable. Order can not be guaranteed after bundling. Isn't this scary as order in css output is important...

That's exactly what it is, and it's best to understand the underlying problem and why the order isn't fixable today.

As you said, in CSS land, order is important. Now, let's say you have a component that imports a style. Then this component is imported in two pages, in one page is imported at the top of the import statements, and in the other page is imported at the bottom of the import statements. The bundler doesn't know the priority of the styles attached to the component, because its order is different in the two pages.

Myself and Matthew talked about it a lot, and as for today, this is an issue that is fixible by end users by understanding the order of the styles. We also talked about introducing some new configuration to mitigate the problem, although fixing isn't as easy.

marceloverdijk commented 11 months ago

I'm trying to understand. And yes if multiple components ordering is probably diffcult. Looking at my setup however, I don't have any component styles.

I have just 1 base Layout used by all pages, and that Layout does the scss include by:

---
<other scss imports from third party libs>
import '@scss/style.scss' <-- my custom theme importing bootstrap etc.
marceloverdijk commented 11 months ago

As you said, in CSS land, order is important. Now, let's say you have a component that imports a style. Then this component is imported in two pages, in one page is imported at the top of the import statements, and in the other page is imported at the bottom of the import statements. The bundler doesn't know the priority of the styles attached to the component, because its order is different in the two pages.

Thinking about a bit more, I wonder if there would be a way to bypass the mechanism importing styles in some way. What I mean is I don't have any component styles. I have just 1 single "big" global scss included in my Layout component, that includes other scss files like Bootstrap. Maybe it's not optimal as there might be to much css for every page, but at the end the single downloaded css file is cached on the client anyway for subsequent page requests. But probably this make no sense in Astro world, but I'm thinking just out loud for possibilities for some use case to avoid this problem...

matthewp commented 11 months ago

@marceloverdijk I checked out your example and I don't see bg-primary defined anywhere. Or bg-opacity-10 or the other classes you are referring to. It looks like the standard blog template to me. Did you forget to update it?

marceloverdijk commented 11 months ago

It's indeed a different example from @sarvex .

This is the original repo I had:

https://github.com/marceloverdijk/astro-7508

This should contain these classes. I'm typing this on my phone now so cannot verify behind my computer. Otherwise I can setup a new example tomorrow if needed.

bluwy commented 11 months ago

I'm able to fix the duplicated styles @marceloverdijk, but it doesn't seem to affect the spacing issue you mentioned before. I guess there's something affecting it but I'm not sure what.

@sarvex I can't seem to see any duplicated styles in your repro. I thinking it's a separate issue if it's only happening on specific machines. Maybe you can try after this issue's fixed as I refactored CSS and HMR overall.

marceloverdijk commented 11 months ago

Thx @bluwy I will try it out asap when new release is available 👍

marceloverdijk commented 11 months ago

Hi @bluwy is there a npm tag or something to try latest from git? Or do you know when this will be released?

bluwy commented 11 months ago

Just released it on Astro v3.2.1 👍

marceloverdijk commented 11 months ago

I will give it a try this evening and will let you know here.

marceloverdijk commented 11 months ago

Interestingly, I cannot reproduce the issue with 3.2.2 with slimmed down example repo (https://github.com/marceloverdijk/astro-7508), but in my actual project I still have that duplicated .bg-primary overriding the opacity.

Although the setup between 2 projects is similar (and using the same theme scss files) I'm will do further comparisons on my side to try to identify what is causing this.

marceloverdijk commented 11 months ago

Found it and fixed it in the 7508 sample project as well. I think it was a wrong way of (re)-importing custom Bootstrap utilities. But it could also be the way the (external) theme I'm using is setup to customize Bootstrap, as it is complete different as documented here. Anyway not related to Astro, so sorry for confusion and thanks for all the help. On the good side, Astro was made better by fixing some issues along the way.

I will also look into the spacing issue as I'm wondering where that is coming from. Will post updates here.

marceloverdijk commented 11 months ago

@bluwy just for your information I found the origin of the spacing issue.

When having html like:

<div class="bd-example m-0 border-0">
  <span class="badge text-bg-primary">Primary</span>
  <span class="badge text-bg-secondary">Secondary</span>
  <span class="badge text-bg-success">Success</span>
  <span class="badge text-bg-danger">Danger</span>
  <span class="badge text-bg-warning">Warning</span>
  <span class="badge text-bg-info">Info</span>
  <span class="badge text-bg-light">Light</span>
  <span class="badge text-bg-dark">Dark</span>
</div>

there is linebreak/whitespace in front of these badges.

As Astro removes these whitespace in the output the spacing is not there anymore.

So instead of e.g. <span class="badge text-bg-primary">Primary</span> I should do <span class="badge text-bg-primary me-1">Primary</span> to add some margin using Bootstrap classes.

bluwy commented 11 months ago

Thanks for the updates @marceloverdijk! These are useful information and glad you got it sorted out.

As Astro removes these whitespace in the output the spacing is not there anymore.

Ah now that you brought it up I remember why now. In Astro 3.0, we have enabled compressHTML by default. If I disable that manually, I'm able to get the whitespace. It's an option in astro.config.js that you can change.

I think that's an intentional change on our end, but it's still good to add the additional CSS for spacing too to futureproof it.