search

remy / inliner

Node utility to inline images, CSS and JavaScript for a web page - useful for mobile sites
MIT License
1.11k stars 165 forks source link

HTML minification is imperfect #63

Open denis-sokolov opened 9 years ago

denis-sokolov commented 9 years ago

Currently the HTML minification in this module is far from perfect. It treats pre and textarea specially, but not other cases, such as newlines in textarea placeholder attributes or elsewhere. It corrupts strings of ~~s~~ in the source code and further.

Perhaps instead of this manual processing step using a 3rd party tool, such as a popular html-minifier would be warranted? As a matter of fact, that tool can also minify inlined css and js.

As a matter of fact, perhaps it would be valuable to keep the tool minimal and suggests users to use html-minifier on the output themselves, keeping the current tool focused on inlining more interesting resources?

remy commented 9 years ago

I like the approach of making the inliner do as much of the leg work for you (allowing you to opt out when it makes sense), so I think it makes sense to use the html-minifier as a module to compress the output.

My only slight concern is aggressive compression on HTML can result on a different visual representation of the markup (ie. I'd want to use the conservativeCollapse flag for instance to ensure spaces remained intact).

I'd be open to a PR if you wanted a shot at it (please check out the contributing doc as it helps with tests and commits).

denis-sokolov commented 9 years ago

I like the approach of making the inliner do as much of the leg work for you (allowing you to opt out when it makes sense), so I think it makes sense to use the html-minifier as a module to compress the output.

Sure! Some projects at some point choose to spin-off a module (say, inliner-lib) that only contains the minimum core logic and make the primary module depend on that and wrap it with more extra functionality. I suggest to keep this possibility in mind if you notice at some point such a need.

My only slight concern is aggressive compression on HTML can result on a different visual representation of the markup (ie. I'd want to use the conservativeCollapse flag for instance to ensure spaces remained intact).

Well, yes. The point is, your current compression on HTML already results in a different visual representation of the markup. As I mentioned in the original issue, for example, you change the visual representation of a placeholder attribute for a textarea, a visual representation for ~~s~~ and others. Minifying html is pretty hard to do for a general case. In particular, any element may have a white-space: pre specified in CSS. Thus it's important to allow disabling and possibly customize the minification process. If you keep HTML minification as part of the module, I'd suggest exposing the ability to pass options to html minifier.

I can't make any promises about PRs though, but maybe.

hsablonniere commented 9 years ago

:+1: for html-minifier with "conservative"/non-agressive options :smile:

marcelboettcher commented 8 years ago

+1

sinedied commented 7 years ago

I also have the issue when I include JS minified code: when the collapseWhitespace option is enabled, it breaks the JS code. The html minification should really be done by a separate proven package, such as htmlmin for example.

It would also be nice if the minifier options for html/css/js could be further customized by passing through arguments directly to the minifier (I also have issues with uglifyjs default options, and there's no way to override them). This way everyone should be happy 😁

remy commented 7 years ago

Folks, remember, if you want this feature, either use the native 👍 or include a real test or use case, or even better: a PR.

I've not personally hit this issue at all, so I've no real motivation to work on it myself (that's not to say others aren't hitting the issue).

I'm generally deleting github issues with just +1 in them these days.