fix(types): stop advertising isAction on components that cannot honour it - #517
Merged
Conversation
…our it `isAction` restyles a link as Bitrix24's dashed "action" text. `src/theme/link.ts` is the only theme that implements it, and it is deliberately absent from `linkKeys`, so `pickLinkProps` never forwards it downstream. Everything spreading `LinkProps` inherited it anyway. A consumer could write `isAction` on a `B24Button` or a breadcrumb item, TypeScript accepted it, Vue registered it — `Button` declared 45 runtime props, one of them dead — and nothing changed on screen. Eight types omit it now: `ButtonProps`, `BreadcrumbItem`, `CommandPaletteItem`, `ContextMenuItem`, `DropdownMenuItem`, `FooterColumnLink`, `NavigationMenuItem` and `PageLink`. Anything extending `ButtonProps` follows. Nothing in the repository passed it anywhere but `B24Link`, so no example, playground or doc changes. Not uniform, and the test says so rather than pretending otherwise: `ButtonProps`, `PageLink` and `FooterColumnLink` have no index signature, so the omission is enforceable. The five item types each declare `[key: string]: any` so an item can carry arbitrary attributes — `keyof` therefore includes `string`, and no `Omit` can make `isAction` unassignable on them. Their omission states intent the compiler cannot check, and the spec asserts only the three it can. The guard is in two halves because they fail under different commands: the runtime assertions on `Button.props` and `linkKeys` go red under `vitest run`, the type assertions only under `vue-tsc`. Both mutation-checked — reverting `Button`'s omit reddens two tests, reverting `PageLink`'s reddens typecheck. `isAction` is ours, not upstream's: `nuxt/ui@v4` has no such prop.
Review caught the sweep stopping short: it read `src/runtime/components/*.vue` and never descended into `content/`, so `ContentSearchLink` and `ContentSearchItem` kept advertising the prop. `ContentSearchLink` has no index signature, which makes it one of the enforceable cases — it is asserted with the other three, and reverting its omit reddens typecheck. The sweep is now a search across all of `src/`, and it finds ten types in nine files. That is the whole set.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked issue
None — found while re-measuring #88 against upstream.
Type of change
revert(Scope): ...)Runtime behaviour does not change:
isActionnever reached anything outsideLink, so nothing that worked before stops working. What changes is that TypeScript now rejectsisActionwhere it used to accept it silently. A project passing it toB24Buttonor to a page/footer/content-search link will fail to compile after upgrading.That was weighed: a
BREAKING CHANGEfooter would take the package to a major version for the removal of a prop that never did anything, and a silent patch would hand people a red build with no warning. The middle course is this —fix(types)with the consequence stated plainly here and in the squash body, so it reads as a visible line in the changelog rather than a surprise. The fix is a one-word deletion at the call site.Description
isActionrestyles a link as Bitrix24's dashed "action" text.src/theme/link.tsis the only theme that implements it, and it is deliberately absent fromlinkKeys— sopickLinkPropsnever forwards it downstream.Everything spreading
LinkPropsinherited it anyway. A consumer could writeisActionon aB24Buttonor a breadcrumb item, TypeScript accepted it, Vue registered it, and nothing changed on screen.Buttondeclared it as a real runtime prop:Ten types across nine files omit it now —
ButtonProps,BreadcrumbItem,CommandPaletteItem,ContentSearchItem,ContentSearchLink,ContextMenuItem,DropdownMenuItem,FooterColumnLink,NavigationMenuItem,PageLink— and anything extendingButtonPropsfollows.Linkkeeps it.Nothing in the repository passed it anywhere but
B24Link, so no example, playground or documentation changes.What is enforceable, and what is not
The omission does not mean the same thing everywhere, and the spec says so rather than pretending:
ButtonPropsPageLinkFooterColumnLinkContentSearchLinkBreadcrumbItem,DropdownMenuItem,NavigationMenuItem,ContextMenuItem,CommandPaletteItem,ContentSearchItem[key: string]: anyThose six accept arbitrary attributes by design, so
keyofincludesstringand noOmitcan makeisActionunassignable on them. Their omission states intent the compiler cannot check. The spec asserts only the four it can — a test that cannot fail is worse than none.The guard, and why it is in two halves
They fail under different commands, and neither covers the other:
expect(Object.keys(Button.props)).not.toContain('isAction')andexpect(linkKeys).not.toContain('isAction')fail undervitest run.expectTypeOfassertions are erased at runtime; onlyvue-tsc(pnpm run typecheck) checks them.Both mutation-checked: reverting
Button's omit reddens two tests invitest run; revertingPageLink's orContentSearchLink's reddenstypecheck.The type predicate is written out as
type HasIsAction<T> = 'isAction' extends keyof T ? true : falserather than usingexpectTypeOf().not.toHaveProperty(). The latter was tried first and gave results that did not match how it reads on types with an index signature, witherror TS2554: Expected 2 arguments, but got 1as its failure message.Review
/reviewcaught the first sweep reading onlysrc/runtime/components/*.vueand never descending intocontent/, leavingContentSearchLinkandContentSearchItemstill advertising the prop. Fixed in the second commit; the sweep is now a search across all ofsrc/, andContentSearchLinkis mutation-checked with the rest.The five-reviewer panel was not convened: nine one-line type edits and one spec, with both halves already proven by mutation.
Provenance
isActionis ours, not inherited:nuxt/ui@v4has no such prop anywhere.Gate
lint,typecheck, 322 test files / 7498 tests,test:module, and coverage 71.89 / 69.93 / 71.19 / 71.44 against thresholds of 70 / 68 / 70 / 70.Checklist