Option to skip attribute normalization#210
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new normalizeAttributes option to allow consumers to opt out of attribute normalization (addressing hydration issues like class=" xyz " being transformed to class="xyz"), aligning functionality with the request in #191.
Changes:
- Introduces
normalizeAttributes(defaulttrue) and short-circuits attribute normalization when disabled. - Adds unit tests validating behavior when
normalizeAttributes: false. - Documents the new option in the README options reference.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/htmlminifier.js |
Adds normalizeAttributes default and bypass logic in normalizeAttr. |
tests/minifier.spec.js |
Adds coverage for disabling attribute normalization. |
README.md |
Documents the new normalizeAttributes option in the options table. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| test('not normalize attributes', async () => { | ||
| expect(await minify('<p title="bar">foo</p>', { normalizeAttributes: false })).toBe('<p title="bar">foo</p>'); | ||
| expect(await minify('<img src="test"/>', { normalizeAttributes: false })).toBe('<img src="test">'); | ||
| expect(await minify('<img src=" test "/>', { normalizeAttributes: false })).toBe('<img src=" test ">'); |
There was a problem hiding this comment.
This test set validates src spacing, but the reported hydration issue is specifically about class value normalization. Please add an assertion that class=" xyz " (or similar) remains unchanged when { normalizeAttributes: false } to prevent regressions in the core use case.
| expect(await minify('<img src=" test "/>', { normalizeAttributes: false })).toBe('<img src=" test ">'); | |
| expect(await minify('<img src=" test "/>', { normalizeAttributes: false })).toBe('<img src=" test ">'); | |
| expect(await minify('<div class=" xyz ">foo</div>', { normalizeAttributes: false })).toBe('<div class=" xyz ">foo</div>'); |
| expect(await minify('<input title="bar" id="boo" value="hello world">')).toBe('<input title="bar" id="boo" value="hello world">'); | ||
| }); | ||
|
|
||
| test('not normalize attributes', async () => { |
There was a problem hiding this comment.
Test name grammar: consider renaming not normalize attributes to something clearer like do not normalize attributes / skip attribute normalization to match the option name and other test titles.
| test('not normalize attributes', async () => { | |
| test('do not normalize attributes', async () => { |
| if (!options.normalizeAttributes) { | ||
| return { | ||
| attr, | ||
| name: attr.name, | ||
| value: attr.value | ||
| }; | ||
| } |
There was a problem hiding this comment.
When normalizeAttributes is false, normalizeAttr returns early and skips all attribute processing, including removeRedundantAttributes, removeScriptTypeAttributes, removeStyleLinkTypeAttributes, removeEmptyAttributes, and decodeEntities handling. This makes normalizeAttributes act like a master switch for several otherwise independent options; either document that explicitly or narrow the guard to only skip attribute value normalization (e.g., cleanAttributeValue / whitespace trimming) while still applying other attribute-related options.
| | `useShortDoctype` | [Replaces the `doctype` with the short (HTML5) doctype](http://perfectionkills.com/experimenting-with-html-minifier#use_short_doctype) | `false` | | ||
| | `normalizeAttributes` | Normalize attributes | `true` | |
There was a problem hiding this comment.
normalizeAttributes is added at the end of the options table, but the table appears to be sorted by option name. Please place it in the correct alphabetical position (after noNewlinesBeforeTagClose and before preserveLineBreaks) to keep the reference easy to scan.
| | `sortClassName` | [Sort style classes by frequency](#sorting-attributes--style-classes) | `false` | | ||
| | `trimCustomFragments` | Trim white space around `ignoreCustomFragments`. | `false` | | ||
| | `useShortDoctype` | [Replaces the `doctype` with the short (HTML5) doctype](http://perfectionkills.com/experimenting-with-html-minifier#use_short_doctype) | `false` | | ||
| | `normalizeAttributes` | Normalize attributes | `true` | |
There was a problem hiding this comment.
The new normalizeAttributes option is documented here, but the CLI option registry in cli.js is driven by a fixed mainOptions list (and currently doesn't include normalizeAttributes), so users won't be able to set this via CLI flags or config files. Please update the CLI to support --no-normalize-attributes (since the default is true) and ensure config parsing passes it through.
| | `normalizeAttributes` | Normalize attributes | `true` | | |
| | `normalizeAttributes` | Normalize attributes (programmatic option only; no CLI flag yet) | `true` | |
Option to skip attribute normalization
Fixes #191