brunch / less-brunch

Adds LESS support to Brunch
14 stars 18 forks source link

Allow passing arbitrary configuration to lessc. #29

Closed philer closed 8 years ago

philer commented 8 years ago

There's an outdated pull request (#21) that was attempting to do this. Also solves #17. Since this is the day and age of Object.assign things can be done without external dependencies.

I also threw out the reference to config.optimize since it's not having any effect. The previous effect was that of preventing line-number comments which would get removed by clean-css anyways.

Example use case: I'm passing non-default options strictMath: true and strictUnits: true which have a direct effect on the compilation outcome but could not previously be passed through.

philer commented 8 years ago

bump

Any particular reason why this is getting ignored? If I did something wrong please let me know so I can fix it.

es128 commented 8 years ago

Sorry about that. Your bump comment was merited.

Not sure about this choice regarding config.optimize and dumpLineNumbers though. Haven't dug through the history, but it appears to be there for a reason. I don't think we should presume from this plugin that all users will also be using the clean-css plugin. Why not just leave the previous behavior alone?

philer commented 8 years ago

Actually I may have made the false assumption there. I was expecting that less-brunch without clean-css is supposed to use less's native compression. Let me just write down everything I've figured out so far.

First of all, there are three possible values for dumpLineNumbers: 'comments' will include them as comments, 'mediaquery' will include them as special @media queries (valid css without any effect, apparently compatible with sass) and 'all' will do both. Especially the @media rules is probably something you wouldn't want in your production code.

  1. Using compress (or -x) in less removes the line number hints regardless of how they were added. The same applies for clean-css. Thus the respective code in less-brunch was doing something that would have happened anyway (prevent line numbers in minified output).
  2. The native compress option in less is deprecated. Using it on the command lines shows a little warning even though it still works. less-plugin-clean-css is the official recommendation.
  3. Unless I'm missing something compression without clean-css isn't even possible in the current version of less-brunch. The compress option isn't set anywhere, which means that unless you're using clean-css you weren't getting any minified output even when running brunch b --production.

That last point now has me thinking that this is intentional, i.e. that the only difference --production is supposed to have in less-brunch without clean-css-brunch is that no line-numbers are dumped in whatever format. If so it might be worth documenting that fact since there may still be users (like me) who expect less-brunch to use less's native compression when building for production.

Please let me know if that's the case. If so I'll add the check back in.

es128 commented 8 years ago

I don't have a really strong recollection or argument for why it's there, but it is, so this is just for the sake of erring on the side of preserving existing behavior.

It's not related to and doesn't need to conflict with the concept of allowing an arbitrary config object pass-thru, so that's why I'm suggesting to leave it alone for this PR.

philer commented 8 years ago

Fair enough, I changed it back. The diff looks a lot cleaner now. ^^

I guess it kinda makes sense to have it this way. Though I'm honestly not sure why anyone would not be using clean-css in production. IMHO it'd even make sense to integrate clean-css directly into this plugin (as you are currently doing with postcss's modules). That way you could use less-plugin-clean-css for production, rather than piping less's output manually to clean-css in a separate plugin. But that's a whole other topic.

es128 commented 8 years ago

Thanks for the patch!