Repository navigation
refactor(bestax-migrate): delete the per-target link-attribute table - #683
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe change removes the per-target link attribute table from polymorphic link filtering. Target-level rejection now relies on ChangesLink attribute filtering
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Refactor Merge Risk: ⚪ Minimal · up to The link-attribute simplification is covered by the expanded href-capability test, with no remaining merge-blocking concern. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Preview DeploymentPreview URL: https://2fcfba0a.bestax.pages.dev |
AI triage — issues this PR may resolve
If this PR resolves one of these, add the line below to the PR description so the issue closes on merge: |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Deep review — 0 blocking · 3 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Robustness | Orphaned JSDoc left behind by the deletion: /** The rows the type test holds to the library. */ now documents nothing and is a verbatim duplicate of line 278, which documents HREF_TABLE. Deleting the line is the whole fix. |
bestax-migrate/src/sources/_shared/polymorphic.ts:332 |
| 2 | 🔵 Advisory | Robustness | Comments still narrate the removed row mechanism — the dropInertLinkAttrs preamble ("A target that enumerates its props can be narrower than the element", "Keying the message off the target has a row") and the describe title "the per-target link-attribute exception matches the library". Nothing false, but they describe a check the code no longer performs. |
bestax-migrate/src/sources/_shared/polymorphic.ts:629, bestax-migrate/e2e/as-unions.test.ts:354 |
| 3 | 🔵 Advisory | Coverage | Point-in-time evidence — move to the issue and link it. The commit message and PR body both claim the file's coverage "rises from 89.74/81.81 to 90.67/84.34"; neither figure reproduced here (see below). | commit message / PR body |
What I ran, and what it reported
Built @allxsmith/bestax-bulma first — the e2e typechecks against dist, and without it every as-unions suite fails on TS2307: Cannot find module '@allxsmith/bestax-bulma'.
| Command | Result |
|---|---|
pnpm --filter bestax-migrate run test -- e2e/as-unions.test.ts |
13 passed — including the widened premise sweep |
pnpm --filter bestax-migrate run test:coverage |
35 suites / 1213 passed; polymorphic.ts 100 % stmts / 94.78 % branches / 100 % lines |
| same, with the e2e path ignored | polymorphic.ts 99.1 / 90.43 |
pnpm --filter bestax-migrate run typecheck / lint / format:check |
clean |
So the touched file measures 100 / 94.78 with the full suite and 99.1 / 90.43 unit-only. The 90.67 / 84.34 in the commit body matches neither, which is the case for rule 6b rather than a defect in the code.
Branch-coverage misses on polymorphic.ts are lines 563, 657-671, 765-774 — not 650/652, so both arms of the collapsed because ternary are exercised.
Overall: The deletion is sound and well-guarded. TARGET_LINK_ATTRS really was empty on main, so declared was always undefined, the else if (declared && ...) arm was unreachable, and the component arm of because could only ever be taken with carriesLinks === false — which is exactly the single message the diff collapses it to. I found no reference to TARGET_LINK_ATTR_TABLE anywhere else in the tree (source, e2e, docs, skills/, bestax-mcp/data), and the package exposes only a bin, so dropping the export breaks no consumer. The release premise checks out too: #675's file list confirms it changed bestax-migrate/src/sources/_shared/polymorphic.ts under a fix(bulma-ui) squash, which the !(bestax-migrate) scope rule suppressed, and refactor at scope bestax-migrate matches the patch rule while matching no false rule. The one thing worth a human's eye is the squash title — the patch release depends entirely on it keeping the refactor(bestax-migrate) scope.
Residual risk:
- The premise sweep is type-level, and the failure it guards against is partly runtime. The
HREF_OKdoc block is explicit that "typechecking cannot answer the question on its own" —Level.Itemtypeshrefat everyasand drops it at runtime. The widened e2e is atsccheck: it proves each target's props accept every link attribute, not that the component forwards it. A component that keeps the type and filters an attribute at runtime while rendering an anchor would slip past it, and that is the shape the deleted arm existed for. Refuted for today's library — I read every anchor branch of the targets inHREF_OK:LevelItemspreadsrestonto the anchor whole (bulma-ui/src/layout/Level.tsx:281),DropdownItemforwardsrestunchanged when the tag is an anchor and only strips on non-anchors (bulma-ui/src/components/Dropdown.tsx:483), and that non-anchor stripping is already caught by the element arm of the rule. Named here as open rather than posted as a finding: the gap predates this PR (the arm has been dead since #672) and the diff does not widen it. - An href-capable target going missing from the sweep — refuted.
HREF_OKis held complete against the library by a separate suite in the same file ("HREF_OK names every reachable target that takes an href › and no others"), which fails if a target that takes anhrefis absent from it. - A link attribute escaping the sweep — refuted.
LINK_ATTRS(whatdropInertLinkAttrsiterates) andLINK_ATTR_TABLE(what the sweep iterates) are bothObject.keys(LINK_ATTR_ELEMENTS), so the two sets are the same object by construction;relis deliberately out of both, and a new row without aSAMPLEentry fails its own guard test. - The
continuethis diff deletes having been live — refuted. The table was empty, so theTARGET_LINK_ATTR_TABLE[target]guard never fired; the coverage of the sweep is unchanged in practice and only its form widened. The PR body says as much.
🏄 Pure paddle-out cleanup, dude — a branch nobody could ever surf, gone, and the e2e even drops the rope it was holding onto. Only flotsam left is a lonely doc comment bobbing at line 332 and some coverage numbers that don't match the tide chart. Good to go. 🌊
7ea7c91 to
d2e7b35
Compare
Preview DeploymentPreview URL: https://02ec78c9.bestax.pages.dev |
Preview DeploymentPreview URL: https://52f540a5.bestax.pages.dev |
|
deep-review: fresh The three advisory findings are addressed and the branch is now one commit (
Worth a look at whether anything else still narrates the deleted mechanism — that was two of the three findings, and the class I keep missing. Copilot and CodeRabbit are unavailable on this PR, so this is the only review pass. |
| else if (declared && !declared.includes(name)) rejectedBy = 'component'; | ||
| // A dynamic `as` leaves the element unknown, and guessing either way is | ||
| // worse than leaving it for the author. | ||
| else if (rendered !== undefined && !elementTakesLinkAttr(name, rendered)) |
There was a problem hiding this comment.
The behaviour this release exists to ship is asserted by nothing — 🟡 Minor · Coverage
What: With the per-target arm gone, the only thing that keeps download on <Level.Item as="a" download> is this element check falling through to if (!rejectedBy) continue. No test pins that. e2e/polymorphic-output.test.ts:119 carries the exact case, but it is a compile-matrix row only — and typecheckTsxFiles returns status: 0 whether the attribute survived or was stripped with a TODO, because removing an attribute never makes output stop compiling. The same is true of the referrerPolicy-on-<img> row at :127, whose comment says "Kept where the element really takes it" while nothing asserts it was kept. Grepping the package, toContain assertions on a surviving attribute exist for href (:227, :237, :246, :282) and for no link attribute at all.
Why it matters: The PR body names this release's whole purpose as unblocking the shipped fix — "Today bestax-migrate@2.3.3 still strips download from <Level.Item as="a" download>". The row that used to strip it is gone, and the PR body says the one test that covered it went with it. So the fix ships with no regression test: re-introducing a component-side strip for Level.Item, or inverting elementTakesLinkAttr's polarity here, leaves all 1213 tests green.
Fix: two assertions beside the existing href ones in e2e/polymorphic-output.test.ts's keeps what the target does accept, and every live expression block:
// The anchor form takes the anchor's whole surface since #672, so the
// link attributes beside the `href` survive with it. The compile matrix
// cannot see this: a stripped attribute typechecks just as well.
const carried = migrate(rbx, 'rbx', [
'Level',
'<Level.Item as="a" href="/x" download>x</Level.Item>',
]);
expect(codeOf(carried)).toContain('download');
// Judged per attribute against the element, not as a group.
const img = migrate(rbx, 'rbx', [
'Button',
'<Button as="img" referrerPolicy="no-referrer" src="/a.png" />',
]);
expect(codeOf(img)).toContain('referrerPolicy');What I ran
| Command | Result |
|---|---|
pnpm --filter bestax-migrate run test |
35 suites / 1213 passed (no count drop) |
pnpm --filter bestax-migrate run test -- e2e/as-unions.test.ts |
13 passed, widened sweep included |
pnpm --filter bestax-migrate run test:coverage |
polymorphic.ts 100 / 94.78 / 100 / 100 |
grep -rn "download" bestax-migrate/src bestax-migrate/e2e bestax-migrate/fixtures bestax-migrate/__testfixtures__ |
only LINK_ATTR_ELEMENTS, the SAMPLE row, and the two unasserted matrix cases |
Path traced for <Level.Item as="a" download>: HREF_OK['Level.Item'] is set → carriesLinks true; declaresAs true and acceptsAs('Level.Item','a') true → rendered === 'a'; elementTakesLinkAttr('download','a') true → rejectedBy stays null → attribute untouched. Correct today, and unpinned.
flowchart LR
A["<Level.Item as='a' download>"] --> B{carriesLinks?}
B -- no --> C["strip · component TODO"]
B -- yes --> D{element takes it?}
D -- no --> E["strip · element TODO"]
D -- yes --> F["keep — the shipped fix"]
F --> G["compile matrix: status 0"]
E --> G
G --> H["green either way"]
| // The premise that let the per-target table go (#672): every href-capable | ||
| // target either follows `as` or carries the anchor's attributes outright, | ||
| // so the element decides for all of them. If one ever narrows, the rule | ||
| // would silently keep an attribute the component rejects -- so the premise | ||
| // is asserted rather than assumed, for every target rather than for the | ||
| // ones that had no row. |
There was a problem hiding this comment.
A third file still narrates the deleted mechanism, and says the opposite of this premise — 🟡 Minor · Correctness
(Anchored here because the file it is about is not in this diff — the defect is in bestax-migrate/e2e/polymorphic-output.test.ts:111-123, which this sweep missed.)
What: This comment states the premise correctly. Two comments in e2e/polymorphic-output.test.ts state its negation:
:115— "Level.Itemenumerates its own props and stops attarget." False. Since [Bug] bulma-ui: Level.Item's props do not follow its constrainedas, in both directions #672,LevelItemProps extends AnchorOnlyAttributes = Omit<React.AnchorHTMLAttributes<HTMLAnchorElement>, keyof React.HTMLAttributes<HTMLAnchorElement>>(bulma-ui/src/layout/Level.tsx:183) — the anchor's whole surface.Level.tsx:198says it outright: "What this list must NOT become is a shorter one. Enumerating onlyhref,targetandrelmade every other anchor attribute a type error even atas="a", which is what [Bug] bulma-ui: props and ref types do not follow the polymorphicas#641 reported.":121-122— "so the component and the element both have to allow it" is the two-check rule this PR deletes.Level.Itemis inHREF_OK, so only the element decides for it.
Why it matters: Those are the lines explaining why <Level.Item as="a" href="/x" download> (:119) is in the matrix. Read as written they say Level.Item rejects download — the pre-#672 defect, and the negation of the premise this whole deletion rests on. It is the one place left that would talk a reader into re-adding the table, which is the class the steer flagged.
Fix: deletion makes the remainder true.
// Targets whose props do NOT follow `as`: every matrix row for link
// attributes used to sit on the four that do, so the gap was invisible by
// construction. `Dropdown.Item`'s props follow a CONSTRAINED `as`, so the
- // anchor form carries a link and the other two do not; `Level.Item`
- // enumerates its own props and stops at `target`.
+ // anchor form carries a link and the other two do not.
['Dropdown', '<Dropdown.Item href="/x" target="_blank">x</Dropdown.Item>'],
['Dropdown', '<Dropdown.Item as="a" target="_blank">x</Dropdown.Item>'],
['Level', '<Level.Item as="a" href="/x" target="_blank">x</Level.Item>'],
['Level', '<Level.Item as="a" href="/x" download>x</Level.Item>'],
['Tab', '<Tab target="_blank">One</Tab>'],
- // `Level.Item` declares `target`, but forwards it only for an <a>, so the
- // component and the element both have to allow it.
+ // `Level.Item` declares `target` at every `as`, but forwards it only for an
+ // <a>, so the element is what refuses it here.
['Level', '<Level.Item as="p" target="_blank">x</Level.Item>'],How I confirmed it, and that this is the only one left
git show --stat 59e8e348 (the #675 squash) touched bestax-migrate/e2e/as-unions.test.ts and bestax-migrate/src/sources/_shared/polymorphic.ts but not e2e/polymorphic-output.test.ts — so these comments predate #672's change to Level.Item and were never revisited.
Sweep for anything else still narrating the mechanism, across bestax-migrate/, skills/, docs/docs/ and bestax-mcp/data/:
| Grep | Surviving hit |
|---|---|
TARGET_LINK_ATTR, TARGET_LINK_ATTRS |
none anywhere |
declares its own props rather than taking the element, not among them |
none |
both have to allow, component and the element |
e2e/polymorphic-output.test.ts:122, plus CHANGELOG.md (history, correctly left) |
per-target, enumerat |
e2e/polymorphic-output.test.ts:115, plus the two lines this PR rewrote |
So once those two clauses go, the narration is gone.
There was a problem hiding this comment.
Deep review — 2 blocking · 3 advisory
| N | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🟡 Minor | Coverage | The "attribute survives" path is asserted by nothing. <Level.Item as="a" download> — the behaviour this release exists to ship — is only a compile-matrix row, and typecheckTsxFiles returns status: 0 whether the attribute survived or was stripped with a TODO. Same for the referrerPolicy-on-<img> row, whose comment claims it is "kept". |
bestax-migrate/src/sources/_shared/polymorphic.ts:642 |
| 2 | 🟡 Minor | Correctness | A third file still narrates the deleted mechanism, and its version of the story is the negation of this premise: "Level.Item enumerates its own props and stops at target" is false since #672, and "the component and the element both have to allow it" is the rule this PR deletes. Not in the diff, so anchored on the sibling comment. |
bestax-migrate/e2e/polymorphic-output.test.ts:111-123 |
| 3 | 🔵 Advisory | Correctness | PR body, maintainer-only — neither this review nor the fix agent can edit it. The reproduce command does not work as written: pnpm --filter bestax-migrate exec jest … bypasses the NODE_OPTIONS=--experimental-vm-modules that the test script sets, so 17 of the 35 suites fail to load and the figure it prints is coverage over the other 18. pnpm --filter bestax-migrate run test:coverage is the invocation that reproduces. |
PR body, Verification |
| 4 | 🔵 Advisory | Coverage | PR body + commit message. Both say the premise sweep "gets wider" / was "widened … since there are no rows to skip". It already was: TARGET_LINK_ATTRS is empty on main, so if (TARGET_LINK_ATTR_TABLE[target]) continue never fired and all ten targets were already swept. This diff removes a dead continue; the widening landed with #675. |
PR body / commit body |
| 5 | 🔵 Advisory | Robustness | With the mechanism gone, a component that narrows again has no expressible answer — the e2e fails and the remedy is a git revert. Recorded as the deliberate trade it is (the comment says so); on the record rather than as an objection. |
bestax-migrate/src/sources/_shared/polymorphic.ts:634 |
Overall: The deletion itself is correct and I could not break it. TARGET_LINK_ATTRS really was empty on main, so declared was always undefined, the else if (declared && …) arm was unreachable, and the "component" arm of because could only ever be reached with carriesLinks === false — exactly the single message the diff collapses it to. Behaviour is unchanged, and the whole diff is dead code plus its narration. The steer was right to suspect one more: a third file still narrates the mechanism, and what it says is false about Level.Item rather than merely stale (finding 2) — read that one first. The riskier gap is finding 1: this PR is the release vehicle for the download fix in #675, the row that used to strip it is gone, the one test that covered it went with the row, and nothing now pins the fix — all 1213 tests stay green if it regresses.
Residual risk:
- A target outside
HREF_OKthat really does accept a link attribute — the!carriesLinksarm strips it and the TODO claims "not a link at anyas, so it takes no such attribute either". Refuted: onlyLevel.Item,Panel.BlockandPagination.*extendAnchorHTMLAttributes(grep -rn AnchorHTMLAttributes bulma-ui/src), and all three are inHREF_OK; nothing outside it extendsImgHTMLAttributes,LinkHTMLAttributes,ScriptHTMLAttributes,MetaHTMLAttributes,SourceHTMLAttributesorStyleHTMLAttributeseither, so no such component declares any of the six.ImageProps,CardFooterItemProps,CardHeaderIconProps,NavbarDropdownPropsandDeletePropsall bottom out inHTMLAttributesorButtonHTMLAttributes, neither of which declarestarget,download,hrefLang,ping,referrerPolicyormedia. - The widened sweep being vacuous — a swept target whose props carried an index signature would accept every attribute and assert nothing. Refuted:
[key: string]: unknownappears only underbulma-ui/src/form/, no form component is inHREF_OK, andBulmaClassesPropshas none. All ten swept targets typecheck each attribute for real. - A future
HREF_OKrow escaping the sweep. Refuted: the sweep iteratesObject.keys(HREF_TABLE)with thecontinuegone, so a new row is swept automatically, and the missing fallback on theSAMPLElookup is covered by thehas a sample value for every attribute in the tabletest. - Open: finding 1 — the kept-attribute side of
dropInertLinkAttrshas no assertion in any suite, so the released behaviour is unpinned. TwotoContainlines close it. - Verified green here:
test35 suites / 1213 passed (no count drop),as-unions13 passed including the widened sweep,test:coveragegivespolymorphic.ts100 / 94.78 / 100 / 100 with no branch miss at the collapsed ternary,typecheckandlintclean,check:conformanceall green (includingfragile-prose— "Two ways" sits below its number-word floor, and it does not read.tscomments anyway). No reference toTARGET_LINK_ATTR_TABLEsurvives anywhere in the tree, andbestax-migrateships abinwith nomain,exportsortypes, so dropping the export breaks no consumer. The release premise holds too:release.config.jscarries arefactorplusbestax-migraterule at patch behind the rule that makes every other scope non-releasing, so thefix(bulma-ui)squash of #675 could not publish the codemod half and this scope can.
🏄 Clean paddle-out, dude — this one is mostly deleting a wave that already broke, and the code side is glassy. Two bits of driftwood left in the lineup: a comment in a third file telling the old story backwards, and a shipped fix nobody roped a test to. Patch those and it is all-time.
It existed for one row. `Level.Item` listed `href`, `target` and `rel` and stopped, so the element alone could not say that `download` was rejected — #672 made it carry the anchor's whole surface and emptied the table, and since then the branch that read it has had no live caller. Nor has the TODO message it selected: "declares its own props rather than taking the element's" is text nothing can reach. Deleted rather than kept for a hypothetical. Restoring it is a revert away if a component ever narrows again, and the e2e keeps asserting that none does. That sweep already covered every target — the table was empty, so its `continue` never fired; what goes here is the dead `continue`, not a narrowing. Reproduce the coverage effect with `pnpm --filter bestax-migrate run test:coverage`, which carries the `--experimental-vm-modules` the suites need. Its narration goes too, which is most of this diff. The loop's preamble still opened with "Both have to allow it" and explained a row deciding the message; the component side is now only "is this a link at any `as`", and everything else is the element's call. The `describe` still called the table an "exception", the deleted export left its JSDoc documenting the declaration after it, and a third file said `Level.Item` "enumerates its own props and stops at `target`" — false since #672 rather than merely stale. The kept-attribute side had no assertion anywhere, which matters because this is the release vehicle for that fix: the compile matrix passes whether the codemod keeps `download` on an `as="a"` or strips it with a TODO. Both directions are pinned now, each against the string that can actually show it — a TODO quotes the attribute it removed, so an absence asserted on the raw output passes either way. Refs #672
d2e7b35 to
8f35ce6
Compare
Preview DeploymentPreview URL: https://a2ff71c7.bestax.pages.dev |
There was a problem hiding this comment.
Deep review — 0 blocking · 2 advisory
| # | Severity | Area | Finding | Location |
|---|---|---|---|---|
| 1 | 🔵 Advisory | Coverage | The strip-side assertion is unkeyed to the rule that fired: toContain('TODO(bestax-migrate)') passes on any TODO, so a regression that dropped download for a different reason (or emitted the wrong message) still goes green. The keep-side pair above it is precise; this one is not. |
bestax-migrate/e2e/polymorphic-output.test.ts:308 |
| 2 | 🔵 Advisory | Correctness | Imprecise, not false: the renamed sweep says it holds each target to "its anchor's whole surface", while what it iterates is LINK_ATTR_TABLE — the six inert-able attributes. href, rel and type are covered by other suites in the same file, not this one. |
bestax-migrate/e2e/as-unions.test.ts:355 |
Overall: The deletion is behaviour-preserving and I could not break it. I re-derived the premise from the history rather than the PR body: git show 59e8e348:…/polymorphic.ts has TARGET_LINK_ATTRS = {} on main, so declared was always undefined, the else if (declared && …) arm was unreachable, and the component arm of because could only ever be taken with carriesLinks === false — exactly the one message the diff collapses it to. The if (TARGET_LINK_ATTR_TABLE[target]) continue the sweep loses never fired for the same reason, so the e2e's coverage is unchanged in fact and only its shape widened. The steer's class — surviving narration of the deleted mechanism — is now clean: TARGET_LINK_ATTR appears nowhere in the tree, and both have to allow / per-target / enumerat survive only in the two rewritten comments and in CHANGELOG.md, which is history and correctly left. Both blocking findings from the earlier pass are answered in this tree (the polymorphic-output.test.ts:111-123 comment now states the post-#672 rule, and the keep-side of dropInertLinkAttrs is asserted for the first time) — their inline threads can be resolved. What deserves the human's eye is not the code but the squash title: the whole point of this PR is the release, and refactor(bestax-migrate) is what the patch rule matches.
What I ran, and what I checked it against
| Command | Result |
|---|---|
pnpm --filter @allxsmith/bestax-bulma run build |
needed first — the e2e typechecks against dist |
pnpm --filter bestax-migrate run test |
35 suites / 1213 passed |
pnpm --filter bestax-migrate run test:coverage |
all files 95.6 / 87.29 (gate 95 / 78); polymorphic.ts 100 / 94.78 / 100 / 100, misses at 561, 657-671, 765-774 — not at the collapsed ternary |
pnpm --filter bestax-migrate run typecheck / lint / format:check |
clean |
pnpm run check:conformance |
21/21 green, fragile-prose included |
Claims chased to their sources:
- "
bestax-migrate@2.3.3still stripsdownload" — true.git show 8ec06ca2:bestax-migrate/src/sources/_shared/polymorphic.ts(the 2.3.3 release commit) carriesTARGET_LINK_ATTRS = { 'Level.Item': ['target'] }, sodownloadon<Level.Item as="a">took thecomponentarm and the now-deleted TODO text. - The release fires —
bestax-migrate/release.config.jshas{ type: 'refactor', scope: 'bestax-migrate', release: 'patch' }, and the always-winning{ scope: '!(bestax-migrate)', release: false }does not match this scope. - The new keep-side assertions are not vacuous —
addTodofolds every line terminator out of the message (jsx-utils.ts:252), so a TODO is always exactly one line andcodeOf'sstartsWith('// TODO(bestax-migrate)')filter really does remove all of it. A strippeddownload="f"would therefore fail bothcodeOf(widerRaw)).toContain(…)andexpect(widerRaw).not.toContain('TODO(bestax-migrate)'). - #672 is
Refs, notFixes— correct: the issue is still open for the source-breakingnext-majorhalf.
Residual risk:
- The premise sweep is type-level; the failure it guards is partly runtime.
HREF_OK's own doc says typechecking cannot answer the question alone —Level.Itemtypeshrefat everyasand drops it at runtime. A component that kept the type and filtered an attribute while rendering an anchor would pass the sweep. Refuted for today's library:LevelItem's anchor branch spreadsrestwhole (bulma-ui/src/layout/Level.tsx:281), and every otherHREF_OKtarget either followsasor extendsAnchorHTMLAttributes. Unchanged by this diff. - A target outside
HREF_OKthat really does accept a link attribute would get the!carriesLinksmessage claiming it "is not a link at anyas". Refuted: the only other components extendingAnchorHTMLAttributesareLinkandLinkButton(grep -rln AnchorHTMLAttributes bulma-ui/src), and neither is a migration target in any source mapping —Pagination.*,Panel.Block,Level.ItemandButtonare all inHREF_OK. - A future
HREF_OKrow escaping the sweep — refuted: with thecontinuegone the loop isObject.keys(HREF_TABLE)unconditionally, a missingSAMPLEis caught by "has a sample value for every attribute in the table", andHREF_OKitself is held complete against the library by the suite above it. - Open: advisory 1 — the
as="p"direction is pinned to "some TODO exists", not to the element-rejection message, so that half of the pair is weaker than the half beside it. - Not a residual risk, on the record: with the mechanism gone, a component that narrows again has no expressible answer; the e2e fails loudly and the remedy is a revert. The diff says so, and I agree it is the right trade.
🏄 The wave already broke back in #675, dude — this just paddles the dead board back to shore and ties a leash on the
downloadfix that was floating free. Lineup's clean, no narration wreckage left in the water. Send it. 🌊
|
🎉 This PR is included in version 2.3.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 5.15.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.2.5 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 4.2.9 🎉 The release is available on: Your semantic-release bot 📦🚀 |
2 participants
Follow-up to #675.
The dead mechanism
TARGET_LINK_ATTRSrecorded the link attributes a target accepts when its props do not followas. It existed for exactly one row:Level.Itemenumeratedhref,targetandreland stopped, so the element alone could not say thatdownloadwas rejected.#672 made it carry the anchor's whole surface by subtracting
HTMLAttributesfromAnchorHTMLAttributes, the row went, and since then:const declared = TARGET_LINK_ATTRS[target]is alwaysundefinedelse if (declared && !declared.includes(name))short-circuits for every attribute of every target, foreverKept, it is a mechanism no test can exercise, since the only test that ever did was deleted with the row. Deleted, it is a
git revertaway the day a component narrows again.The e2e premise check stays: every href-capable target accepts every link attribute on its anchor. It already covered all of them — the table has been empty since #675, so the
if (TARGET_LINK_ATTR_TABLE[target]) continueguard never fired. What goes here is that deadcontinue, not a narrowing; the widening landed with #675.Most of the diff is the narration going with it — the loop preamble that opened "Both have to allow it" and explained a row deciding the TODO's wording, a
describestill calling the table an "exception", and the JSDoc the deleted export left behind documenting whatever followed it. The one case worth keeping is recorded as history, because it is the shape that would bring the table back: a component enumerating a subset of the link attributes, narrower than the element it renders.Why this PR exists at all
#675 was squash-merged under
fix(bulma-ui), andbestax-migrate/release.config.jsopens with{ scope: '!(bestax-migrate)', release: false }— so the codemod changes in that squash reachedmainbut were never published. Todaybestax-migrate@2.3.3still stripsdownloadfrom<Level.Item as="a" download>and writes a TODO calling it unsupported, against abulma-uithat now supports it.This PR's
refactor(bestax-migrate)scope cuts that release, so the fix ships. The cleanup is real work rather than a version bump for its own sake, but the release is the reason it is happening now.Verification
pnpm allgreen,check:conformancegreen, migrate suite green. Coverage on the touched file improves because the unreachable arm stops counting against it, not because anything new is tested —pnpm --filter bestax-migrate run test:coveragereproduces it. (Notexec jestdirectly: that bypasses the--experimental-vm-modulesthetestscript sets, half the suites fail to load, and the figure it prints covers only the rest.)The kept-attribute side is now pinned, which it was not. This PR is the release vehicle for #672's
downloadfix, the row that used to strip it is gone, and the one test covering that path went with the row — so the compile matrix passed whether the codemod kept the attribute or stripped it with a TODO. Both directions are asserted now, each against the string that can show it: a TODO quotes the attribute it removed, so an absence checked on the raw output would pass either way.