Conversation
|
Looks good |
There was a problem hiding this comment.
Pull Request Overview
This PR migrates the package from using html-minifier to htmlnano as the HTML minification library. The change updates the minification engine while attempting to maintain compatibility with existing configuration options.
- Replaced
html-minifierwithhtmlnanoand its dependencies (cssnano,postcss,svgo,terser) - Updated configuration options to match htmlnano's API (e.g.,
minifyJS→minifyJs,minifyCSS→minifyCss) - Converted the filter function to async and updated all tests accordingly
Reviewed Changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Updated dependencies from html-minifier to htmlnano and added required peer dependencies |
| lib/filter.js | Migrated from html-minifier to htmlnano, converted function to async, added custom comment filtering logic |
| index.js | Updated default configuration options to match htmlnano's API |
| test/index.js | Updated all tests to handle async filter function and adjusted expectations for htmlnano behavior |
| README.md | Updated documentation to reflect new configuration options and removed trailing whitespace |
| .gitignore | Added package-lock.json to ignore list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| delete options.exclude; | ||
| delete options.ignoreCustomComments; | ||
| delete options.removeComments; |
There was a problem hiding this comment.
Mutating the config options object is dangerous. Since options is a reference to this.config.html_minifier, these deletions will permanently modify the configuration for subsequent calls. This will cause issues if the filter is invoked multiple times. Create a shallow copy of options before mutation: const options = { ...this.config.html_minifier };
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Mimi <stevenjoezhang@gmail.com>
| [](https://github.com/hexojs/hexo-html-minifier/actions?query=workflow%3ATester) | ||
| [](https://www.npmjs.com/package/hexo-html-minifier) | ||
|
|
||
| Minify HTML files with [HTMLMinifier](https://github.com/kangax/html-minifier). |
There was a problem hiding this comment.
The implementation appears to be slower compared to the current one. I tested with the following configuration (there should be no difference between the two):
# Current
html_minifier:
collapseBooleanAttributes: true
collapseWhitespace: true
ignoreCustomComments: [ !!js/regexp /^\s*more/]
removeComments: true
removeEmptyAttributes: true
removeScriptTypeAttributes: true
removeStyleLinkTypeAttributes: true
minifyJS: true
minifyCSS: true
# This PR
html_minifier:
collapseBooleanAttributes: true
collapseWhitespace: true
ignoreCustomComments: [ !!js/regexp /^\s*more/]
removeComments: true
removeEmptyAttributes: true
removeRedundantAttributes: true
removeAttributeQuotes: true
minifyJs: true
minifyCss: true
I only tested with Cold Processing on Hexo 8.1.1, but when generating around 2000 articles, this PR seems to be approximately 1 to 1.5 minutes slower than the current implementation.
It's unclear whether the slowdown is caused by htmlnano itself or by something in this implementation, but this performance difference might not be acceptable for some users.
|
|
||
| - **ignoreCustomComments**: Array of regex'es that allow to ignore certain comments, when matched. Need to prepend [`!!js/regexp`](https://github.com/nodeca/js-yaml#supported-yaml-types) to support regex. | ||
|
|
||
| Description of the above options and other available options, see [HTMLMinifier](https://github.com/kangax/html-minifier#options-quick-reference) |
| removeRedundantAttributes: true | ||
| removeAttributeQuotes: true | ||
| minifyJs: true | ||
| minifyCss: true |
There was a problem hiding this comment.
IMHO:
We should provide a migration guide since the configuration has changed.
| removeRedundantAttributes: true | ||
| removeAttributeQuotes: true | ||
| minifyJs: true | ||
| minifyCss: true |
There was a problem hiding this comment.
We should provide a migration guide since the configuration has changed.
|
@yoshinorin If considering runtime speed and performance, another option is to use the Rust-based |
|
I'll create another pull request so you can compare it with htmlnano |
|
I have used this library before. While it is fast, it can sometimes produce errors during compression in certain scenarios. wilsonzlin/minify-html#181 Additionally, the library is not very well maintained. |
|
I found a benchmark: https://github.com/maltsev/html-minifiers-benchmark
|
We might wanna do a real-world hexo project minify benchmark for minify time instead, since hexo might produce hundreds, thousands of HTML files. Preferably, we might wanna contribute some performance optimization to the upstream (like htmlnano, which responds to pull requests well). |
|
Based on the Unless there are better alternatives, IMHO, a reasonable compromise would be to switch to |
Another performance test generated with Mitata shows that htmlnano incurs a relatively high overhead on small files. Additionally, I noticed a significant discrepancy between my benchmark and @stevenjoezhang . I’m not sure how your benchmark was conducted, but if it was modified directly based on https://github.com/maltsev/html-minifiers-benchmark, there could be issues—because its benchmark uses |
Check list
Description
Issue resolved: #84
Issue resolved: #148
I managed to complete the migration, and the configuration options were almost entirely unaffected. Users can upgrade seamlessly without any noticeable changes.
Additional information