Kagami / jade-pages-brunch

Adds Jade static pages support to brunch
Creative Commons Zero v1.0 Universal
7 stars 1 forks source link

htmlmin: false ignored #8

Closed githistory closed 9 years ago

githistory commented 9 years ago

Below is my code for production build, the html is always minified whatever I set htmlmin option to.

  overrides:
    production:
      plugins:
        jadePages:
          htmlmin: false
Kagami commented 9 years ago

Hi. I think that's not minification, but default jade output. Add jade: pretty: true if you want to see formatted output.

Kagami commented 9 years ago

Though now it's always overriden to be false in optimize mode. But it's not html minification, I've just checked. Do you want to pass pretty: true in production mode too?

githistory commented 9 years ago

I actually tried pretty: true. Still, no luck. Here's my config:

  overrides:
    production:
      plugins:
        jadePages:
          jade: pretty: true
          htmlmin: false
githistory commented 9 years ago

Just experimented a bit. Indeed it's due to pretty always being set to false in optimize mode.

Guess it'd be good to be able to override that option. Something along the lines of

      if (optimize && !_.isBoolean(this.jadeOptions.pretty)) {
        this.jadeOptions.pretty = false;
      }

What would you think?

githistory commented 9 years ago

Just noticed your source is in coffee. Then great!

if optimize
      # We don't want redundant whitespaces for product version, right?
      # Sometimes we do though...
      @jadeOptions.pretty ?= false
Kagami commented 9 years ago

Fixed, thanks.

githistory commented 9 years ago

My pleasure to help!

I just noticed something else in your commit though:

else if _.isObject(htmlminConfig)
  @htmlmin = true
  @htmlminOptions = _.extend({}, htmlminConfig)

This is saying, if htmlminConfig is an object then use it as options. Then the following defaults are not applied at all:

DEFAULT_HTMLMIN_OPTIONS:
    removeComments: true
    removeCommentsFromCDATA: true
    removeCDATASectionsFromCDATA: true
    collapseBooleanAttributes: true
    useShortDoctype: true
    removeEmptyAttributes: true
    removeScriptTypeAttributes: true
    removeStyleLinkTypeAttributes: true
Kagami commented 9 years ago

Yes, it was planned to behave like this, because otherwise you will need to readjust all defaults if you want custom ones.

Kagami commented 9 years ago

I've just realized that this change may break things badly for people who already have pretty: true in their config, so I reverted it for 1.8.1 and applied again to 2.0.0. So it's breaking change.

githistory commented 9 years ago

Exactly. Updated. Thanks for the help!