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

SFC style ordering #808

Closed
Justineo opened this issue May 9, 2017 · 30 comments
Closed

SFC style ordering #808

Justineo opened this issue May 9, 2017 · 30 comments

Comments

@Justineo
Copy link
Member

Justineo commented May 9, 2017

What problem does this feature solve?

Let's say we have two components:

<!-- MyButton.vue -->
<template>
  <button class="my-button" @click="$emit('click')"><slot></slot></button>
</template>

<style>
.my-button {
  color: #666;
}
</style>
<!-- AlertButton.vue -->
<template>
  <my-button class="alert-button" @click="$emit('click')"><slot></slot></my-button>
</template>

<style>
.alert-button {
  color: #c00;
}
</style>

<script>
import MyButton from './MyButton.vue'
export default {
  components: {
    MyButton
  }
}
</script>

We've made a wrapper component called AlertButton to "extend" the original MyButton component and we intend to make its text color red. But as a result the style of AlertButton comes earlier than MyButton now using vue-loader.

IMO styles of "parent" components should have higher precedence than their children (or "superclass" in inheritance), just like that when we compose a webpage with HTML elements, whose default styles comes from UA styles and have lower precedence than author styles (the styles of the "parent component"). So I think the order of the result CSS should be MyButton - AlertButton instead of AlertButton - MyButton (which is the current behavior of vue-loader).

What does the proposed API look like?

Not proposing API update yet.

@LinusBorg
Copy link
Member

LinusBorg commented May 16, 2017

Hi @Justineo

I understand your problem, but I'm afraid the proposed solution will be next to impossible to realize.

You see, in order to be able to inject styles of the parent components before the styles of the child, vule-loader would have to read the component's code:

  • parse the javascript code of the component,
  • walk the resulting AST and check weither the child is in fact extending some other component.
  • do that for the whole app (while skipping style processing/injection) and generating some sort of dependecy graph for the extends,
  • and afterwards, calculate the order that the styles should be injected in, and process those styles.

@Justineo
Copy link
Member Author

I think the logic is not that complicated. Just ensure we import style of current component after all other imports like this:

// AlertButton.js

import MyButton from './MyButton.vue' // 'MyButton.css' is imported inside this
import './AlertButton.css' // Insert styles of current component after all other imports

export default {
  components: {
    MyButton
  }
}

It doesn't really matter whether we are actually "extending" components. It's just keeping the way CSS handles imported styles: always import other styles first (@import directives must be at the top of a CSS file, only after @charset).

@yyx990803
Copy link
Member

This would indeed be difficult because:

  • In 12.0 the css injection is a bit different now. Styles are now injected in the beforeCreate hook of the component when it's rendered for the first time.

  • If using style extraction, extract-text-plugin seems to align the styles in the order of the import statements are parsed - basically even if we move style import statements to be below child component imports, the resulting extracted CSS would still be in current order.

  • This would also be a breaking change.

Still I think relying on CSS appearing order is probably not that good an idea in the first place...

@Justineo
Copy link
Member Author

Justineo commented May 24, 2017

If we do not rely on order, we'll have to increase specificity to override styles. I don't think that's a good idea either...

@Justineo
Copy link
Member Author

Justineo commented May 25, 2017

And the breaking change is not required. We can provide a new option if possible.

I understand that the implementation is not trivial in current design but still think the order is important if we don't want to increase specificity or use inline styles.

@Jinjiang
Copy link
Member

Well, I also think it's really difficult but I have tried some different ways to do this and probably found a solution, that is:

  1. In vue-loader inject a special "empty" stylesheet by an option for each component as an additional <style> part at last.
  2. In vue-style-loader/lib/addStylesClient.js detect the special stylesheet and change the function createStyleElement() into like:
    function createStyleElement (isLastStyleOfComponent) {
      var styleElement = document.createElement('style')
      styleElement.type = 'text/css'
      if (!currentStyleBeginning) {
        currentStyleBeginning = styleElement
      }
      if (isLastStyleOfComponent) {
        lastStyleBeginning = currentStyleBeginning
        currentStyleBeginning = null
      } else {
        head.insertBefore(styleElement, lastStyleBeginning)
      }
      return styleElement
    }

So the CSS order is finally right.

To be continued, maybe we could:

  1. Do the similar hack for old IE and SSR (I guess they also work, I haven't tried yet)
  2. Fork a new extract-text-webpack-plugin to hack the same effect.
  3. Take a note about this hack for 3rd-party customized extractor.

Any ideas of that?

Thanks.

@trainiac
Copy link

trainiac commented Dec 1, 2017

@Jinjiang I think there needs to be a little more discussion on what exactly the preferred CSS order is.

Here is the current state of things:

<!-- MyButton component -->
<template>
  <button :class='$style.container'>
    <slot/>
  </button>
</template>

<style module>
  .container {
    width: 100%;
  }
</style>
<!-- JoinPage component -->
<template>
  <div :class='$style.container'>
    <MyButton :class='$style.btn'>
      Join
    </MyButton>
  </div>
</template>

<style module>
  .container {
    width: 600px;
    margin: 0 auto;
  }

  .btn {
    width: 80%;
  }
</style>
<!-- LoginPage component -->
<template>
  <div :class='$style.container'>
    <MyButton :class='$style.btn'>
      Login
    </MyButton>
  </div>
</template>

<style module>
  .container {
    width: 500px;
    margin: 0 auto;
  }

  .btn {
    width: 50%;
  }
</style>
<!-- Root component -->
<template>
  <div>
    <LoginPage :class='$style.login'/>
    <JoinPage :class='$style.join'/>
  </div>
</template>

<style module>
  .login {
    margin-top: 10px;
  }

  .join {
    margin-top: 8px;
  }
</style>
/* output */
.Root_login {
  margin-top: 10px; /* wont work because overridden below */
}

.Root_join {
  margin-top: 8px; /* wont work because overridden below */
}

.LoginPage_container {
  width: 500px;
  margin: 0 auto;  
}

.LoginPage_btn {
  width: 50%; /* wont work because overridden below */
}

.MyButton_container {
  width: 100%;
}

.JoinPage_container {
  width: 600px;
  margin: 0 auto;  
}

.JoinPage_btn {
  width: 80%; /* will work because comes after */
}

Of course you could solve this problem by always using more specificity in the parent but this is not an ideal pattern for several reasons:

  • File size bloat
  • Requires developers to remember to create a more specific rule each time they add a class to a child component
  • Reading the code it's hard to understand why a more specific CSS rule is being created vs just defining a standalone class.

So if we could use order to solve these problems what is the ideal order?

From what I can see, the order of the css classes currently is collected as follows:

Root
Root, LoginPage
Root, LoginPage, MyButton
Root, LoginPage, MyButton, JoinPage

I think ideally the order should be:

Root
LoginPage, Root
MyButton, LoginPage, Root
JoinPage, MyButton, LoginPage, Root
MyButton, JoinPage, LoginPage, Root

So we are still traversing the render tree the same way but the difference is instead of appending classes, prepend and if you come across a component that you have already seen move it to the beginning of the list.

/* output */
.MyButton_container {
  width: 100%;
}

.JoinPage_container {
  width: 600px;
  margin: 0 auto;  
}

.JoinPage_btn {
  width: 80%; /* still works */
}
.LoginPage_container {
  width: 500px;
  margin: 0 auto;  
}

.LoginPage_btn {
  width: 50%; /* fixed */
}

.Root_login {
  margin-top: 10px; /* fixed */
}

.Root_join {
  margin-top: 8px; /* fixed */
}

I can see one problem with this strategy which is If you add a scenario where componentA is parent of componentB in one case and componentB is parent of componentA in another part of the render tree but it's hard for me to think of an example like that.

What do you guys think?

@LinusBorg
Copy link
Member

LinusBorg commented Dec 1, 2017

Well, I think generally speaking, the behaviour should be consistent between dev (using style-loader for HMR & faster builds) and producton (extracting CSS into one file). And since I believe we can't do much about the order that ExtractTextPlugin creates, I think the best thing would be to stick to that.

However, there are other things to consider as well:

  • Specific advantages of lazy insertion? ( @yyx990803 Why did you add the lazy insertion? What's the advantage? relevant for SSR?)
  • Speaking of SSR: inserting "critical CSS" into the generated HTML will always result in different order of CSS depending on the route. That might not be a problem for tightly coupled components that would appear in the same order anyway, but more distant ones could also be messed up.
  • Code-splitting will also lead to different insertion order depending on the setup:
    • If we have extracted all CSS with allChunks: true, CSS of codesplit bundles will be in the main css file, so the order would be the same as without code-splitting
    • without that option, it will be inserted by style-loader at the end of <head>, when the chunk has been loaded - a similar effect as for critical CSS.
    • so if you start with the first option and find your CSS is growing out of proportion, switching to CSS that's dynamically inserted with the second option could also break things, and that's not really fixable, maybe until CSS becomes a first-class module type in webpack 4.
  • Relying on insertion order can generally also lead to bugs when you have to use a component in a different, "earlier" place than where you previously had imported it.

So my opinion is pretty much this:

  1. Unless lazy insertion has a specific advantage that I am missing, I would prefer to disable it (optionally, to be backwards-compatible - though the last switch to this lazy insertion was a brekaing change we accepted as well).
  2. Since this alone won't guarantee insertion order in all likely scenarios, this would not be a "fix" for all problems, though. So generally I think that:
    2.1 Write components in a way that *doesn't rely on insertion order, and use higher specificity instead. Since we can scope styles per component, we can keep specificity very low in components themselves, so increasing specificity by 1 to re-style a child component in specific sitatiuons won't lead to overly specific selectors.
    2.2 if you truly want to / have to guarantee insertion order, put that CSS in one file.

@trainiac
Copy link

trainiac commented Dec 1, 2017

@LinusBorg I see your point and I think the most convincing argument is the route code splitting issue. I'm officially convinced you can't rely on css order output.

So let's change the discussion to formalizing how you add classes to children components. I agree that it must be done with specificity and I would go even further to say that it's recommended that you always add the specificity even in the case that it's not needed to avoid bugs that can creep in by changing where a component appears (predictability over efficiency). It would be great if there was a Vue community standardized pattern for passing classes to children. There are two ways of doing this. First, via the standard class property and second, via a custom property so that the child component can decide where to put the class internally (admittedly a slot is usually a better solution, sometimes this is better).

<template>
   <child 
     :class='$style.overrideContainer' 
     :innerClass='$style.overrideInnerElement'  <!-- a prop defined by the child component -->
     data-child-styles <!-- attribute to help with specifity -->
   />
</template>

<style module>
  .overrideContainer[data-child-styles] {
   
  }

  [data-child-styles] .overrideInner {
   
  }
</style>

<!-- or with sass -->
<style lang="scss" module>
  [data-child-styles] {
     &.overrideContainer  {
     }
     .overrideInner {
     }
  }
</style>

@Jinjiang
Copy link
Member

Jinjiang commented Dec 2, 2017

@trainiac yes I realized my test doesn't work correctly in all scenarios (which is including your case and SSR / hot reload etc.). I will hold it on for a while.

@LinusBorg actually I definitely wish to make it work because imo the current status is unnatural. People write CSS as usual but get the unexpected order results. And always raising up specificities in sub components will make a large project hard to maintain. 🤔

But if it's hard to do. I suggest to take a note in docs about this. And the standardized pattern I suggest is putting global styles in one file and using scoped style in components by default.

Thanks.

@NoRabbit
Copy link

NoRabbit commented Dec 7, 2017

Hi all, I just got dispatched here as I have the same problem.
I understand this is not trivial to solve for everyone's case, but here is my proposal:

let the dev choose the order, like so:

<style order="1"> </style>

I don't know how styles are collected but if it's possible to reorder them this way, this would allow for total control over how styles are being applied.

@trainiac
Copy link

trainiac commented Dec 7, 2017

I dont want to speak for @LinusBorg but I think what he is trying to say is even if you had complete control over the order styles, there are scenarios where chunks of your code will not be loaded in the same order. Imagine the hot module reload (HMR) case and the Route code splitting case.

  1. HMR - With HMR, if you update styles, the new styles will get appended to the head of the DOM potentially changing the intended order of your styles.

  2. Route code splittling - If you have an application that is split on routes and you want to only load in the CSS for a given route, your css will not be inserted in the same order for every user because it depends on which route they hit first.

Assuming you aren't using HMR or route code splitting, this can be solved. I think the solution that I proposed above is actually the most natural way to order the styles.

I think there are a number of things in the way of being able to do that but I believe it should be the default experience. For users you dont use HMR or Route code splitting it would be a much more intuitive experience. However, if you use HMR or Route code splitting you will still have to deal with load order but perhaps that problem could be solved later down the road. It might be possible for vue-style-loader (to address HMR) and vue-server-renderer plugins (to address Route code splitting) to inject styles into the page in a predictable order.

@NoRabbit
Copy link

NoRabbit commented Dec 7, 2017

ok right... so just throwing ideas: imagine we can order things the way we want in the loader (with the idea I proposed for example), then it's "just" a matter of how HMR and the Route code splitting append the styles in the dom, isn't it ?
They will then need to append them respecting the right original order, instead of just appending to the end of the list. When adding styles to the dom, they need to put a custom attribute with the order as value and that's it... they know where to put the new styles... Or has the browser already registered the previous styles, no matter where the new styles are appended, it won't affect the previous ones ? or the opposite, the new ones will prevail (even if pushed at the right "index") over the previous ones ? in that case, then we need to reset all the styles and reapply them all with the right order... shitty but yeah...

@trainiac
Copy link

trainiac commented Dec 8, 2017

I'm saying why even specify an order at all? Have you read my suggested styles ordering algorithm above? If all of your components had their styles ordered that way you wouldn't need to explain the order to your build tool it would just do the right thing for everyone.

@LinusBorg I took a crack at code splitting css files awhile back, but I wasn't actually able to get it work and according to webpack it's actually not a feature currently. Is this what you were referring to when talking about route code splitting for your styles? If so, how do you do it today?

@trainiac
Copy link

trainiac commented Dec 8, 2017

Nevermind I see that PR is about generating a CSS file per split point and today it inlines CSS with the split js file. Pretty cool 👍

@FossPrime
Copy link

FossPrime commented Dec 13, 2017

I use vue-material throughout a project, the buttons come with an annoying text-transform: uppercase;. For some reason it's particularly sticky on the a tag version that comes when you pass href.

I've ran into this a couple times with different projects, you don't find out until it's near production, which is traumatizing.

Is the current guidance to

  • use important! everywhere
  • use the highest specificity possible
  • duplicate your code on every component's scoped style

screen shot 2017-12-13 at 1 33 31 pm

@trainiac
Copy link

@rayfoss First off, I totally agree that this aspect of how styles are generated is a Major issue. The fact that you can't detect that you have a CSS specificity bug until production runtime is the stuff of nightmares.

I'm working on a big application and am working out some guidelines that I'd be happy to share here but I think there might not be a silver bullet. There are a number of application decisions that determine the how one has to address specificity:

  • How do you load CSS into your page? Hot module reloading, CSS code splitting on routes, one file, etc.
  • How do you scope your component styles? scoped vs modules
  • How do you import styles? import sass in the style section, use css modules composes in the style section, import css into the script section via css modules
  • Do you apply classes to a components container element within the component and also from parent components?
  • Do you pass classes into child components from your parent components?

I don't think any of these decisions individually are a definite no-no. I think there needs to be a document written up that is working body of knowledge of what the implications of these decisions are with regards to CSS specificity and how one might work around them. I'd like to see if someone has already written it, if not maybe I'll take a crack.

@ankitsinghaniyaz
Copy link

ankitsinghaniyaz commented Jan 6, 2018

I have an easy solution that works. I know people know this already, just adding it as it solves my immediate issue. As we always have an id of app on the main component, so prefixing the css with #app will give them a higher priority.

Something like this:

#app .login-btn {
  // some css here
}

This can get repetitive very easily. I mostly use SCSS/SASS so I do something like:

#app {
  .login-btn {
  }
}

Now we don't need to remember to add #app every time. In fact, this is kind of progressive as vue-loaders comes up with a solution, it will just be a matter of removing the top nesting.

Update
I also had a good idea about how vue-loader can fix(hack) this easily. The style tag can have an additional property like <style lang="css" scoped specific></style> this specific tag will automatically add the #app specificity before all the css.

@FossPrime
Copy link

FossPrime commented Jan 6, 2018 via email

@ankitsinghaniyaz
Copy link

ankitsinghaniyaz commented Jan 6, 2018

@rayfoss I'm not sure if the specificity thing can be done on a case by case basis. This will most importantly mean I do some kind of manual testing to identify the problem first, which will be very nasty.

I also completely agree that there is nothing better than predictable ordering. 👍

On using inline CSS, this is a controversy which we would not want to get into. Yes, we can do that, but Vue single file component provides a style section which feels elegant to me, but doing inline is also fine if someone prefers.

@FossPrime
Copy link

FossPrime commented Jan 6, 2018 via email

@trainiac
Copy link

trainiac commented Jan 8, 2018

@ankitsinghaniyaz I think the #app solution is an elegant way to address most of the problem, thank you for sharing.

What if you add a specific attribute to the app container element specifically for this purpose (e.g. [x])? This would de-couple the app id from it's other uses?

I think for vue-loader to solve this problem would be extremely difficult given the points made above.

I think 99% of problems can be addressed if your app follows this convention.

If you are applying a class to a child component always prefix it with #app (or some attribute)

Pros

  • Parent styles will always override child styles.
  • #app is a small name so good for file size
  • Doesn't require editing current component (i.e. changing html to add a new class or attribute for specificity sake)
  • It's readable. When you see #app .className you know that it's there to ensure specificity over child.
  • As @ankitsinghaniyaz says, it's progressive in that if vue-loader ever does fix this problem you can simply remove #app prefixes everywhere.

Cons

  • This is something that you must remember to do and there is no linting tool at the moment that's going to tell you that aren't doing it.
  • There may be cases where your parent styles don't clash with your child styles, making the #app prefix unnecessary page weight, but I think it's better to have deterministic results vs saving max 1KB.
  • This breaks down when a parent and grand-parent want to apply a class to a child. Because both grand-parent and parent will use #app .className, the grand-parent would need to be even more specific. I think this is really rare but worth noting.

Examples

<template>
  <!-- modules example -->
  <div :class='$style.container'>
    <child-component :class='$style.someClass' :innerClass='$style.anotherClass' />
  </div>
</template>

<style lang="scss" module>
   .container {
     /*styles*/
  }
 :global(#app) {
    .someClass {
       /*styles*/
    }
    .anotherClass {
       /*styles*/ 
    }
  }

</style>
<template>
  <child-component class='someClass' />  
</template>

<style scoped>
  .container {
     /*styles*/
  }
  #app .someClass {
     /*styles*/
  }
</style>

@FossPrime
Copy link

FossPrime commented Apr 17, 2018

We should perhaps change the label from improvement to "Regression" as CSS order between production and dev environments used to not be a problem when CSS was used more directly.

I'm now having issues with Vue material base css, theme css, my own global css and component CSS moving around between Dev and Production. The #app trick won't work on CSS that I don't control.

Anyone know how to nudge the order of imported (vue material) css above to be loaded above template CSS (global and component)? Update: I just imported it with app.vue instead of main.js, which allowed me to better control the order.

@LinusBorg
Copy link
Member

Small PSA: we will revert to the old behaviour in vue-loader 15, which is already in beta and will be release shortly.

@FossPrime
Copy link

FossPrime commented Jun 28, 2018

<style lang="scss" scoped>
#dfg8dfsf9gsd8fg98sdf98gs9 {
ul {
...

Id's have really high specificity... why not have loader enclose all scoped css in a random ID of the root?

btw, im using "^15.2.4" now my vue-material sass is below my scoped component styles, causing them to take priority.

chenxeed added a commit to chenxeed/chenxeed that referenced this issue Jul 15, 2018
- move the main css initialize to app.vue
(as suggested on vuejs/vue-loader#808)
@adi-zz
Copy link

adi-zz commented Oct 25, 2018

@LinusBorg, when you say the old behaviour will be reverted in vue-loader 15, it means the initial issue reported by OP won't happen anymore? And this revert was done in vue-loader 15? I saw you said it was reverted, but I'm asking because I use vue-loader 15 and I still have an issue which seems to me identical to OP's...

@LinusBorg
Copy link
Member

The Code that caused this issue was reverted, yes.

Your issue likely has some other cause.

@mattaningram
Copy link

I'm a little confused at the logic here, and I'm worried the reversion broke a lot of my styles but I'm not sure.

Not long ago when I added a global style to a child component, I knew its styles would override the parent's styles even if they had the same specificity because the <style> object would come after. This makes sense because children come after parents. But now all of a sudden my parent styles are sometimes overriding my child styles because the <style>s are all out of order.

Can someone explain what the issue was with child styles coming after parent styles and overriding them? That makes so much more sense.

@sodatea sodatea reopened this Jan 13, 2019
@sodatea sodatea closed this as completed Jan 13, 2019
@catskull
Copy link

catskull commented Feb 13, 2019

Just for documentions sake, I fought the same issue @mattaningram is describing above. I don't think it was exactly related to vue-loader, but upgrading from webpack 3 to 4 (and really upgrading from extract text plugin to mini css extract plugin).

In my webpack entry point, I was importing some css like this:

import Vue from 'vue'
import Vuetify from 'vuetify'
import Router from 'vue-router'

import App from '~/components/root/App.vue'

import '~/assets/css/vuetify.scss'

But I was also lazy loading my routes like this:

const router = new Router({
  mode: 'history',
  routes: [
    {
      path: '/arbitrary/:pathID/name',
      component: () =>
        import('~/components/myComponents/Component')
      }
    }
  ]
})

I'm not totally sure, but I think it's due to the way mini css extract plugin splits CSS by default, before extract text plugin would put all CSS in this entry point into one giant CSS file. Now it splits all the imports into one big file, but splits each dynamic import into it's own CSS chunk. In my case the fix was pretty simple:
change
import '~/assets/css/vuetify.scss'
to
import('~/assets/css/vuetify.scss')

Basically we're telling webpack to lazy load that CSS import, just like we're lazy loading the component's CSS.
I have no idea if this is bad, but it fixes the problem for me and doesn't seem to cause any performance issues.

I think you could also make mini css extract plugin use a single CSS file for the entire entry point like this: https://webpack.js.org/plugins/mini-css-extract-plugin/#extracting-all-css-in-a-single-file

Also worth noting this issue only appeared in a production environment, which makes sense since in dev mode vue-style-loader does CSS stuff, not mini css plugin. To debug that I set VueLoaderPlugin to production mode like this:

new VueLoaderPlugin({
  productionMode: true
})

Docs here: https://vue-loader.vuejs.org/options.html#productionmode
I think you could also just set NODE_ENV to production as well.

I know this isn't exactly what this issue is regarding, but this was the closest thing I could find when googling so I wanted to document my solution for others.

@rwam
Copy link

rwam commented Sep 16, 2020

Coming from nuxt/nuxt#4219 and it seems the issue still exists. ;-( The only solution that works for me is #808 (comment) Any suggestions appreciated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests