fix(react-breadcrumb): make BreadcrumbButtonBaseProps distribute over the ARIA button union - #36663
Open
Ray Knight (ArrayKnight) wants to merge 5 commits into
Conversation
… the ARIA button union
`BreadcrumbButtonBaseProps` was declared with a plain `Omit`:
export type BreadcrumbButtonBaseProps = Omit<BreadcrumbButtonProps, 'size'>;
`BreadcrumbButtonProps` includes `ComponentProps<ButtonSlots>`, whose `root`
slot is `ARIAButtonSlotProps<'a'>` -- a union over `{ as?: 'button' } & button
attrs` and `{ as: 'a' } & anchor attrs`. Plain `Omit` is `Pick<T, Exclude<keyof
T, K>>`, and `keyof` a union keeps only the keys common to every member, so the
omit collapses the union and every anchor-only prop (`href`, `target`, `rel`,
...) disappears from the derived type. `@fluentui/react-button` avoids exactly
this on the same shape by using `DistributiveOmit`
(Button.types.ts:72,84); this makes `react-breadcrumb` consistent with it.
Healed -- all three now type-check against the base surface where none did
before:
<BreadcrumbButton href="#a"> (the spelling react-breadcrumb's
own Default story uses)
<BreadcrumbButton as="a" href="#a">
<BreadcrumbButton as="a" href target rel>
Not changed, and honestly not a regression: a props object literal whose only
property is `href` is still rejected. That is TypeScript weak-type detection --
the `{ as?: 'button' }` union member has no required properties and shares no
property with `{ href }`, so the source only matches the `a` member, which then
demands an explicit `as: 'a'`. It fires identically on the Griffel
`BreadcrumbButtonProps`, and adding any shared property (`children`, which every
real JSX usage has) satisfies it on both. Runtime behaviour is unchanged: this
file declares types only and emits nothing.
etc/react-breadcrumb.api.md regenerated by the build.
Verified: react-breadcrumb type-check + lint pass; react-headless-components-preview
and react-components type-check pass. react-breadcrumb:test is 2 failed / 105
passed both with and without this change (pre-existing @fluentui/react-icons
snapshot drift -- SVG path data and the `fui-Icon` class -- unrelated to it).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013wmpBCYJpDJCLXcScCWz1i
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Aj9uA3rCVgosnh2zNn8qkc
📊 Bundle size report
Unchanged fixtures
|
|
Pull request demo site: URL |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…rumbButtonBaseProps
| // union and dropped the anchor arm's `href`. These assignments are validated by the package's | ||
| // type-check target. | ||
| it('keeps both arms of the ARIA button union assignable to BreadcrumbButtonBaseProps', () => { | ||
| const anchorProps: BreadcrumbButtonBaseProps = { as: 'a', href: '/somewhere' }; |
Contributor
There was a problem hiding this comment.
apologies for the confusion, but could we test the base hook with needed args, something like:
const { result } = renderHook(() => useBreadcrumbButtonBase({ as: 'a', href: '/somewhere' })
expect(result.current).toMatchObject({
components: { root: 'a' },
root: { href: '/somewhere' }
})
it will cover both base hook and types, as we run typecheck against tests as well
Contributor
Author
There was a problem hiding this comment.
Done in 6c355b2 — reworked to the renderHook shape. One adjustment from your sketch: components.root stays 'button' (the slot's declared default element type in useButtonBase); the anchor arm resolves through the slot props, where useARIAButtonProps carries as: 'a' to the render layer. So the assertion pins root: { as: 'a', href: '/somewhere' }, which is the field that actually witnesses the anchor path. 108/108 green.
Contributor
There was a problem hiding this comment.
lets clean up the unnecessary comments in tests and should be good to go.
thank you!
…Type ternary (microsoft#36681) `as ?? href ? 'a' : 'button'` parses as `(as ?? href) ? 'a' : 'button'`, so an explicit `as: 'button'` (truthy) computed controlType 'a' and the ARIA button pipeline emitted the anchor arm (as: 'a', role="button") for a caller who asked for a real <button>. Parenthesize so an explicit `as` short-circuits the href inference, and add a red/green renderHook regression test alongside the existing anchor-arm test.
Contributor
Author
|
Dmytro Kirpa (@dmytrokirpa) While adding the base-hook test you asked for, we hit an operator-precedence bug in the same hook: |
Dmytro Kirpa (dmytrokirpa)
approved these changes
Sep 2, 2026
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
2 participants
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.
BreadcrumbButtonBasePropsis declared as a plainOmit<BreadcrumbButtonProps, 'size'>.BreadcrumbButtonPropsincludesComponentProps<ButtonSlots>, whose root isARIAButtonSlotProps— a distributive union overas: 'button' | 'a'. A plainOmitcollapses that union into a single object type keyed on the intersection of its members, so the anchor arm'shrefdisappears and the anchor spelling ofBreadcrumbButton(the one the component's own Default story uses) no longer type-checks against the base props type.The fix switches to
DistributiveOmit, matching what@fluentui/react-buttonalready does for the same reason. Type-level only — no runtime change — and the widened type is strictly a superset of the current one, so no existing consumer code stops compiling. Theetc/react-breadcrumb.api.mdreport is regenerated to match.Fixes #36645.
Extracted from #36656 per maintainer request — each in-tree fix from that PR as an isolated change.
Also fixes #36681: while adding the requested base-hook test, we found that
controlType'sas ?? href ? 'a' : 'button'parses as(as ?? href) ? 'a' : 'button', so an explicitas: 'button'(truthy) computed'a'and the ARIA button pipeline emitted the anchor arm (<a role="button">). Parenthesized so an explicitasshort-circuits thehrefinference, with a red/green renderHook regression test alongside the existing anchor-arm test.