Add ignoreOrder option to avoid the OrderUndefinedError (version 2)

[grrowl]

This rebases webpack/extract-text-wepback-plugin@2.0.0-beta.3 against MicheleBertoli/extract-text-webpack-plugin's order-undefined-error branch.

It adds a ignoreOrder config param which can be set in environments where the result is not reliant on extraction order.

Fixes an issue with the popular CSS Modules, which has support in css-loader, where an undefined import order is not an issue due to the nature of CSS Modules scoping.

Previous order-undefined-error pull request: #166
Related issue on the CSS Modules repo: css-modules/css-modules#12

[codecov-io]

Current coverage is 89.45% (diff: 100%)

Merging #241 into master will increase coverage by 3.61%

@@             master       #241   diff @@
==========================================
  Files             4          4          
  Lines           332        332          
  Methods          68         68          
  Messages          0          0          
  Branches         72         72          
==========================================
+ Hits            285        297    +12   
+ Misses           47         35    -12   
  Partials          0          0          

Powered by Codecov. Last update 27e73bf...a12840c

[outdooricon]

What is the resistance to adding this? This conversation has been going on for a pretty long time in various threads. It would be nice if we could get some direction from the maintainers on what would be the right direction if this isn't it. As CSS Modules gains traction, this problem is only going to become more visible to the community.

[besh]

I agree with @outdooricon. I've been pointing our package.json at various commits like this for a couple months. Would be nice to finally see it officially in the plugin.

[justgook]

+1

[sunny-g]

@grrowl is the blocker to merging this the CLA?

[grrowl]

Who knows? I'm not aware of a CLA needed for the webpack project, willing to do nearly anything to get this merged since I know a lot of people are having an awkward time with webpack and CSS Modules and this very particular issue.

Note: I feel like it could cause an issue if you compose a class from multiple files, and the order of those files is undefined/change: e.g.: given composes largeHeading from "a.css"; composes 10pt from "b.css";, the calculated style depends on the output order of a.css and b.css — you may have a .largeHeading with 10pt text, or .largeHeading may come later and therefore override 10pt's font-size declaration.

Well written, modular CSS will avoid this double-up, but it's worth mentioning and may be the reason for the resistance. Of course, undefined import order is a much lighter compromise compared to keeping all imports in an extremely strict order, which is near impossible in a decent-sized app.

[grrowl]

See also these very reasonable workarounds (compose more simply, or define the import order in one file and import from that intermediate file)

css-modules/css-modules#12 (comment) (@geelen) and later css-modules/css-modules#12 (comment) (@NogsMPLS)

[grrowl]

It doesn't look like this is going to be merged in. The webpack team has enough of their plate getting version 2 ready for stable release, and as explained in the links in my previous comment, ignoring OrderUndefinedError may lead to very very subtle bugs in your code (due to CSS of the same specificity relying on the order within the file).

Read the previous links to understand how these subtle bugs might occur, to either avoid making the same mistake in future, or so you can refactor your existing code. Until then, no harm in using this fork. Sorry to be the bearer of unexciting news everyone 😧

[taion]

What's the blocker here exactly? If the concern is stability – we've been running with this code for months now.

Arguably this should be the default with CSS modules, as CSS modules explicitly makes no ordering guarantees on how your code ends up.

[grrowl]

To borrow from @geelen's above linked example:

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

given

.a { border: solid yellow; background-color: blue; color: white; }

.b { background-color: white; border-bottom: solid blue; }

you'd expect the computed styles for .foo to be

.foo { 
  border: solid yellow; background-color: white; border-bottom: solid blue; color: white; 
}

but if earlier in the file or bundle we have @import './b.css'; @import './a.css';, then .b's class will be output earlier in the file than .a's classes, and as long as they have the same level of specificity (they're all classes) the actual computed styles for .foo will be:

.foo { 
  border: solid yellow; background-color: blue; border-bottom: solid blue; color: white;
                                       /* ☝️ hey! */
}

This is the error ExtractTextPlugin is trying to avoid introducing into your code. CSS Modules makes no guarantees as to order, because it's up to style-loader or extract-text-plugin to determine how and in what order your CSS rules are output — otherwise the rules on how styles are computed are left to the CSS spec.

style-loader inserts styles in the page in whatever order they're require()d at runtime, while ExtractTextPlugin decides to warn you and explode instead. What might be really helpful is a fork of ExtractTextPlugin which warns you which of your imports are out of order, and print the filename and line number then compile anyway. Then these potential conflicts can be resolved, and if your .foos look fine on development but are all blue on production you might have a chance of resolving it.

[taion]

I don't think of these as code bugs. If the spec explicitly says that order in this case is undefined, per https://github.com/css-modules/css-modules#dependencies, it's not a good thing for the plugin here to give me additional guarantees – it makes my code less spec-compliant.

[taion]

Specifically:

Note that when composing multiple classes from different files the order of appliance is undefined. 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.

The goal should be to code to the spec, not to the implementation here. Ordering imports to take advantage of the implementation here to get property overriding is not something that should work per spec.

[grrowl]

tl;dr: The concern is not stability, but that in the cascade, the order is very important. CSS Modules effectively removes the need for specificity (everything is a .class) — without a stable source order we're only left with !important to define which rules should overlap others in very complex apps.

[taion]

The spec explicitly says that ordering is not defined in the case you give, though. Anybody who writes code per the above and expects it to work is doing something wrong, because the spec explicitly says that there are no such ordering guarantees.

[grrowl]

CSS Modules says the order is undefined, but in the resulting CSS the order is very important. I believe the CSS Modules spec writers may have meant it as in undefined behaviour. C famously specified the behaviour for dividing by zero as undefined — some compilers will throw an exception, some will return the maximum integer, some will return an incorrect result.

That's kind of what we're seeing here, because style-loader cannot ascertain the order the CSS is in, even if it wanted to, it continues without error. ExtractTextPlugin can and does, and throws an exception when the order is undefined to boot.

I don't disagree with you btw, hence why I'm pushing this PR and have done a bunch of research (to make sure I wasn't baking bugs into projects) and chased up people to try to get this merged. Now that Webpack 2.2 is finally released (:tada:) we might be able to get some response from @sokra who originally wrote ExtractTextPlugin and the CSS Modules feature, at least for another viewpoint.

[felixsanz]

Why this doesn't get merged? I'ts been a long time now of bugs and workarounds. CSS Modules doesn't guarantee order.

[grrowl]

See #166, and depend on @taion's fork at @taion/extract-text-webpack-plugin if you need this feature.

There's no need for this PR any more as workarounds are available, the webpack team will address this issue in a future release, and we've discussed this pretty much to death 😅

Thanks for the discussion all.

[grrowl]

Sounds like the original code was merged in #166, so I'll close this ticket. Thanks!