Skip to content

fix!: type the v11 prop contracts (discriminated unions, no index signatures) - #3252

Open
Niznikr wants to merge 10 commits into
masterfrom
v11-typing-the-contract
Open

Niznikr wants to merge 10 commits into
masterfrom
v11-typing-the-contract

Conversation

@Niznikr

@Niznikr Niznikr commented Oct 23, 2026

Copy link
Copy Markdown
Collaborator

Additional description

Breaking change (types only, before GA). Several prop and data types let invalid combinations through, mostly because a | string or an index signature widened them. This PR tightens them into discriminated unions and declared fields. Runtime behaviour is unchanged except for the comboboxFilter fix below. Where old input still works at runtime, a development-only warning now points to the new form.

Component Root cause Change Runtime shim
textarea maxLength: string, parsed with parseInt maxLength: number Strings still parsed, with a dev warning (check-props was never called; it is now)
vertical-navigation NavigationItem index signature; icon / notificationBadge undeclared and cast Declared as ReactElement<IconProps> / ReactElement<BadgeProps>; casts removed —
tree type: 'branch' | 'item' | string widened to string; nodes allowed on any node TreeNode = TreeBranchNode | TreeItemNode —
split-view Index signatures on SplitViewHeaderProps and SplitViewListboxItem dropped the PageHeader and item types Header extends PageHeaderProps; SplitViewListbox<T> carries the item type through options, selection, unread, events and listItem —
color-picker swatchColors / defaultSelectedTab accepted with any variant and silently ignored ColorPickerVariantProps union —
welcome-mat Any prop accepted with any variant WelcomeMatVariantProps union: tiles vs. actions; infoBadge only on trailhead-connected —
menu-dropdown Every field allowed on every option type; leftIcon.category: string Item / header / divider option union; IconCategory —
combobox (options) type: ... | string widened to string. Items reused type as the comboboxAddSubheadings grouping key, so structural rows couldn't be told apart and labels were cast to string Union keyed on type: item (new category field, string label), separator, header/footer, deselect addSubheadings reads category ?? type; dev warning when an item has a non-structural type
combobox (variant) "options required unless popover" and "popover needs assistiveText.popoverLabel" were runtime warnings only ComboboxVariantProps: listbox variants require options; popover requires popover + popoverLabel and rejects options Existing runtime warnings kept

Also in this PR

  • comboboxFilter threw on a separator whose label is a render function. It now matches only string labels (regression test added).
  • onRenderMenuItem / menuItem receive ComboboxItemOption | ComboboxDeselectOption. That's the only data the menu ever passes them.
  • Combobox examples, stories and tests (plus two portal-settings examples) move from type: 'account' to category: 'account'. Rendered markup is identical, so there are no snapshot changes.
  • The combobox stories (53 errors) and the tree hashmap story data (37 errors) didn't type-check on master. Both now do.
  • New type-level regression tests in components/*/__tests__/*.type-test.tsx, using @ts-expect-error assertions. They get their own tsconfig.type-tests.json because the build config excludes __tests__, and npm run typecheck now runs both projects. I checked that each assertion fails for its intended reason.

Migration

  • Combobox items: rename type: '<group>' to category: '<group>'. type is now only 'separator' | 'header' | 'footer' | 'deselect'.
  • Textarea: pass maxLength={100}, not maxLength="100".
  • Tree nodes: set type: 'branch' or type: 'item'. Only branches take nodes / expanded / loading.
  • ColorPickerProps, ComboboxProps and WelcomeMatProps are now type aliases, so interface X extends ComboboxProps no longer compiles. Use type X = ComboboxProps & {...}. Nothing in this repo extends them. MenuDropdownProps stays an interface because builder-header and trial-bar extend it.
  • New exported types: TreeBranchNode, TreeItemNode, ColorPickerVariantProps, WelcomeMatVariantProps, MenuDropdownItemOption / HeaderOption / DividerOption, ComboboxItemOption / SeparatorOption / ActionOption / DeselectOption, ComboboxVariantProps.

Not in scope: the other parts of these ROADMAP checklist items (renames, controlled state, child contracts), so no checkboxes are ticked. Runtime warnings for tree, color-picker and welcome-mat were skipped because the runtime already ignores the invalid props.


CONTRIBUTOR checklist (do not remove)

Please complete for every pull request.

Green gate — all of these must pass locally (CI runs the same):

  • npm run lint passes with 0 errors (npm run lint:fix to auto-fix). Warnings are the tracked gradual-migration backlog; do not add new errors. (223 warnings, same as master)
  • npm run typecheck is clean (tsc --noEmit). Now also runs the type-test project.
  • npm test -- --run passes — this runs both Vitest projects (jsdom unit + real-browser browser) and the story HTML snapshots. If you changed rendered markup on purpose, update snapshots with npm test -- --run -u and review the diff. (1035/1035 with --update=none; no snapshot changes)
  • npm run build succeeds (type-check + Vite library build + .d.ts + entry shims).

Behavior & docs:

  • First-time contributors: sign the CLA (a bot will comment with instructions). (N/A)
  • Added/updated Vitest + React Testing Library tests for the change (template: components/button/__tests__). New props and bug fixes should get a regression test. Tests that genuinely need a real browser (focus traversal, layout/measurement, canvas) go in a *.browser.test.jsx file (the browser project); everything else is jsdom.
  • Reviewed the component's Storybook stories: npm run storybook → http://localhost:6007/. Confirm the story actually renders and behaves (deprecated Storyshots/Karma are gone; there is no separate browser-tests server). (Not yet done; snapshots are unchanged)
  • Story/example code uses the component's real prop API (e.g. render-props like onRenderContent / onRenderVisualPicker, icon props, events), not assumptions — a common source of "component looks broken" reports has been stories authored against the wrong API. Cross-check against components/<name>/__examples__. (The combobox, color-picker, menu-dropdown and tree stories now type-check against the new types)
  • Storybook Accessibility (a11y) panel shows no new violations for the affected stories. (Not yet checked; no markup changes)
  • If you added or changed a type, remember runtime PropTypes were removed in the TS migration — types are the contract. Ship accurate .d.ts (they're generated by the build); JS consumers rely on them via editor type-acquisition.

REVIEWER checklist (do not remove)

  • CI is green (GitHub Actions CI workflow: lint, typecheck, test, build). Chromatic visual review, if configured, has been checked.
  • Change has adequate test coverage; bug fixes include a regression test.
  • Reviewed the affected Storybook stories and their a11y panels.
  • Rendered markup still conforms to SLDS — check the story HTML snapshot diff (components/__tests__/__snapshots__/story-snapshots.test.jsx.snap).
  • Public API / package surface: if exports changed, the deep-import contract still holds (@salesforce/design-system-react/components/<name> and nested paths resolve for ESM, CJS, and types). Removing/renaming a public component or export is a breaking change → feat!:/BREAKING CHANGE: and a major version bump.

Niznikr and others added 10 commits October 23, 2026 16:50
Prettier-only pass over files that were not formatted on master, so the
following fix! commits show only functional changes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`maxLength` was typed `string` and parsed with `parseInt` before reaching
the DOM. It is now `number`. Strings are still parsed at runtime, and the
component now calls its check-props, which warns in development when
`maxLength` is a string. Wiring check-props also enables its existing
`assistiveText`/`label` checks.

BREAKING CHANGE: TextareaProps `maxLength` is `number`; passing a string
is a type error (it still works at runtime, with a development warning).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`NavigationItem` had `[key: string]: unknown`, so `icon` and
`notificationBadge` were undeclared and cast at the point of use. They
are now declared as `ReactElement<IconProps>` and
`ReactElement<BadgeProps>`, and the casts in the item renderer are gone.

BREAKING CHANGE: NavigationItem no longer accepts arbitrary keys; extra
fields on items are a type error.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`type: 'branch' | 'item' | string` collapsed to `string`, and `nodes`,
`expanded` and `loading` were allowed on any node. `TreeNode` is now
`TreeBranchNode | TreeItemNode`; only branches take children or expand.
Both members are exported. The hashmap story data, which did not
type-check, now uses a hashmap node type with child ids.

BREAKING CHANGE: TreeNode `type` must be 'branch' or 'item', and
`nodes`/`expanded`/`loading` are a type error on items.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`SplitViewHeaderProps` and `SplitViewListboxItem` had index signatures,
so the header lost PageHeader's types and list items lost theirs.
`SplitViewHeaderProps` now extends `PageHeaderProps`, and
`SplitViewListbox<T>` carries the item type through `options`,
`selection`, `unread`, the event data and `listItem`.

BREAKING CHANGE: SplitViewHeader and SplitViewListboxItem no longer accept
arbitrary keys; a `listItem` component must accept the same item type as
`options`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`swatchColors` and `defaultSelectedTab` were accepted with any `variant`
and silently ignored where they do not apply. `ColorPickerProps` is now
base props plus `ColorPickerVariantProps`: `base` takes both, `swatches`
takes `swatchColors` only, and `custom` takes neither.

BREAKING CHANGE: ColorPickerProps is a type alias rather than an interface,
so it can no longer be extended with `interface ... extends`; passing
`defaultSelectedTab` to `swatches` or `custom`, or `swatchColors` to
`custom`, is a type error.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every prop was accepted with every `variant`. `WelcomeMatProps` is now
base props plus `WelcomeMatVariantProps`: `steps` and
`trailhead-connected` take tiles (only the latter takes `infoBadge`),
`info-only` takes tiles and actions, and `splash` takes actions only.

BREAKING CHANGE: WelcomeMatProps is a type alias rather than an interface;
`steps`, `trailhead-connected` and `info-only` require children, `splash`
rejects them, and props outside a variant are a type error.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`MenuDropdownOption` allowed every field on every option type, and
`leftIcon.category` was a plain string. Options are now
`MenuDropdownItemOption | MenuDropdownHeaderOption |
MenuDropdownDividerOption`, keyed on `type`; only headers take `divider`,
dividers take no label, and icon categories are `IconCategory`.

BREAKING CHANGE: MenuDropdownOption is a union; fields that do not apply
to an option's `type` are a type error, and icon `category` must be an
IconCategory.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
`ComboboxOption.type` was `'separator' | 'header' | 'footer' | 'deselect'
| string`, which collapsed to `string`: item options reused `type` as a
grouping key for `comboboxAddSubheadings`, so the structural rows could
not be told apart from items, and labels were cast to `string` throughout.

Options are now a union keyed on `type`: items (no `type`, string label,
new `category` field), separators, header/footer actions and the
deselect row. `comboboxAddSubheadings` groups by `category`, falling back
to a legacy `type`, and check-props warns in development when an item
uses a non-structural `type`. `onRenderMenuItem` receives only item or
deselect rows, which is all it is ever called with.

`ComboboxProps` is now base props plus `ComboboxVariantProps`: the
listbox variants require `options`, and `popover` requires `popover` and
`assistiveText.popoverLabel` and rejects `options`. Both rules were
previously runtime warnings only.

`comboboxFilter` threw on a separator whose label is a render function;
it now only matches string labels.

Examples, stories and tests move from `type: 'account'` to
`category: 'account'`; rendered markup is unchanged.

BREAKING CHANGE: item options use `category` instead of a non-structural
`type` (still honored at runtime, with a development warning).
ComboboxProps is a type alias rather than an interface; listbox variants
require `options`, and the popover variant requires `popover` and
`assistiveText.popoverLabel`. `onRenderMenuItem` and `menuItem` components
receive `ComboboxItemOption | ComboboxDeselectOption`.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The build tsconfig excludes `__tests__`, so the type-level regression
tests (`@ts-expect-error` assertions on the v11 type contracts) get their
own project, and `typecheck` runs it after the main check.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
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