feat(react-utilities): make dictionaries strict and get rid of unused generics to improve DX and type safety#23019
Conversation
…generics to improve DX and type safety
…identify issues in react-utilities
…unused generics to improve DX and type safety
📊 Bundle size reportUnchanged fixtures
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 1c7023ccb89fd70eea5c8084e40786981b7df172 (build) |
Perf Analysis (
|
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| AlertMinimalPerf.default | 225 | 195 | 1.15:1 |
| CardMinimalPerf.default | 461 | 401 | 1.15:1 |
| AccordionMinimalPerf.default | 123 | 109 | 1.13:1 |
| AvatarMinimalPerf.default | 155 | 138 | 1.12:1 |
| StatusMinimalPerf.default | 551 | 492 | 1.12:1 |
| MenuButtonMinimalPerf.default | 1429 | 1283 | 1.11:1 |
| TreeMinimalPerf.default | 678 | 615 | 1.1:1 |
| VideoMinimalPerf.default | 559 | 510 | 1.1:1 |
| AttachmentMinimalPerf.default | 131 | 120 | 1.09:1 |
| RosterPerf.default | 908 | 831 | 1.09:1 |
| ButtonMinimalPerf.default | 131 | 121 | 1.08:1 |
| SegmentMinimalPerf.default | 301 | 280 | 1.08:1 |
| TableMinimalPerf.default | 333 | 307 | 1.08:1 |
| TextMinimalPerf.default | 267 | 247 | 1.08:1 |
| BoxMinimalPerf.default | 287 | 267 | 1.07:1 |
| LabelMinimalPerf.default | 303 | 284 | 1.07:1 |
| ItemLayoutMinimalPerf.default | 961 | 909 | 1.06:1 |
| ProviderMergeThemesPerf.default | 1073 | 1012 | 1.06:1 |
| ListWith60ListItems.default | 508 | 482 | 1.05:1 |
| LayoutMinimalPerf.default | 305 | 293 | 1.04:1 |
| ListCommonPerf.default | 518 | 498 | 1.04:1 |
| CheckboxMinimalPerf.default | 2227 | 2168 | 1.03:1 |
| DialogMinimalPerf.default | 652 | 634 | 1.03:1 |
| DividerMinimalPerf.default | 294 | 285 | 1.03:1 |
| DropdownManyItemsPerf.default | 574 | 557 | 1.03:1 |
| FormMinimalPerf.default | 338 | 327 | 1.03:1 |
| ImageMinimalPerf.default | 316 | 307 | 1.03:1 |
| LoaderMinimalPerf.default | 572 | 555 | 1.03:1 |
| RefMinimalPerf.default | 199 | 194 | 1.03:1 |
| TableManyItemsPerf.default | 1598 | 1559 | 1.03:1 |
| ChatDuplicateMessagesPerf.default | 251 | 246 | 1.02:1 |
| EmbedMinimalPerf.default | 3451 | 3376 | 1.02:1 |
| SliderMinimalPerf.default | 1425 | 1392 | 1.02:1 |
| TextAreaMinimalPerf.default | 407 | 400 | 1.02:1 |
| ButtonOverridesMissPerf.default | 1272 | 1257 | 1.01:1 |
| ButtonSlotsPerf.default | 456 | 452 | 1.01:1 |
| FlexMinimalPerf.default | 234 | 231 | 1.01:1 |
| HeaderMinimalPerf.default | 298 | 294 | 1.01:1 |
| MenuMinimalPerf.default | 708 | 704 | 1.01:1 |
| RadioGroupMinimalPerf.default | 362 | 360 | 1.01:1 |
| SkeletonMinimalPerf.default | 290 | 286 | 1.01:1 |
| SplitButtonMinimalPerf.default | 3440 | 3404 | 1.01:1 |
| CustomToolbarPrototype.default | 2269 | 2239 | 1.01:1 |
| AnimationMinimalPerf.default | 446 | 447 | 1:1 |
| AttachmentSlotsPerf.default | 911 | 907 | 1:1 |
| DropdownMinimalPerf.default | 2555 | 2555 | 1:1 |
| HeaderSlotsPerf.default | 614 | 611 | 1:1 |
| ListNestedPerf.default | 402 | 404 | 1:1 |
| ProviderMinimalPerf.default | 335 | 335 | 1:1 |
| GridMinimalPerf.default | 272 | 274 | 0.99:1 |
| ListMinimalPerf.default | 421 | 427 | 0.99:1 |
| ChatWithPopoverPerf.default | 314 | 319 | 0.98:1 |
| InputMinimalPerf.default | 1062 | 1082 | 0.98:1 |
| PopupMinimalPerf.default | 506 | 517 | 0.98:1 |
| IconMinimalPerf.default | 507 | 515 | 0.98:1 |
| ToolbarMinimalPerf.default | 769 | 788 | 0.98:1 |
| TreeWith60ListItems.default | 132 | 135 | 0.98:1 |
| ChatMinimalPerf.default | 597 | 615 | 0.97:1 |
| TooltipMinimalPerf.default | 911 | 943 | 0.97:1 |
| DatepickerMinimalPerf.default | 4649 | 4849 | 0.96:1 |
| ReactionMinimalPerf.default | 280 | 302 | 0.93:1 |
| PortalMinimalPerf.default | 129 | 141 | 0.91:1 |
| CarouselMinimalPerf.default | 349 | 395 | 0.88:1 |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| TeachingBubble | mount | 47490 | 89297 | 5000 | Possible regression |
All results
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| BaseButton | mount | 802 | 704 | 5000 | |
| Breadcrumb | mount | 2331 | 2165 | 1000 | |
| Checkbox | mount | 1181 | 1158 | 5000 | |
| CheckboxBase | mount | 1070 | 1085 | 5000 | |
| ChoiceGroup | mount | 3627 | 3731 | 5000 | |
| ComboBox | mount | 851 | 745 | 1000 | |
| CommandBar | mount | 8788 | 8924 | 1000 | |
| ContextualMenu | mount | 10176 | 10392 | 1000 | |
| DefaultButton | mount | 1005 | 873 | 5000 | |
| DetailsRow | mount | 3102 | 3141 | 5000 | |
| DetailsRowFast | mount | 3058 | 3076 | 5000 | |
| DetailsRowNoStyles | mount | 2957 | 3071 | 5000 | |
| Dialog | mount | 1830 | 1732 | 1000 | |
| DocumentCardTitle | mount | 141 | 153 | 1000 | |
| Dropdown | mount | 2731 | 2750 | 5000 | |
| FocusTrapZone | mount | 1580 | 1451 | 5000 | |
| FocusZone | mount | 1517 | 1558 | 5000 | |
| IconButton | mount | 1383 | 1377 | 5000 | |
| Label | mount | 268 | 302 | 5000 | |
| Layer | mount | 2395 | 2411 | 5000 | |
| Link | mount | 403 | 373 | 5000 | |
| MenuButton | mount | 1215 | 1127 | 5000 | |
| MessageBar | mount | 1660 | 1802 | 5000 | |
| Nav | mount | 2516 | 2799 | 1000 | |
| OverflowSet | mount | 926 | 896 | 5000 | |
| Panel | mount | 1755 | 1841 | 1000 | |
| Persona | mount | 787 | 832 | 1000 | |
| Pivot | mount | 1235 | 1120 | 1000 | |
| PrimaryButton | mount | 1054 | 1064 | 5000 | |
| Rating | mount | 6236 | 6302 | 5000 | |
| SearchBox | mount | 1017 | 1064 | 5000 | |
| Shimmer | mount | 2176 | 2030 | 5000 | |
| Slider | mount | 1555 | 1678 | 5000 | |
| SpinButton | mount | 3994 | 4289 | 5000 | |
| Spinner | mount | 342 | 334 | 5000 | |
| SplitButton | mount | 2563 | 2410 | 5000 | |
| Stack | mount | 401 | 455 | 5000 | |
| StackWithIntrinsicChildren | mount | 1864 | 1886 | 5000 | |
| StackWithTextChildren | mount | 4252 | 4223 | 5000 | |
| SwatchColorPicker | mount | 9368 | 9542 | 5000 | |
| TagPicker | mount | 2276 | 2187 | 5000 | |
| TeachingBubble | mount | 47490 | 89297 | 5000 | Possible regression |
| Text | mount | 368 | 315 | 5000 | |
| TextField | mount | 1238 | 1169 | 5000 | |
| ThemeProvider | mount | 973 | 1043 | 5000 | |
| ThemeProvider | virtual-rerender | 488 | 564 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 1516 | 1511 | 5000 | |
| Toggle | mount | 662 | 708 | 5000 | |
| buttonNative | mount | 97 | 103 | 5000 |
…ule to identify issues in react-utilities
…rid of unused generics to improve DX and type safety
…ncompatible with Slot api
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 947 | 908 | 5000 | |
| Button | mount | 586 | 596 | 5000 | |
| FluentProvider | mount | 1896 | 1990 | 5000 | |
| FluentProviderWithTheme | mount | 292 | 300 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 243 | 224 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 295 | 339 | 10 | |
| MakeStyles | mount | 1706 | 1706 | 50000 |
…nd get rid of unused generics to improve DX and type safety
…trict and get rid of unused generics to improve DX and type safety
| const { onMouseEnter: onMouseEnterOriginal, onKeyDown: onKeyDownOriginal } = rootProps; | ||
|
|
||
| rootProps.onMouseEnter = useEventCallback((e: React.MouseEvent<HTMLElement>) => { | ||
| rootProps.onMouseEnter = useEventCallback((e: React.MouseEvent<HTMLDivElement>) => { |
There was a problem hiding this comment.
because we now have strict type checking and inference, we need to provide correct types instead of wide types
packages/react-components/react-utilities/src/utils/getNativeElementProps.ts
Outdated
Show resolved
Hide resolved
| }, | ||
| }), | ||
| ), | ||
| ) as ButtonState['root'], |
There was a problem hiding this comment.
Because we now have proper getNativeProps types, elements that can have variadic root cannot match with original Props definition where Slot mapped type is used to define the api.
Why?
Slot api design is cannot match with implementation/runtime - Slot uses binary arguments, where 1st generic can only be a 1st string. from component API perspective that's impossible to achieve as Slot is always 1 string or union of multiple strings -> we cannot extract first type from union nor transform unions to arrays.
// impossible transform in TypeScript ❌
type Element = 'div' | 'span' | 'section'
↓↓↓
type SlotCompatibleType = <'div' , 'span' | 'section'>
| 'data-automation-id': 1, | ||
| }, | ||
| divProperties, | ||
| divProperties as typeof divProperties & { |
There was a problem hiding this comment.
because our TS version doesn't support tagged literal as index types we cannot type this properly.
Do we wanna move forward with this (Breaking change) or we should go back to following:
Q: can we determine if partners use this function in their codebase ? We export it from react-components so it's a public API
// Implementation with type unaware Allowed Prop Names
// - this will not return proper type but will allow/resolve data-* aria-* properly
// - basically only provided ExcludedProps will properly resolve the return prop
declare function getNativeProps<
Props extends Record<string, unknown>,
E extends Extract<keyof Props, string> = never
>(
props: Props,
allowedPropNames: string[] | Record<string, 1>,
excludedPropNames?: E[],
): Omit<Props, E>
// Example:
// $ExpectType {a:number, c:number} - actual return value is {a:number} 🆘
const result = getNativeProps({ a: 1, b: 2, c: 3 }, ['a', 'b'], ['b']);|
Because this pull request has not had activity for over 150 days, we're automatically closing it for house-keeping purposes. The pull request will still be available for reference. If it's still relevant to merge at some point, you can reopen or make a new version based on the latest code. |
|
Because this pull request has not had activity for over 150 days, we're automatically closing it for house-keeping purposes. The pull request will still be available for reference. If it's still relevant to merge at some point, you can reopen or make a new version based on the latest code. |

Current Behavior
react-utilities exposes helper functions used almost in every v9 component that:
New Behavior
react-utilities exposes helper functions used almost in every v9 component that:
To prevent these issues , react-utilities adds lint rule that will catch those 🔥
Demo
Strict property maps via
toObjectMapgetNativeProps
getNativeElementProps
getPartitionedNativeProps