Skip to content

fix(lint): back readAttr with the HTML parser - #2802

Draft
xuanruli wants to merge 1 commit into
mainfrom
xuanru/lint-read-attr-parser-backed
Draft

fix(lint): back readAttr with the HTML parser#2802
xuanruli wants to merge 1 commit into
mainfrom
xuanru/lint-read-attr-parser-backed

Conversation

@xuanruli

Copy link
Copy Markdown
Contributor

Problem

readAttr (packages/lint/src/utils.ts) matched only quoted attribute values. Unquoted values are valid HTML5 and the runtime honours them, so the quoted-only pattern was simultaneously inventing errors and hiding real violations. readAttr backs ~130 call sites across ~68 error-severity rules, and lint errors gate render --strict and the lint section of check.

fixture before after
root written data-composition-id=main data-start=0 data-width=1920 data-height=1080 2 errorsroot_missing_dimensions, root_composition_missing_data_start — for attributes that are present those are gone; the 3 genuine violations it was hiding (deprecated_data_layer, deprecated_data_end, timed_element_missing_clip_class) now surface
two genuinely overlapping unquoted clips, no class="clip" 0 errors overlapping_clips_same_track + 2x timed_element_missing_clip_class

The defect was isolated by the linter's own output: the baseline error message printed the composition id "main", because readDecodedAttr — the htmlparser2-backed reader twenty lines below in the same file — parsed that very tag correctly.

Why not just widen the regex

The first attempt added an unquoted alternative ((?:["']([^"']+)["']|([^\s"'\=<>]+))). Two independent reviews rejected it with measurements, and they were right: the pattern has no notion of attribute-list position, so it captures name=value` pairs out of other attributes' quoted values.

markup attr read regex result correct
<div data-hf-src="clip.mp4?id=5" id="root"> id "5" "root"
<img src="x.png?class=big" class="clip"> class "big" "clip"
<video src="v.mp4?preload=none"> preload "none" none

id is read at 46 of the call sites, so the damage was concrete and error-severity:

  • two <video> elements with distinct real ids sharing a ?id=9 source both read as id "9": duplicate_media_id (error) fired on an id that does not exist, and swallowed two genuine video_missing_muted findings via same-id dedupe;
  • a <video> with no id at all but ?id=7 in its URL had media_missing_id (error) masked — hiding the frozen-video render bug that rule exists to catch;
  • a Google-Drive-style ?export=view&id=<FILEID> asset URL — a canonical pattern in generated compositions — is enough to trigger it.

Delegating to the parser fixes the blind spot without introducing that class, and drops two further divergences for free: the regex silently truncated any unquoted value containing = (href=a.html?x=1&y=2 -> "a.html?x"), and never decoded character references. || null preserves the existing "empty value means absent" contract exactly.

Verification

  • Corpus unchanged. All 13 registry/examples/* and every registry/blocks/* html produce byte-identical lint findings before and after (compared via git show HEAD:<path> file swaps, not git stash — see note below). One reviewer independently reproduced this over 166 targets.
  • packages/lint: 515 tests pass (505 pre-existing + 10 new).
  • The new tests catch both rejected implementations: 4 fail against the original quoted-only regex, 2 fail against the widened regex (the cross-attribute cases). Delegation passes 10/10.
  • oxlint / oxfmt / tsc --noEmit clean.
  • Not slower: a reviewer measured a full 211-file lint pass at 1197ms delegated vs 1282ms with the regex (readAttr built an uncached new RegExp per call).

Behaviour change to surface in release notes

There is no .changeset/ in this repo — release notes are hand-curated from the commit log — so this needs to be stated rather than filed. Attributes that were invisible are now linted, so a project written with unquoted attributes and a real violation will newly fail lint and render --strict. A correct unquoted-attribute project lints clean (verified). The repo's own compositions all quote their attributes, hence the zero-delta corpus.

Not in scope

readJsonAttr is left alone. The parser makes it redundant (it exists specifically because the old regex truncated JSON attribute values, per its own docblock), but removing it is its own change.

Four other lint defects confirmed in the same testing round are untouched and each needs its own fix: non_deterministic_code and requestanimationframe_in_composition flag banned identifiers inside string literals (this blocks the repo's own /pr-to-video output — a typed code-diff animation with "Math.random()" in a data array); new Date( fires on the deterministic new Date("2024-01-15"); overlapping_clips_same_track has no hierarchy model (a parent clip "overlaps" its own children) and keys tracks by raw string, so "1" and "01" are different tracks; imperative_media_control misses ?.play().

Process note for reviewers: git stash shares one refs/stash across all worktrees of this repo. Use git show HEAD:<path> for baseline swaps instead — a concurrent stash collision during this work cost one reviewer a 146-path conflict.

🤖 Generated with Claude Code

`readAttr` matched only quoted attribute values. Unquoted values are valid
HTML5 and the runtime honours them, so on a root written
`data-composition-id=main data-start=0 data-width=1920 data-height=1080` the
linter reported two errors for attributes that are present, while a file with
genuinely overlapping unquoted clips and no `class="clip"` linted clean. The
same rule set printed the composition id in its own message, because
`readDecodedAttr` — the htmlparser2-backed reader twenty lines below — parsed
that tag correctly, which isolated the defect to the regex.

Widening the regex with an unquoted alternative is worse, not better: it has no
notion of attribute-list position, so it captures `name=value` pairs out of
other attributes' quoted values. Measured on that version, two `<video>`
elements with distinct real ids sharing a `?id=9` source both read as id "9" —
`duplicate_media_id` fired on an id that does not exist and swallowed two
genuine `video_missing_muted` findings — and a video with no id at all but
`?id=7` in its URL had `media_missing_id` masked, hiding a frozen-video render
bug. A Google-Drive-style `?export=view&id=…` asset URL is enough to trigger it.

Delegating to the parser fixes the blind spot without that class of mis-read,
and drops the `=`-truncation and undecoded-entity divergences as well. `|| null`
keeps the existing "empty value means absent" contract.

Behaviour change worth surfacing in release notes: attributes that were
invisible are now linted, so a project written with unquoted attributes AND a
real violation will newly fail `lint` and `render --strict`. Correct
unquoted-attribute projects lint clean. The repo's own 13 examples and all
registry blocks produce byte-identical findings.

`readJsonAttr` is left alone; the parser makes it redundant, but removing it is
its own change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant