Repository navigation
build(deps)!: upgrade htmlparser2 to v12 - #22
Conversation
htmlparser2 v12 tokenizes foreign content itself through `isInForeignContext`, so the Tokenizer subclass no longer ran. The parser now points that hook at our own namespace stack, and the workarounds v12 makes redundant (self-closing script end search, iframe/noembed/noframes/ plaintext raw text tracking, isInScript) are gone. v12 always treats annotation-xml as an integration point and makes more tags raw text, which broke annotation-xml without an HTML encoding and let raw text tags swallow </noscript>. Fixed by: - skipping noscript content up to its end tag, as the spec raw text state does - namespace-aware integration points, pushed after the self-closing decision - foreign content breakout (p, div, font[color], ...) - template inside svg/math is not inert - svg/math nested in foreign content keep the parent namespace - scripts cut off at the end of the input are not reported Differential fuzz against parse5 (WHATWG spec): spec mismatches go from ~7% of random inputs to ~0.05%. Benchmark is at parity with master. BREAKING CHANGE: htmlparser2 v11+ requires Node.js >= 20.19. Closes #20 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Namespace model and parser requirements package.json, src/index.js |
The htmlparser2 requirement changes to ~12.0.0, and the minimum Node.js version changes to >= 20.19. The parser defines namespace constants and sets for integration points, breakouts, font attributes, and noscript end-tag detection. |
Namespace-aware parsing and validation src/index.js, test/index.js |
Parser callbacks track namespaces and template depth. Script attributes are collected outside HTML templates. HTML noscript content is skipped through its closing tag, while foreign-content noscript is parsed normally. Tests cover raw-text elements, integration points, breakouts, templates, nested namespaces, and incomplete script tags. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Other
Sequence Diagram(s)
sequenceDiagram
participant HTMLSource
participant htmlparser2
participant NamespaceAndTemplateState
participant ScriptAttributeCollection
HTMLSource->>htmlparser2: parse input
htmlparser2->>NamespaceAndTemplateState: pass tag callbacks
NamespaceAndTemplateState-->>htmlparser2: provide namespace and template context
htmlparser2->>ScriptAttributeCollection: collect script attributes outside HTML templates
Possibly related PRs
- Kikobeats/html-scripts#18: Introduced the htmlparser2 integration and foreign-content behavior that this PR revises for v12.
Merge Risk: 
c523c
-
No actionable issue remains from this review; the PR is mergeable after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly identifies the primary change: the breaking upgrade of htmlparser2 to version 12. It is concise and related to the dependency, parser, and Node.js changes. Docstring Coverage No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentat… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Docstring Coverage
-
Explanation
No eligible functions found in the changed files. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 0 functions across 2 files.
-
Finishing Touches
-
📝 Generate docstrings
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks 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 help to get the list of available commands.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
html-scripts
Details
| Benchmark suite | Current: c523c07 | Previous: 114f48b | Ratio |
|---|---|---|---|
getScripts 1MB page |
54.8 ms (54.1-56.3) |
90.4 ms (89.7-98.4) |
0.61 |
getScripts 20KB page |
72.8 ms (71.8-77.5) |
116.6 ms (116.5-120.0) |
0.62 |
This comment was automatically generated by workflow using github-action-benchmark.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b0fc2e5. Configure here.
The tree construction dispatcher inserts an svg start tag in the SVG namespace when the current node is a MathML annotation-xml, whatever its encoding. Track open element depth so only a direct child gets this. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Those two elements stay in MathML when the current node is a text integration point. Treating them as HTML let a script there swallow later scripts. Co-authored-by: Cursor <cursoragent@cursor.com>
Supersedes #20 (Dependabot bump to htmlparser2 12.0.0, which failed
test).Why #20 failed
parser.isInForeignContext().stateBeforeSpecialS/Tare gone, so ourForeignTokenizersubclass was dead code (that's the uncovered lines 17-32 in the failing run).annotation-xmlas an HTML integration point. Withoutencoding="text/html"that's wrong per spec, so<math><annotation-xml><script src="/a.js"/>…swallowed the following scripts.What changed
src/index.js:require.resolve('htmlparser2')/Tokenizer.jspath hack.isInForeignContextnow points at our own namespace stack (html | svg | math).<script/>end search (findScriptEnd/scriptEnd),isInScript, and iframe/noembed/noframes/plaintext raw-text tracking.noscript(htmlparser2 has no raw-text state for it) is skipped by jumping the tokenizer to its</noscript. v12's new raw-text tags could otherwise swallow the end tag.desc/title/foreignObjectonly in SVG,mi/mo/mn/ms/mtextand htmlannotation-xmlonly in MathML). They're pushed after htmlparser2 decides on/>, so<math><title/>self-closes.<p>,<div>,<font color>, …) per spec.<template>inside svg/math isn't inert.<scriptcut off at the end of the input is no longer reported.package.json:htmlparser2 ~12.0.0,engines.node >= 20.19.Breaking change
htmlparser2 v11+ needs Node.js >= 20.19 (ESM-only, loaded through
require(esm)). The commit is marked!, sostandard-versionwill cut 2.0.0.How it was tested
npm test: 40 tests, 100% coverage. 6 new tests, and each one fails on the intermediate version that lacked its fix.Differential fuzz against parse5 (WHATWG spec, scripting on), 50k random inputs per seed, 3 seeds:
The remaining mismatches are htmlparser2's simplified element stack (
</div>/</template>popping elements the spec keeps). Master has them too.Interleaved A/B benchmark against master: 1MB page −0.9%, 20KB page −1.1% (parity). Bare htmlparser2 v10 and v12 perform the same.
Cold adversarial review (separate agent, parse5 oracle, about 190 hand-written cases + 160k fuzz inputs) found one regression (svg inside math). It's fixed and covered by a test.
Known coupling
The noscript skip writes
parser.tokenizer.index, a private field. That's the same level of internals the old code relied on. The~12.0.0pin plus the noscript tests catch any change in a future release.🤖 Generated with Claude Code
Note
Medium Risk
Large rewrite of script-extraction parsing (SVG/MathML/noscript) on a security-sensitive path, plus a breaking Node engine bump and coupling to htmlparser2’s private tokenizer index for noscript skipping.
Overview
Upgrades htmlparser2 from v10 to v12 and raises the minimum Node.js version to 20.19 (breaking release).
The parser no longer subclasses htmlparser2’s tokenizer. Instead it drives
parser.isInForeignContextfrom an internal HTML/SVG/MathML namespace stack, and drops older workarounds (findScriptEnd, manual raw-text tracking, theForeignTokenizerhack).Script discovery is aligned more closely with HTML foreign-content rules: namespace-aware integration points (
title/descin SVG,mi/mo/… and conditionalannotation-xmlin MathML), breakout when HTML tags likedivorfont colorappear inside SVG/MathML,noscripthandling by skipping to</noscript>in HTML (via tokenizer index), non-inert<template>inside SVG/MathML, and incomplete<scripttags at EOF are ignored. Raw-text hiding in tests now also coversstyle,textarea,title, andxmp.Tests add broad coverage for these edge cases (noscript, breakout tags, mglyph/malignmark, nested svg/math, truncated scripts).
Reviewed by Cursor Bugbot for commit c523c07. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
noscriptcontent, templates, script attributes, and incomplete script tags at the end of input.style,textarea,title, andxmp, as well as additional SVG and MathML edge cases.