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

Vite injected styles and Astro injected styles are conflicting #8637

Closed
1 task
marceloverdijk opened this issue Sep 22, 2023 · 20 comments · Fixed by #8706
Closed
1 task

Vite injected styles and Astro injected styles are conflicting #8637

marceloverdijk opened this issue Sep 22, 2023 · 20 comments · Fixed by #8706
Assignees
Labels
- P3: minor bug An edge case that only affects very specific usage (priority) feat: hmr Related to HMR (scope)

Comments

@marceloverdijk
Copy link

marceloverdijk commented Sep 22, 2023

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

  • 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 Sep 22, 2023
@matthewp matthewp added needs repro Issue needs a reproduction and removed needs triage Issue needs to be triaged labels Sep 22, 2023
@github-actions
Copy link
Contributor

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
Copy link

sarvex commented Sep 24, 2023

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
Copy link
Author

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

@bluwy bluwy added needs triage Issue needs to be triaged and removed needs repro Issue needs a reproduction labels Sep 25, 2023
@sarvex
Copy link

sarvex commented Sep 26, 2023

@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
Copy link
Author

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
Copy link
Author

I also found this issue: #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
Copy link
Member

ematipico commented Sep 26, 2023

I also found this issue: #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.

@bluwy bluwy self-assigned this Sep 26, 2023
@marceloverdijk
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

@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?

@matthewp matthewp added the needs response Issue needs response from OP label Sep 27, 2023
@marceloverdijk
Copy link
Author

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 bluwy added - P3: minor bug An edge case that only affects very specific usage (priority) feat: hmr Related to HMR (scope) and removed needs response Issue needs response from OP needs triage Issue needs to be triaged labels Sep 29, 2023
@bluwy
Copy link
Member

bluwy commented Sep 29, 2023

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
Copy link
Author

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

@marceloverdijk
Copy link
Author

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

@bluwy
Copy link
Member

bluwy commented Oct 2, 2023

Just released it on Astro v3.2.1 👍

@marceloverdijk
Copy link
Author

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

@marceloverdijk
Copy link
Author

marceloverdijk commented Oct 2, 2023

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
Copy link
Author

marceloverdijk commented Oct 2, 2023

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
Copy link
Author

@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
Copy link
Member

bluwy commented Oct 3, 2023

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.

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: hmr Related to HMR (scope)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants