Improve support for JSON minification #130
Conversation
Introduce automatic minification for JSON-based script types like `application/json` and `application/ld+json`. Add tests to ensure coverage for valid and malformed JSON, improving overall functionality and reliability. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Document the addition of automatic minification for JSON script tags, covering various MIME types and improvements in handling malformed JSON. This update ensures the changelog reflects the latest enhancements in functionality. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
WalkthroughAdds automatic minification of JSON content in script tags for several MIME types, introduces internal JSON helpers in the HTML minifier, bumps package to 4.6.0, adds tests and a changelog entry, and updates README wording for a preset. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.40.0)tests/html.spec.jsThanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/htmlminifier.js (1)
465-479: Critical: Same type attribute normalization issue in processScript.Line 469 has the same critical issue as
hasJsonScriptType- it comparescurrentAttrs[i].valuedirectly without normalization. This will fail to match type attributes with different casing, whitespace, or parameters.Apply this diff to fix the issue:
async function processScript(text, options, currentAttrs) { for (let i = 0, len = currentAttrs.length; i < len; i++) { if (currentAttrs[i].name.toLowerCase() === 'type') { + const normalizedType = trimWhitespace((currentAttrs[i].value || '').split(/;/, 2)[0]).toLowerCase(); // Minify JSON script types automatically - if (jsonScriptTypes.includes(currentAttrs[i].value)) { + if (jsonScriptTypes.includes(normalizedType)) { return minifyJson(text, options); } // Process custom script types if specified if (options.processScripts && options.processScripts.includes(currentAttrs[i].value)) { return await minifyHTML(text, options); } } } return text; }Note: The
processScriptscomparison at line 473 also has this issue, but that appears to be a pre-existing bug not introduced by this PR.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)package.json(1 hunks)src/htmlminifier.js(2 hunks)tests/html.spec.js(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: jonasgeiler
Repo: j9t/html-minifier-next PR: 58
File: package.json:84-84
Timestamp: 2025-09-10T13:18:01.568Z
Learning: In html-minifier-next project (JavaScript/Node.js), the maintainer jonasgeiler prefers not to add git worktree checks to the prepare script in package.json. They consider the failure scenario (ZIP downloads, non-git environments) acceptable since those users aren't following standard git workflows and can remove the script if needed.
Learnt from: jonasgeiler
Repo: j9t/html-minifier-next PR: 58
File: package.json:84-84
Timestamp: 2025-09-10T13:18:01.568Z
Learning: In html-minifier-next project (JavaScript/Node.js), the maintainer jonasgeiler prefers not to add git worktree checks to the prepare script in package.json. They consider the failure scenario (ZIP downloads) acceptable since those users aren't following standard git workflows and can remove the script if needed.
📚 Learning: 2025-09-04T19:01:27.237Z
Learnt from: j9t
Repo: j9t/html-minifier-next PR: 51
File: README.md:176-178
Timestamp: 2025-09-04T19:01:27.237Z
Learning: In html-minifier-next, the minifyJS option callback uses parameters (text, inline) while the minifyCSS option callback uses parameters (text, type). The README documentation correctly reflects these different parameter names for each option.
Applied to files:
src/htmlminifier.js
📚 Learning: 2025-09-10T13:18:01.568Z
Learnt from: jonasgeiler
Repo: j9t/html-minifier-next PR: 58
File: package.json:84-84
Timestamp: 2025-09-10T13:18:01.568Z
Learning: In html-minifier-next project (JavaScript/Node.js), the maintainer jonasgeiler prefers not to add git worktree checks to the prepare script in package.json. They consider the failure scenario (ZIP downloads, non-git environments) acceptable since those users aren't following standard git workflows and can remove the script if needed.
Applied to files:
src/htmlminifier.js
📚 Learning: 2025-09-10T13:18:01.568Z
Learnt from: jonasgeiler
Repo: j9t/html-minifier-next PR: 58
File: package.json:84-84
Timestamp: 2025-09-10T13:18:01.568Z
Learning: In html-minifier-next project (JavaScript/Node.js), the maintainer jonasgeiler prefers not to add git worktree checks to the prepare script in package.json. They consider the failure scenario (ZIP downloads) acceptable since those users aren't following standard git workflows and can remove the script if needed.
Applied to files:
src/htmlminifier.js
🧬 Code graph analysis (1)
tests/html.spec.js (1)
src/htmlminifier.js (2)
minify(1517-1523)minify(1517-1523)
🔇 Additional comments (4)
package.json (1)
87-87: Version bump aligned with new feature releaseVersion
4.6.0here matches the new changelog entry and added JSON script minification behavior; nothing else needed.src/htmlminifier.js (3)
434-441: LGTM - Comprehensive list of JSON MIME types.The array covers the most common JSON-related script types including JSON-LD, import maps, and speculation rules.
443-453: Verify error handling consistency with other minification functions.The function always returns the original text on error without checking
options.continueOnMinifyError. In contrast,minifyJS(lines 849-858) andminifyCSS(lines 796-829) respect this flag and throw when it's set tofalse.Should this function also respect the
continueOnMinifyErroroption for consistency? Here's a suggested fix:function minifyJson(text, options) { try { return JSON.stringify(JSON.parse(text)); } catch (err) { + if (!options.continueOnMinifyError) { + throw err; + } if (options.log) { options.log(err); } return text; } }
1355-1357: Good integration approach for automatic JSON minification.The condition correctly triggers script processing for both custom
processScriptstypes and JSON script types automatically. This allows JSON minification to work without requiring users to configure theprocessScriptsoption.However, this depends on the fixes needed in
hasJsonScriptType(line 455-463) to handle type attribute normalization correctly.
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @j9t. * #130 (comment) The following files were modified: * `src/htmlminifier.js`
Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Add tests for various JSON script MIME types, malformed content, and error-handling scenarios to ensure robust minification. Includes validations for presets and automatic behavior checks. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
CHANGELOG.md(1 hunks)README.md(1 hunks)src/htmlminifier.js(2 hunks)tests/html.spec.js(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: jonasgeiler
Repo: j9t/html-minifier-next PR: 58
File: package.json:84-84
Timestamp: 2025-09-10T13:18:01.568Z
Learning: In html-minifier-next project (JavaScript/Node.js), the maintainer jonasgeiler prefers not to add git worktree checks to the prepare script in package.json. They consider the failure scenario (ZIP downloads, non-git environments) acceptable since those users aren't following standard git workflows and can remove the script if needed.
Learnt from: jonasgeiler
Repo: j9t/html-minifier-next PR: 58
File: package.json:84-84
Timestamp: 2025-09-10T13:18:01.568Z
Learning: In html-minifier-next project (JavaScript/Node.js), the maintainer jonasgeiler prefers not to add git worktree checks to the prepare script in package.json. They consider the failure scenario (ZIP downloads) acceptable since those users aren't following standard git workflows and can remove the script if needed.
Learnt from: j9t
Repo: j9t/html-minifier-next PR: 51
File: README.md:176-178
Timestamp: 2025-09-04T19:01:27.237Z
Learning: In html-minifier-next, the minifyJS option callback uses parameters (text, inline) while the minifyCSS option callback uses parameters (text, type). The README documentation correctly reflects these different parameter names for each option.
📚 Learning: 2025-09-04T19:01:27.237Z
Learnt from: j9t
Repo: j9t/html-minifier-next PR: 51
File: README.md:176-178
Timestamp: 2025-09-04T19:01:27.237Z
Learning: In html-minifier-next, the minifyJS option callback uses parameters (text, inline) while the minifyCSS option callback uses parameters (text, type). The README documentation correctly reflects these different parameter names for each option.
Applied to files:
README.mdsrc/htmlminifier.js
📚 Learning: 2025-09-10T13:18:01.568Z
Learnt from: jonasgeiler
Repo: j9t/html-minifier-next PR: 58
File: package.json:84-84
Timestamp: 2025-09-10T13:18:01.568Z
Learning: In html-minifier-next project (JavaScript/Node.js), the maintainer jonasgeiler prefers not to add git worktree checks to the prepare script in package.json. They consider the failure scenario (ZIP downloads, non-git environments) acceptable since those users aren't following standard git workflows and can remove the script if needed.
Applied to files:
src/htmlminifier.js
📚 Learning: 2025-09-10T13:18:01.568Z
Learnt from: jonasgeiler
Repo: j9t/html-minifier-next PR: 58
File: package.json:84-84
Timestamp: 2025-09-10T13:18:01.568Z
Learning: In html-minifier-next project (JavaScript/Node.js), the maintainer jonasgeiler prefers not to add git worktree checks to the prepare script in package.json. They consider the failure scenario (ZIP downloads) acceptable since those users aren't following standard git workflows and can remove the script if needed.
Applied to files:
src/htmlminifier.js
🧬 Code graph analysis (1)
tests/html.spec.js (2)
src/htmlminifier.js (5)
minify(1523-1529)minify(1523-1529)result(802-808)result(856-856)result(1526-1526)src/presets.js (1)
getPreset(54-58)
🔇 Additional comments (5)
README.md (1)
109-111: Preset wording now correctly reflects comprehensive behaviorUpdating “all conservative options” to “relevant conservative options” matches the current preset composition (after removing
conservativeCollapsefromcomprehensive) and avoids overselling its option set.tests/html.spec.js (2)
1288-1295: Good regression coverage for JSON scripttypepreservationThese assertions nicely lock in the contract that
removeScriptTypeAttributesmust not strip JSON/structured‑data types (application/ld+json,importmap), preventing them from ever becoming executable JS by accident.
1777-3315: JSON script tests comprehensively exercise types, failures, and presetsThe new JSON-script block covers:
- All documented MIME types (
application/json,application/ld+json,application/manifest+json,application/vnd.geo+json,importmap,speculationrules), including malformed cases that must be preserved whencontinueOnMinifyErroristrue.- Type‑matching edge cases (case‑insensitive values, surrounding whitespace, charset parameters) while preserving the original/normalized
typetext as intended.- Error‑mode semantics via
continueOnMinifyError(best‑effort vs throwingSyntaxError), aligned with other minifiers.- Preset and “no options” flows, asserting that JSON minification is automatic under presets and even with
{}options.This suite is thorough and matches the behavior documented in the changelog; I don’t see gaps that would block merging.
src/htmlminifier.js (2)
456-467: hasJsonScriptType now normalizes type correctly
hasJsonScriptTypenow trims, strips parameters, and lowercases thetypeattribute before comparing againstjsonScriptTypes, mirroring the logic inisScriptTypeAttribute. This addresses the earlier concern about case/whitespace/charset variants liketype="Application/JSON"ortype="application/json; charset=utf-8"not being detected.
1361-1365: Chars handler: JSON scripts now always go through processScript—confirm precedence over processScriptsThe new condition
if (specialContentTags.has(currentTag) && (options.processScripts || hasJsonScriptType(currentAttrs))) { text = await processScript(text, options, currentAttrs); }nicely ensures JSON script types get minified even when
options.processScriptsis unset.Two behavioral nuances to be aware of:
- For JSON-like types that are also listed in
processScripts,processScriptwill now always take the JSON path (returning fromminifyJson) before it ever considers treating them as HTML, effectively making the JSON behavior take precedence.- Non-JSON
processScriptstypes continue to work as before, but with the matching change noted around Line 471 this may not align with howprocessScriptsis matched increateSortFns.If this precedence for JSON over
processScriptsis intentional (and you don’t expect users to treatapplication/jsonetc. as HTML templates), then this looks good. Otherwise, you might want to invert the order inprocessScriptso that explicitprocessScriptsconfig wins over auto JSON handling.
Refactor `jsonScriptTypes` to use `Set` for improved performance and simplify related checks. Adjust processing logic for better readability and accuracy when handling `type` attributes. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Add test cases to validate `processScripts` matching semantics and JSON script type detection. Covers case-sensitive matching, normalized comparison, and parameter handling for robust script minification behavior. (This commit message was AI-generated.) Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
Signed-off-by: Jens Oliver Meiert <jens@meiert.com>
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
CHANGELOG.md(1 hunks)src/htmlminifier.js(2 hunks)tests/html.spec.js(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: jonasgeiler
Repo: j9t/html-minifier-next PR: 58
File: package.json:84-84
Timestamp: 2025-09-10T13:18:01.568Z
Learning: In html-minifier-next project (JavaScript/Node.js), the maintainer jonasgeiler prefers not to add git worktree checks to the prepare script in package.json. They consider the failure scenario (ZIP downloads, non-git environments) acceptable since those users aren't following standard git workflows and can remove the script if needed.
Learnt from: jonasgeiler
Repo: j9t/html-minifier-next PR: 58
File: package.json:84-84
Timestamp: 2025-09-10T13:18:01.568Z
Learning: In html-minifier-next project (JavaScript/Node.js), the maintainer jonasgeiler prefers not to add git worktree checks to the prepare script in package.json. They consider the failure scenario (ZIP downloads) acceptable since those users aren't following standard git workflows and can remove the script if needed.
Learnt from: j9t
Repo: j9t/html-minifier-next PR: 51
File: README.md:176-178
Timestamp: 2025-09-04T19:01:27.237Z
Learning: In html-minifier-next, the minifyJS option callback uses parameters (text, inline) while the minifyCSS option callback uses parameters (text, type). The README documentation correctly reflects these different parameter names for each option.
📚 Learning: 2025-09-04T19:01:27.237Z
Learnt from: j9t
Repo: j9t/html-minifier-next PR: 51
File: README.md:176-178
Timestamp: 2025-09-04T19:01:27.237Z
Learning: In html-minifier-next, the minifyJS option callback uses parameters (text, inline) while the minifyCSS option callback uses parameters (text, type). The README documentation correctly reflects these different parameter names for each option.
Applied to files:
CHANGELOG.mdsrc/htmlminifier.js
📚 Learning: 2025-09-10T13:18:01.568Z
Learnt from: jonasgeiler
Repo: j9t/html-minifier-next PR: 58
File: package.json:84-84
Timestamp: 2025-09-10T13:18:01.568Z
Learning: In html-minifier-next project (JavaScript/Node.js), the maintainer jonasgeiler prefers not to add git worktree checks to the prepare script in package.json. They consider the failure scenario (ZIP downloads, non-git environments) acceptable since those users aren't following standard git workflows and can remove the script if needed.
Applied to files:
tests/html.spec.jssrc/htmlminifier.js
📚 Learning: 2025-09-10T13:18:01.568Z
Learnt from: jonasgeiler
Repo: j9t/html-minifier-next PR: 58
File: package.json:84-84
Timestamp: 2025-09-10T13:18:01.568Z
Learning: In html-minifier-next project (JavaScript/Node.js), the maintainer jonasgeiler prefers not to add git worktree checks to the prepare script in package.json. They consider the failure scenario (ZIP downloads) acceptable since those users aren't following standard git workflows and can remove the script if needed.
Applied to files:
src/htmlminifier.js
🧬 Code graph analysis (1)
src/htmlminifier.js (1)
src/htmlparser.js (1)
i(85-85)
🔇 Additional comments (3)
CHANGELOG.md (1)
7-19: JSON script minification entry accurately reflects behavior and coverageThe 4.6.0 notes line up with the implementation: supported script types match
jsonScriptTypes, and the error-handling description correctly mirrorscontinueOnMinifyErrorsemantics used inminifyJson(best-effort by default, strict when disabled). Looks good.src/htmlminifier.js (2)
434-467: JSON helpers and type detection are consistent and robustUsing a
SetforjsonScriptTypesand routing JSON throughminifyJsongives a clear, self-contained path with error handling that matches the existing CSS/JS/URL contracts (respectingcontinueOnMinifyErrorandlog).hasJsonScriptTypenow correctly normalizes thetypeattribute (trim, strip parameters, lowercase), so valid variants like different casing or charset parameters are detected reliably.
471-482: processScript + chars integration preserves processScripts semantics and adds safe JSON auto-minificationThe updated
processScriptuses the normalized type only for JSON detection while matchingoptions.processScriptsagainst the rawtypevalue, keeping behavior in sync withcreateSortFns.scanand avoiding surprises for existing configs. Thecharshandler now callsprocessScriptforscript/stylewhen eitherprocessScriptsis configured or a JSON script type is present, so JSON blocks are minified even withoutprocessScripts, while executable scripts still flow exclusively throughminifyJSand styles throughminifyCSS. This wiring looks correct and non-breaking.Also applies to: 1362-1364
Picks up kangax/html-minifier#1152 to offer the respective support in HMN.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.