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

Import order when composing multiple classes #12

Closed
Ambroos opened this issue Jun 21, 2015 · 18 comments
Closed

Import order when composing multiple classes #12

Ambroos opened this issue Jun 21, 2015 · 18 comments

Comments

@Ambroos
Copy link

@Ambroos Ambroos commented Jun 21, 2015

Imagine:

// a.css
.a { color: #aaa; }
// b.css
.b { color: #bbb; }
// ab.css
.ab {
    composes: a from './a.css';
    composes: b from './b.css';
}
// ba.css
.ba {
    composes: b from './b.css';
    composes: a from './a.css';
}
# ab.js
import ab from './ab.css';

render() {
    return <div class={ab.ab}>AB</div>;
}
# ba.js
import ba from './ba.css';

render() {
    return <div class={ba.ba}>BA</div>;
}

You'd expect AB to always be #bbb (because b gets imported last), and BA to always be #aaa for the same reason. If you look at both JS files independently, that is. However, since you depend on import order (which can be affected by other imports happening anywhere in the same codebase), you lose control over the inheritance.

This is fine-ish when you know your codebase and can be sure that CSS is always imported in the right order, but the moment someone imports b.css before you get the chance to import a.css first, you lose the expected result.

With preprocessors and things like @extends you don't get this issue because imports just flow linear and deciding what CSS comes first is a lot more explicit. Their documentation explicitly says that the order of inheritance is purely defined by when selectors occur in the CSS.

There's a way to fix this, but it means adding specificity tricks, and adding selectors to the imported CSS selector in the output.

Using the examples above, it'd lead to this (no class name mangling for clarity, together in a gist):

.a, .a.ab-a-specific, .a.ba-a-specific.ba-a-specific-specific { color: #aaa; }
.b, .b.ab-b-specific.ab-b-specific-specific, .b.ba-b-specific { color: #bbb; }
<div class="ab a ab-a-specific b ab-b-specific ab-b-specific-specific">AB</div>
<div class="ba b ba-b-specific a ba-a-specific ba-a-specific-specific">BA</div>

The good things: it works, repetition gzips well. The bad thing: it's ugly, a challenge to implement.

In projects with lots of composing this could lead to huge selectors, since you're basically replacing all inheritance-by-inclusion-order with explicit specificity. Luckily it's only necessary once you start dealing with composing from multiple classes in a single new class, so maybe it's still something to consider? It would vastly increase complexity of the code, since any class would cause new selectors to be added to all classes it composes from and all their parents too, all the way up the tree.

The other option is probably just making no guarantees about import/override order when dealing with composing from multiple classes. Which is also not ideal and basically means it's an unreliable feature.

Maybe I'm thinking too far here, but I think it's something to look into, at least.

@sokra
Copy link
Member

@sokra sokra commented Jun 21, 2015

Yes, we already discussed this problem. see https://gitter.im/css-modules/css-modules?at=556b2cada18520fe7a59149d and prev comments.

I think we decided to add this as "undefined behavior" in the spec.

In summary: When you compose multiple classes that have conflicting properties the behavior is undefined.

Design your classes to do a single thing and it should not occur that classes have conflicting properties. In exceptional cases !important helps...

sokra added a commit that referenced this issue Jun 22, 2015
@geelen
Copy link
Member

@geelen geelen commented Jun 29, 2015

Actually @sokra (apologies for not catching this when it was reported), I think we can and should define an order for imports. I've actually written code for both the loader core (for Browserify) and the JSPM loader to do this (relevant PR: geelen/jspm-loader-css#12)

Basically, here are the rules:

  1. Each file is injected as its encountered except
  2. Dependencies are injected above their parents and
  3. A file can be injected only once

So, if ab.js is included before ba.js, ab.css is encountered first, its dependencies are injected above it in the order they are encountered so a.css then b.css. Once ba.css is encountered, a.css and b.css are already injected so nothing happens. Result, they both get #bbb.

This is exactly the same as using @extend, i.e. source order is everything 😃

@sokra
Copy link
Member

@sokra sokra commented Jun 29, 2015

Each file is injected as its encountered except
Dependencies are injected above their parents and
A file can be injected only once

yes, but we are talking about the case when there is a conflict.

i. e.

for a module

// ab.css
.ab {
    composes: a from './a.css';
    composes: b from './b.css';
}

You cannot promise the user that a.css is before b.css, because there could be another module which is imported before ab.css with the following content:

// ba.css
.ba {
    composes: b from './b.css';
    composes: a from './a.css';
}

Because of this the spec says the order of multiple imports is undefined. In 99% the order is the source order, but we can't promise that for every case. The spec only tells you that you should not rely on this!

@Ambroos
Copy link
Author

@Ambroos Ambroos commented Jun 29, 2015

I think that with code splitting and on-demand loading you can't even count on import order app-wide. Imagine if clicking one button loads ab.css and clicking another loads ba.css. Depending on which button you click first you'd get different output.

How realistic would adding the specificity-boosting selectors be? It looks like it'd a lot of work to implement for tiny edge cases, but once it's there you have the enormous advantage of having 100% guaranteed CSS inheritance... I don't think that's something that's been done before.

@sokra
Copy link
Member

@sokra sokra commented Jun 29, 2015

How realistic would adding the specificity-boosting selectors be? It looks like it'd a lot of work to implement for tiny edge cases

This would be a global transform, because you would need a global view on all CSS Modules.

I don't like global transforms...

@andreypopp
Copy link

@andreypopp andreypopp commented Jun 29, 2015

We did implement specificity-boosting in react-style which resulted in bloated class properties of DOM elements. css-modules has an advantage is that compositions are statically defined (in react-style you can compose style at runtime) and so specificity boosters can be activated only where needed.

@andreypopp
Copy link

@andreypopp andreypopp commented Jun 29, 2015

This would be a global transform, because you would need a global view on all CSS Modules.

You can actually always generate specificity boosters while in dev and implement more complex algorithm as optimisation for prod build.

@geelen
Copy link
Member

@geelen geelen commented Jun 29, 2015

I am 100% against any kind of specificity-boosting for this feature. CSS has always worked like this, and CSS modules is about enabling useful JS functionality without departing too far from CSS semantics.

Maybe there's a better way to conceptualise composes. Consider the following:

/* foo.css */
.foo {
  composes: a from "./a.css";
  composes: b from ".b.css";
  color: blue;
}

If this was JS, this might mean:

  • Load everything from a in a.css
  • Then Object.merge everything from b in b.css
  • Then Object.merge all the inline styles from .one
  • Then take all those styles and add them to every element marked as foo.

But this is CSS, so really it means:

  • All foos are also as and bs, so include their classes as well when you see foo
  • Load a from a.css and load b from b.css
  • Ensure .a and .b appear in the DOM above .foo so .foo can override

The order of .a and .b in the DOM is specified, it's just specified by the order in which they're first encountered, that just may not be the component you're currently looking at 😄


There's a far simpler solution though that has clearer semantics for both computers and humans:

/* a.css */
.a { color: #aaa; }
/* b.css */
.b { composes: a from "./a.css"; color: #bbb; }
.foo {
  composes: b from ".b.css";
  color: blue;
}

Even if b.css is loaded first, its dependency on a.css will ensure that .a always gets injected above it, and the behaviour is totally fixed and explicit.

For me, having two unrelated classes both setting (and competing for precedence over) the same property has long been a CSS smell, and CSS Modules lets you define which is the base style and which should be considered an override, which I think is neat!

@NogsMPLS
Copy link

@NogsMPLS NogsMPLS commented Dec 16, 2015

This started to be an issue for us on a large and complex React UI Component library we are building internally. We were using small reusable classes, like in Basscss, to compose almost all of our styles. But we kept running into an Order in extracted chunk undefined error. Sometimes this was because of composes order, sometimes this was because of javascript import order of components.

Basically, in the library and any app that may use the library we were unable to guarantee import order in javascript, and also frustrations with having no way to enforce 'composes' order convention across many many files, except for getting a build error and then rearranging.

The way we solved this internally was, instead of having multiple css files and composing from them like this:

.someClass {
    composes: red from '../styles/colors.css';
    composes: block floatLeft from '../styles/layout.css';
}

We instead have 1 style.css file that has a series of imports, thus ensuring composes/import order at all times. So the same class would look something like:

.someClass {
    composes: red from '../styles.css';
    composes: block floatLeft from '../styles.css';
}

and then styles.css would either be 1 css file with the shared amicro/atomic classes in it, or better yet, would just have a series of imports thus keeping shared styles structurally separate, but without the issue of having to worry about order of composes input. So styles.css could look like:

@import './styles/colors.css';
@import ./'styles/layout.css';

This solution admittedly relies on PostCSS Import plugin, so I understand it may not be a solution for all situations. I just wanted to document another possible way to tackle composes and import order issues, that so far seems to have fixed things for us.

@dbow
Copy link

@dbow dbow commented Jan 20, 2016

@NogsMPLS thanks for that comment - I ran into the same issue where it was surfacing errors (though there was no problem with the extracted CSS) because of the order of composes calls (that had no conflicting properties). Went with the PostCSS Import option in a shared common.css file that all composes calls use now. I feel like I don't understand the underlying issue enough to know if this is just sort of the reality of using composition? It felt like there shouldn't have been a problem in my case. I wasn't ever relying on source order - none of the composes calls created any kind of property conflict.

@NogsMPLS
Copy link

@NogsMPLS NogsMPLS commented Jan 21, 2016

Glad it helped @dbow. I agree on the 'not understanding the underlying issue', as none of my properties that I was importing should have been conflicting with anything. Its a little unfortunate to ahve to abstract the composes by one layer. I really liked the idea of composing directly from a colors.css file, so you knew right away where it was coming from. Now it's an extra step to figure out, but no worse than any current SASS/LESS/etc implementation.

@okonet
Copy link

@okonet okonet commented Apr 5, 2016

It seems that @value syntax is also affected.

@tlrobinson
Copy link

@tlrobinson tlrobinson commented Apr 15, 2016

@NogsMPLS's suggestion to aggregate your "atomic" class files into a single file using @import worked well for me, except initially I got an error like "Module build failed: TypeError: Cannot read property 'red' of undefined" because I didn't have css-loader?importLoaders=1 configured.

@AlexGilleran
Copy link

@AlexGilleran AlexGilleran commented May 16, 2016

After writing tonnes of CSS Modules code with composes: and having it all work fine when hot-loading but running into @NogsMPLS's error when doing a build, I switched to using this fork: https://github.com/MicheleBertoli/extract-text-webpack-plugin/tree/order-undefined-error from this PR webpack-contrib/extract-text-webpack-plugin#166. Works great, I don't really see how using composes heavily is viable without it...

Apologies if this is dragging the discussion away from the original issue but this comes up pretty prominently when you google the error message and thought others might benefit.

@delijah
Copy link

@delijah delijah commented Sep 21, 2016

+1

@cfatorNSS
Copy link

@cfatorNSS cfatorNSS commented Feb 6, 2017

+1

@hoschi
Copy link

@hoschi hoschi commented Feb 23, 2017

Is there a linter plugin to check this?

Make sure to not define different values for the same property in multiple class names from different files when they are composed in a single class.

https://github.com/css-modules/css-modules#dependencies

Even when I try to do one single thing in a CSS class I hit the use case where I want to

  • bundle more than one style into a class name for reusabillity
  • adding another class name to a "composes" statement which sets the same property by accident
@gl2748
Copy link

@gl2748 gl2748 commented Jun 9, 2017

jquense added a commit to jquense/css-modules that referenced this issue May 7, 2019
jquense added a commit to jquense/css-modules that referenced this issue May 7, 2019
jquense added a commit to jquense/css-modules that referenced this issue May 7, 2019
jquense added a commit to jquense/css-modules that referenced this issue May 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

You can’t perform that action at this time.