Decide what to do with the HTML minification feature

[edmorley]

Is your feature request related to a problem? Please describe.
html-webpack-plugin has a minify option that provides support for minifying the generated HTML.

However:

• it currently defaults to off (false) so unless people manually enable it, isn't used
• it adds 14 transitive dependencies (according to cost-of-modules)
• there are other plugins out there for HTML minification (for example html-minifier-webpack-plugin
  ) in case people do want to minify
• the win from minifying HTML is minimal, since most of the possible optimisations can (and IMO should) really be made to the original template - making it easier to track down any possible regressions that result
• the current implementation in html-webpack-plugin has some issues (eg Uglify fails silently when passed in incorrect options #743)

Describe the solution you'd like
For the minify feature and the dependency on the html-minifier package to be removed in the next major version of html-webpack-plugin (ie v4 on the webpack-4 branch).

This idea was previously mentioned in #558 and #452.

Describe alternatives you've considered
Leaving the html-minifier dependency as-is in v4.

Additional context
N/A

[jantimon]

Your points are great 👍
However large projects like create-react-app use it: https://github.com/facebook/create-react-app/blob/next/packages/react-scripts/config/webpack.config.prod.js#L397 and the html-minifier-webpack-plugin doesn't use any caching and will minify on every build (even if no html file was changed) . Furthermore it will only pick up html-webpack-plugin files if it is added after the html-webpack-plugin.
Webpack itself also ships with a built in js minifier.

So we have three possible ways to go here:

1. minify by default if webpack is set to production mode
2. remove it for faster installation
3. keep as it is

[edmorley]

Ah good point about html-minifier-webpack-plugin not using caching, whereas the minify feature here does. If we were to go with (1), which minify options would we enable? (Given some are potentially unsafe)

[jantimon]

We should try to use only safe options - maybe we can reuse the create-react-app settings?

[edmorley]

I've updated the issue title given the choice of direction has changed - or at least is TBD :-)

We should try to use only safe options - maybe we can reuse the create-react-app settings?

CRA uses:
https://github.com/facebook/create-react-app/blob/7b7acde872637a308bb84db75c31980100aed07e/packages/react-scripts/config/webpack.config.prod.js#L397-L408

Neutrino uses (though I've been meaning to see whether we should update this list):
https://github.com/neutrinojs/neutrino/blob/fa9c504a75c91ac2a56a2711dc9d5a5105504255/packages/html-template/index.js#L19-L24

vue-cli uses:
https://github.com/vuejs/vue-cli/blob/1682ff7c177c1808b3632253f88740848210f325/packages/%40vue/cli-service/lib/config/app.js#L110-L115

The options reference is here:
https://github.com/kangax/html-minifier#options-quick-reference

Using any option that isn't used by the projects above is probably risky, since (a) they probably had good reason not to enable it, (b) it's not getting as much real-world coverage.

Reviewing the options used at least once by the projects above:

• collapseBooleanAttributes: Omit attribute values from boolean attributes
  • Not always safe, since this can break CSS attribute selectors.
• collapseWhitespace: Collapse white space that contributes to text nodes in a document tree
  • Not always safe
  • However we can combine it with conservativeCollapse that makes it safe (which means whitespace is collapsed down to a single character, and not removed entirely)
  • Update: Decide what to do with the HTML minification feature #1036 (comment)
• keepClosingSlash: Keep the trailing slash on singleton elements
  • Enabling this option actually reduces the minification performed
  • It seems to have been requested to prevent inline SVG breakage (html-minifier produces invalid inline SVG kangax/html-minifier#45), however trying that testcase in the online minifier shows SVGs are handled properly now even without it set
  • This is one of the few html-minifier settings that is opt-out, rather than opt-in - so if it was dangerous in practice, presumably the default should be changed upstream?
• minifyCSS: Minify CSS in style elements and style attributes (uses clean-css)
  • Enabling this would mean opening ourselves up to any bugs in clean-css on top of any HTML minification issues, so I'd suggest we ignore for now - especially given people shouldn't be including large inline stylesheets in the first place
• minifyJS: Minify JavaScript in script elements and event attributes (uses UglifyJS)
  • We should not enable this since it uses uglify-js by default - which doesn't support ES6
• minifyURLs: Minify URLs in various attributes (uses relateurl)
  • Seems risky since unlike for CRA we can't guarantee what the base URL is?
• preserveLineBreaks: Always collapse to 1 line break (never remove it entirely) when whitespace between tags include a line break. Must be used in conjunction with collapseWhitespace=true
  • Enabling this option actually reduces the minification performed
  • Only Neutrino sets it, and conservativeCollapse is actually the one that improve safety more, so lets ignore this one
• removeAttributeQuotes: Remove quotes around attributes when possible
  • Only vue-cli uses this, and the linked doc says the HTML spec recommends against doing this, so let's skip
• removeComments: Strip HTML comments
  • Used by CRA+vue-cli; seems fine to try this
• removeEmptyAttributes: Remove all attributes with whitespace-only values
  • From the linked doc: "The caveat here is that, similar to “collapse boolean attributes” option, this change can affect certain style or script behavior"
  • so lets skip
• removeRedundantAttributes: Remove attributes when value matches default.
  • see the list of redundant attributes
  • Used by CRA; seems fine to try this
• removeScriptTypeAttributes: Remove type="text/javascript" from script tags. Other type attribute values are left intact
  • Used by vue-cli; seems fine to try this
• removeStyleLinkTypeAttributes: Remove type="text/css"fromstyleandlinktags. Othertype` attribute values are left intact
  • Used by CRA; seems fine to try this
• useShortDoctype: Replaces the doctype with the short (HTML5) doctype
  • Used by CRA+Neutrino; seems fine to try this

As such, how about this?

minify: {
  collapseWhitespace: true,
  // See: https://github.com/jantimon/html-webpack-plugin/issues/1036#issuecomment-421577653
  // conservativeCollapse: true,
  removeComments: true,
  removeRedundantAttributes: true,
  removeScriptTypeAttributes: true,
  removeStyleLinkTypeAttributes: true,
  useShortDoctype: true,
}

(This would just be for when webpack mode is 'production')

[jantimon]

Wow this is really really awesome 👍

[edmorley]

One other thing we would need to decide, is whether any user-provided minify options should be merged into the proposed defaults above, or completely override them? The former would save them having to copy-paste all options into their project when they only want to tweak it slightly - so perhaps it makes most sense?

[jantimon]

So I would propose to have the following mapping for the simple cases:

• false -> no minify
• true -> minify options from Decide what to do with the HTML minification feature #1036 (comment)
• undefined & dev mode -> no minify
• undefined & prod mode -> minify options from Decide what to do with the HTML minification feature #1036 (comment)

Now for the object notation it's harder to decide.

I would rather go for explicit over implicit and not merge it (you would just get what you configured and nothing else).

We could also add a new property to allow the preset:

minify: {
  htmlWebpackPluginPreset: true,
  useShortDoctype: false,
}

However if most users expect it to be merged we can also merge it.
Merging would be your preferred way wouldn't it?

[edmorley]

The true/false/undefined proposal above sounds great.
The Python part of me does prefer explicit over implicit too, so I agree not merging might actually be best :-)

[jantimon]

Perfect 👍 👍

[edmorley]

I'm happy to put together a PR, unless you're already working on it?

[jantimon]

I haven't started yet so sure go for it! :)

Please use the version 4 branch:
https://github.com/jantimon/html-webpack-plugin/tree/webpack-4

#953

[edmorley]

• However we can combine it with conservativeCollapse that makes it safe (which means whitespace is collapsed down to a single character, and not removed entirely)

Looking at conservativeCollapse more closely, I don't think we need (or want) to use it. The examples given in the doc linked above now preserve whitespace correctly, even without using conservativeCollapse. For example try this in the online minifier:

<!DOCTYPE html>
<html>
  <head></head>
  <body>
    <div>
      <span>foo</span> <span>bar</span>
    </div>
  </body>
</html>

...and there is still a space between the two <span>s in the output, even when not using conservativeCollapse.

In addition:

• conservativeCollapse adds extra whitespace in places that it really doesn't need to be (for example after the last </html> and between elements in <head>)
• this comment from the top committer to html-minifier suggests that conservativeCollapse is really only intended for extreme edge cases
• none of CRA, Neutrino or vue-cli use the conservative collapse mode, so even with us being more cautious, it's unlikely we need to either.

As such, I think we should just use standard collapseWhitespace without enabling conservativeCollapse for now. I've updated the suggested config above accordingly.