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

Add CSS Modules capability to vue components #13

Closed
wants to merge 10 commits into from

Conversation

FWeinb
Copy link

@FWeinb FWeinb commented Jul 5, 2015

Like requested in #12 I added support for including css via css-loaders css local scope feature to achieve true CSS Modules like described in this blog post.

Technically this is done by injecting the result of the required CSS into the data object of each component, to gain access to the exported identifiers in the vue instance. Makeing this work required that you can't have multiple <style>-tags when using this feature. I choose to implement it behind the runtime option injectStyle.

I don't now if injecting it into the Vue component is the most elegant way to do it but it is definitely working. It would be great if it where possible to have a object on the vue instance which will, by default, be a on time binding, without explicitly using {{* }} every time.

}
}, function (window) {
var module = window.testModule
expect(module.data().styles.red).to.equal('_3iZItEFWW8HWfKNmM95gGD')
Copy link
Member

Choose a reason for hiding this comment

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

Can we add 2 assertions here:

  1. the data should also contain the original msg field;
  2. the style should have been inserted with the same class name. (possibly by selecting the first <style> node and do a regex match on its textContent)

@yyx990803
Copy link
Member

Great work! I left a few comments, small things. LGTM after those are fixed :)

@FWeinb
Copy link
Author

FWeinb commented Jul 5, 2015

I removed the newlines and added the assertions. Thanks for the hint! Could you think of an better way to inject the styles into vue? It would be great to have some way to specify if an object in data won't change. Something like:

new Vue({
   data : {
      styles : {
          $$immutable: true,
           [...]
       }
   }
})

Just to be a little bit more economical with the amount of bindings vue creates.

[Edit]
Another thing that would be possible is to detect if an object was frozen (via Object.isFrozen) and only if it is not frozen create a watcher for these variables.

@yyx990803
Copy link
Member

The cost of setting up a single watcher is relatively cheap in Vue, and once created it has no effect on other parts of the application, so I wouldn't worry too much about performance here. I've added support for frozen Objects in yyx990803/vue@9259325 , but using frozen objects in vue-loader would break compat with older versions.

Another possible alternative is using a special syntax for local class names in templates, e.g:

<div class:local="red"></div>
<!-- or -->
<div class=":local(red)"></div>

Then, we need to retrieve the generated class names in the loader (by executing the css module inside the loader), with that we can rewrite all these local classes into the generated names. This would have 0 runtime cost and has a imo cleaner syntax.

@FWeinb
Copy link
Author

FWeinb commented Jul 6, 2015

I like that but we still need the ability to access classes in js sometimes. Than it would definitely be better to allow the injection on a per component basis than globally on the vue-loader.

@yyx990803
Copy link
Member

Instead of adding the option to the webpack config, how about

<style inject-locals>
  /* ... */
</style>

@FWeinb
Copy link
Author

FWeinb commented Jul 6, 2015

👍 I like that

[Edit]
But how would we test for the attribute to be present? I think I got it.

@FWeinb
Copy link
Author

FWeinb commented Jul 6, 2015

I switch from the global option to the per style tag option like you suggested. To get this working I modefied the parser.js to expose an object like {attribs: { [node attributes]}, content: [str content of node]}.

The only thing left would be to support some kind of template html preprocessors and inject the style object into that too. That would enable the 0 runtime cost of supporting CSS Modules.

@yyx990803
Copy link
Member

Cool, I think I've got it somewhat figured out. We just need to nail down the syntax. The tricky part is handling both global class names and local ones at the same time. Two possible options:

<div class="global" class:local="local"></div>

vs.

<div class="global :local(local)"></div>

I personally prefer the former a little bit, as it's more explicit and easier to parse/transform.

@FWeinb
Copy link
Author

FWeinb commented Jul 6, 2015

The fist method would be definitely easier to parse but is kind of invalid html. I don't know if the html-parser used in this plugin would like that. I still like it that method a bit more too.

@yyx990803
Copy link
Member

It is valid html, as per the spec html attribute names can contain "all characters except tab, line feed, form feed, space, solidus, greater than sign, quotation mark, apostrophe and equals sign", and parse5 with htmlparser2 adaptor can handle these attribute names just fine.

@FWeinb
Copy link
Author

FWeinb commented Jul 6, 2015

Oh, my bad good to know. Than everything looks good. Should this be added in this PR or should we create a new one for that?

@yyx990803
Copy link
Member

I'll flesh out the locals extraction/rewrite part a bit more, then we can look at how to move on from this PR.

@azamat-sharapov
Copy link

👍

Evan, if you are going with class:local="local" syntax for local classes, how will this work with v-class ? do we use v-attr instead? or have some plan for changing v-class syntax? new directive like this for example: v-class:local="local: true".

@yyx990803
Copy link
Member

@azamat-sharapov that's a good question! v-class:local seems reasonable, but I'll think about it a bit more before we commit to a syntax.

@yyx990803 yyx990803 mentioned this pull request Jul 7, 2015
@yyx990803
Copy link
Member

So with af04dbb...master I've implemented what we discussed, with slightly different syntax:

<style>
:local(.red) {
  color: red;
}
</style>

<template>
  <div local-class="red" v-local-class="red: showRed"></div>
</template>

<script>
module.exports = {
  data: function () {
     return { showRed: true }
  },
  created: function () {
    // accessing local classes map from js
    console.log(this.$localClasses)
  }
}
</script>

local-class does not support interpolations; dynamic local classes should use the v-local-class directive which is injected automatically.

@FWeinb
Copy link
Author

FWeinb commented Jul 14, 2015

Great work. So this PR can ne closed?

@yyx990803
Copy link
Member

Yes, although I feel bad that you're not getting credit for this (I worked on this when I was on the plane). Do you want to help with the docs?

@FWeinb
Copy link
Author

FWeinb commented Jul 15, 2015

It's okay don't worry.

@yyx990803 yyx990803 closed this Jul 15, 2015
@yyx990803
Copy link
Member

So, after trying this out for a while, I somehow feel it's still a bit awkward to use, because essentially I have to write every class with :local(), but not for other types of selectors. Having to think about which to use among class, local-class, v-class and v-local-class, and their respective gotchas, plus the interaction with this mechanism inside JS, is just too much mental burden.

Using local-by-default mode probably works better, but we can't turn it on by default, and it still has the same problems interacting with v-class and JavaScript.

Instead, what I am trying now is auto-scoping:

<style local>
h1 {
  color: red;
}
</style>

For style blocks with the local attribute, we generate a unique hash id for the current component, and prepend that id to every selector in that block. So after compilation we get something like:

<style>
#_3iZItEFWW8HWfKNmM95gGD h1 {
  color: red;
}
</style>

This gives the guarantee your local styles doesn't leak out, and the mental model is almost identical to traditional CSS authoring.

One problem with this is that it doesn't prevent outside global class selectors from seeping in; However, if the whole app is written with vue-loader, this won't happen because you have control over what global classes are defined, and even in a team environment, you can avoid conflicts with good naming conventions (e.g. all global classes should start with .global-).

@FWeinb
Copy link
Author

FWeinb commented Jul 16, 2015

There is a Post css loader that will make all styles local by default. See https://github.com/css-modules/webpack-demo

@sjoerdvisscher
Copy link

I think the unique id has to be a class, so it can be used with components with multiple root elements.

@azamat-sharapov
Copy link

Evan, did you mean h1#_3iZItEFWW8HWfKNmM95gGD instead of #_3iZItEFWW8HWfKNmM95gGD h1 ? If no, does this mean, it's just adding unique ID (#_3iZItEFWW8HWfKNmM95gGD) to component root tag (like I commented in the beginning of all this)?

@yyx990803
Copy link
Member

@FWeinb I'm aware of that, but having it being the default is a big hoop to jump through for new users. And as I said, it still causes extra care to be taken for v-class and using local classes inside JS.

@sjoerdvisscher that's a good observation.

@azamat-sharapov yeah it's basically what you mentioned, but done automatically with the local flag.

@azamat-sharapov
Copy link

+1 for that local flag then, personally I find it "easy to think", when debugging, etc. But after this, we would need to strongly recommend users to always use root tag for component probably? Because if components have too many different elements non-wrapped with single root tag, vue-loader adds those unique classes (like Sjoerd mentioned) to every element and maybe it would be mess.. :)

@yyx990803
Copy link
Member

It is already recommended to always use a root element. But you shouldn't really worry about the rendered markup if it makes your code easier to write and maintain.

@azamat-sharapov
Copy link

Yeah, you are right. Just thought how many things React adds to DOM behind the scenes.. It's more about maintaining.

@FWeinb
Copy link
Author

FWeinb commented Jul 16, 2015

I just had some time to test the current implementation in a small project and can second @yyx990803 opinion now; <style local> would definitely make it much easier.
On thing that we would loose with that approach would be the ability to minify classes in production but I think that should not be to bad of an issue.

@yyx990803
Copy link
Member

I've implemented the new proposal (changed local to scoped since it's not really local, but more like lexical scope).

One small issue with <style scoped> is that you have to use a special selector :root to refer to root elements, which can be a gotcha, but not too bad.

What we do lose though, is the ability for components to securely avoid parent styles from seeping in. For example, say we have styles for .test in a parent component, it could accidentally affect elements in the child component that also have .test elements. This is not that big an issue for projects that consist purely of in-house components, but could be a problem when we try to ship fully-encapsulated 3rd party components.

So maybe for reusable 3rd party components, we still need the local-class syntax? Oh well, 3rd party components can get around this problem with naming conventions as well (e.g. prefixing all classes with a namespace).

@FWeinb
Copy link
Author

FWeinb commented Jul 16, 2015

The gotcha for 3rd party components is kind of a killer, these kinds of bugs are very confusing and hard to debug. We want to make it easy for the user to write vue component but by introducing these subtle sources of bugs it can become very hard to reason about a component. Having to rely on some kind of "well written" 3rd party components is a very big burden.

@yyx990803
Copy link
Member

Well, CSS cascading problems have existed all the time, and it's not easy to solve (even from the Browser vendors perspective). Right now we have two ways to deal with it:

  1. Provide some level of scoping via <style scoped>. It's easy to use, but we need to clearly state that it's simply nesting the selectors;
  2. Provide real local classes, but introducing non-trivial API friction. (I looked at the CSS modules spec, what concerns me is that it's actually a lot of new syntax to grok, and it's non-standard. Almost like learning another pre-processor. I don't want to force Vue users into using that.)

@yyx990803
Copy link
Member

Turns out we can achieve CSS-in-JS inline styling (what most React components are doing) pretty easily:

<template>
  <div v-style="styles.button">hi</div>
</template>

<script>
var styles = {
  button: {
    color: 'red',
    fontFamily: 'Source Sans Pro',
    fontWeight: 'bold'
  }
}

module.exports = {
  data: function () {
    return {
      styles: styles
    }
  }
}
</script>

Aha! We can translate CSS into these JS objects for the user so that they can write actual CSS!

@azamat-sharapov
Copy link

It will be really messy in my opinion.. there is big discussion/debate going on around web about "writing CSS in JS". I personally wouldn't use it, it's really like "react way" (they write HTML and now CSS in JS). Vue is completely different honey for me. That <style scoped> sounded really Evan to me, v-style doesn't :)

@yyx990803
Copy link
Member

Well, the problem with <style scoped> is that it only solves half the problem. It works well enough when you are developing small apps with everything under your control. But what truly reusable 3rd party components want, is to completely eliminate the possibility of clashing class names with a parent component, because you don't know what environment the component could be used in. That's why most standalone React components use inline styles.

@azamat-sharapov
Copy link

I agree about standalone components. Have never developed standalone yet, but that's good info to keep in mind. Let's just leave standalone component makers to stick with v-style and <style scoped> for regular app dev? :)

@yyx990803
Copy link
Member

That's what I'm thinking. If one really wants to ship a reusable-in-any-environment component, the choice is to either 1) use custom namespaced class names or 2) inline everything with v-style. Which I think is an ok tradeoff.

@FWeinb
Copy link
Author

FWeinb commented Jul 27, 2015

Did you think about this a little bit more? I can see that there will always be a tradeoff in some kind but I would really like to see a viable solution in vue-loader.

@yyx990803
Copy link
Member

@FWeinb for now what we have is scoped styles (which can possibly affect child components). I'll come back to this when I get time, for now the focus will be shipping the router.

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

Successfully merging this pull request may close these issues.

None yet

4 participants