Props use types over interfaces #19850
Conversation
…rops-use-types-over-interfaces
…rops-use-types-over-interfaces
GeoffCoxMSFT
requested review from
behowell,
dzearing,
khmakoto,
layershifter,
ling1726 and
a team
as code owners
📊 Bundle size report
Unchanged fixtures
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit cdc03f6:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 6d3408ab7a96381825b2925f64dbb388811bd36d (build) |
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 1036 | 1006 | 5000 | |
| BaseButton | mount | 1024 | 1026 | 5000 | |
| Breadcrumb | mount | 2781 | 2810 | 1000 | |
| ButtonNext | mount | 472 | 487 | 5000 | |
| Checkbox | mount | 1645 | 1667 | 5000 | |
| CheckboxBase | mount | 1462 | 1428 | 5000 | |
| ChoiceGroup | mount | 5269 | 5244 | 5000 | |
| ComboBox | mount | 1117 | 1073 | 1000 | |
| CommandBar | mount | 10796 | 10819 | 1000 | |
| ContextualMenu | mount | 6820 | 7088 | 1000 | |
| DefaultButton | mount | 1214 | 1232 | 5000 | |
| DetailsRow | mount | 4082 | 3973 | 5000 | |
| DetailsRowFast | mount | 4122 | 4066 | 5000 | |
| DetailsRowNoStyles | mount | 3892 | 3969 | 5000 | |
| Dialog | mount | 2596 | 2531 | 1000 | |
| DocumentCardTitle | mount | 155 | 154 | 1000 | |
| Dropdown | mount | 3495 | 3581 | 5000 | |
| FluentProviderNext | mount | 7368 | 7309 | 5000 | |
| FluentProviderWithTheme | mount | 368 | 349 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 101 | 97 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 521 | 495 | 10 | |
| FocusTrapZone | mount | 1885 | 1926 | 5000 | |
| FocusZone | mount | 1921 | 1912 | 5000 | |
| IconButton | mount | 1880 | 1917 | 5000 | |
| Label | mount | 354 | 353 | 5000 | |
| Layer | mount | 3213 | 3347 | 5000 | |
| Link | mount | 512 | 523 | 5000 | |
| MakeStyles | mount | 1913 | 1894 | 50000 | |
| MenuButton | mount | 1594 | 1614 | 5000 | |
| MessageBar | mount | 2166 | 2166 | 5000 | |
| Nav | mount | 3531 | 3551 | 1000 | |
| OverflowSet | mount | 1195 | 1274 | 5000 | |
| Panel | mount | 2542 | 2545 | 1000 | |
| Persona | mount | 925 | 891 | 1000 | |
| Pivot | mount | 1552 | 1520 | 1000 | |
| PrimaryButton | mount | 1444 | 1454 | 5000 | |
| Rating | mount | 8558 | 8749 | 5000 | |
| SearchBox | mount | 1478 | 1516 | 5000 | |
| Shimmer | mount | 2752 | 2845 | 5000 | |
| Slider | mount | 2137 | 2090 | 5000 | |
| SpinButton | mount | 5747 | 5606 | 5000 | |
| Spinner | mount | 441 | 458 | 5000 | |
| SplitButton | mount | 3493 | 3495 | 5000 | |
| Stack | mount | 541 | 550 | 5000 | |
| StackWithIntrinsicChildren | mount | 1703 | 1774 | 5000 | |
| StackWithTextChildren | mount | 5155 | 5031 | 5000 | |
| SwatchColorPicker | mount | 11039 | 11076 | 5000 | |
| Tabs | mount | 1493 | 1635 | 1000 | |
| TagPicker | mount | 2872 | 2840 | 5000 | |
| TeachingBubble | mount | 13944 | 13948 | 5000 | |
| Text | mount | 439 | 455 | 5000 | |
| TextField | mount | 1510 | 1505 | 5000 | |
| ThemeProvider | mount | 1229 | 1229 | 5000 | |
| ThemeProvider | virtual-rerender | 605 | 600 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 2051 | 2037 | 5000 | |
| Toggle | mount | 893 | 857 | 5000 | |
| buttonNative | mount | 114 | 120 | 5000 |
Perf Analysis (@fluentui/react-northstar)
Perf tests with no regressions
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| ChatDuplicateMessagesPerf.default | 351 | 312 | 1.13:1 |
| TreeMinimalPerf.default | 939 | 847 | 1.11:1 |
| ButtonMinimalPerf.default | 201 | 185 | 1.09:1 |
| FlexMinimalPerf.default | 330 | 304 | 1.09:1 |
| AccordionMinimalPerf.default | 166 | 155 | 1.07:1 |
| RefMinimalPerf.default | 244 | 229 | 1.07:1 |
| TableMinimalPerf.default | 472 | 441 | 1.07:1 |
| AnimationMinimalPerf.default | 461 | 440 | 1.05:1 |
| AttachmentMinimalPerf.default | 177 | 168 | 1.05:1 |
| BoxMinimalPerf.default | 393 | 373 | 1.05:1 |
| DividerMinimalPerf.default | 411 | 393 | 1.05:1 |
| HeaderSlotsPerf.default | 880 | 840 | 1.05:1 |
| LabelMinimalPerf.default | 450 | 430 | 1.05:1 |
| FormMinimalPerf.default | 468 | 451 | 1.04:1 |
| ImageMinimalPerf.default | 418 | 403 | 1.04:1 |
| ListCommonPerf.default | 729 | 700 | 1.04:1 |
| ListMinimalPerf.default | 575 | 555 | 1.04:1 |
| MenuMinimalPerf.default | 965 | 927 | 1.04:1 |
| PortalMinimalPerf.default | 194 | 186 | 1.04:1 |
| RadioGroupMinimalPerf.default | 515 | 497 | 1.04:1 |
| TextMinimalPerf.default | 381 | 366 | 1.04:1 |
| CardMinimalPerf.default | 620 | 602 | 1.03:1 |
| ChatMinimalPerf.default | 735 | 711 | 1.03:1 |
| DatepickerMinimalPerf.default | 5928 | 5780 | 1.03:1 |
| DialogMinimalPerf.default | 834 | 808 | 1.03:1 |
| DropdownManyItemsPerf.default | 767 | 746 | 1.03:1 |
| GridMinimalPerf.default | 376 | 364 | 1.03:1 |
| TableManyItemsPerf.default | 2146 | 2084 | 1.03:1 |
| AlertMinimalPerf.default | 309 | 304 | 1.02:1 |
| AttachmentSlotsPerf.default | 1176 | 1148 | 1.02:1 |
| CarouselMinimalPerf.default | 536 | 526 | 1.02:1 |
| CheckboxMinimalPerf.default | 2957 | 2905 | 1.02:1 |
| HeaderMinimalPerf.default | 387 | 381 | 1.02:1 |
| LoaderMinimalPerf.default | 750 | 735 | 1.02:1 |
| ReactionMinimalPerf.default | 437 | 428 | 1.02:1 |
| SegmentMinimalPerf.default | 392 | 385 | 1.02:1 |
| SkeletonMinimalPerf.default | 422 | 414 | 1.02:1 |
| SliderMinimalPerf.default | 1768 | 1730 | 1.02:1 |
| IconMinimalPerf.default | 678 | 666 | 1.02:1 |
| ToolbarMinimalPerf.default | 1035 | 1019 | 1.02:1 |
| TreeWith60ListItems.default | 192 | 188 | 1.02:1 |
| AvatarMinimalPerf.default | 218 | 215 | 1.01:1 |
| EmbedMinimalPerf.default | 4498 | 4435 | 1.01:1 |
| InputMinimalPerf.default | 1377 | 1364 | 1.01:1 |
| ItemLayoutMinimalPerf.default | 1355 | 1346 | 1.01:1 |
| LayoutMinimalPerf.default | 408 | 404 | 1.01:1 |
| PopupMinimalPerf.default | 636 | 630 | 1.01:1 |
| CustomToolbarPrototype.default | 4088 | 4034 | 1.01:1 |
| DropdownMinimalPerf.default | 3253 | 3247 | 1:1 |
| ProviderMergeThemesPerf.default | 1745 | 1738 | 1:1 |
| SplitButtonMinimalPerf.default | 4669 | 4666 | 1:1 |
| StatusMinimalPerf.default | 735 | 732 | 1:1 |
| VideoMinimalPerf.default | 699 | 696 | 1:1 |
| ListWith60ListItems.default | 689 | 697 | 0.99:1 |
| TooltipMinimalPerf.default | 1126 | 1132 | 0.99:1 |
| ButtonOverridesMissPerf.default | 1824 | 1869 | 0.98:1 |
| ChatWithPopoverPerf.default | 403 | 411 | 0.98:1 |
| ListNestedPerf.default | 595 | 608 | 0.98:1 |
| RosterPerf.default | 1286 | 1330 | 0.97:1 |
| ButtonSlotsPerf.default | 598 | 622 | 0.96:1 |
| ProviderMinimalPerf.default | 1101 | 1149 | 0.96:1 |
| MenuButtonMinimalPerf.default | 1803 | 1903 | 0.95:1 |
| TextAreaMinimalPerf.default | 548 | 574 | 0.95:1 |
| inline?: boolean; | ||
| onToggle?(event: AccordionToggleEvent, data: AccordionToggleData): void; | ||
| } | ||
| export type AccordionProps = ComponentProps & |
There was a problem hiding this comment.
Rather than continue to keep this in sync with the actual props, should we just update this to point to the react-*.api.md or the actual types file (tho given the actual typoes are a little harder to read)
There was a problem hiding this comment.
Agreed, after implementation is done I don't think there's much sense to having the types fully duplicated in the spec
| import { ShorthandRenderFunction } from '@fluentui/react-utilities'; | ||
|
|
||
| // @public | ||
| export const Accordion: React_2.ForwardRefExoticComponent<AccordionProps & React_2.RefAttributes<HTMLDivElement>>; | ||
| export const Accordion: React_2.ForwardRefExoticComponent<Pick<{ |
There was a problem hiding this comment.
We talked about this a bit, but I'll ask here.
Do we know a way to avoid, mitigate, or reduce this type explosion?
It being future work is OK.
There was a problem hiding this comment.
Explicitly declaring the types fixes it. export const Accordion: SomeExplicitType = ...
| @@ -290,7 +290,7 @@ export function getCallbackArguments( | |||
| ); | |||
| } | |||
|
|
|||
| const propertySignature = findPropertySignature(program, sourceFile, typeName, propertyName); | |||
| const propertySignature = findPropertySignature(program.getTypeChecker(), sourceFile, typeName, propertyName); | |||
There was a problem hiding this comment.
curious, these tests have been running in the repo for a while now, why was this necessary in this PR ?
There was a problem hiding this comment.
The existing method only inspects for interfaces. With the change to types, the props themselves can be intersection types. I could inspect the type declarations directly, but I would have to iterate over the types array and then recursively handle any types that were themselves intersection types. The TypeChecker provides this aggregation of properties into a single list to check.
…rops-use-types-over-interfaces
…rops-use-types-over-interfaces
To be clear I am not against For example, folks from Bloomberg have completely different recommendation based on their experience:
https://www.techatbloomberg.com/blog/10-insights-adopting-typescript-at-scale/ I noticed that #19823 is already closed, but what is the plan with using |
* Updated interfaces to types * Merge branch 'master' of https://github.com/microsoft/fluentui into props-use-types-over-interfaces * Change files * Updated API.md for slider * Updated API definition * Updated getCallbackArguments for complex types * Fixed missing arg * Change files * Fixed broken test * Prevent re-evaluating statements recursively.
Pull request checklist
$ yarn changeDescription of changes
See issue and RFC for details.
Generally we should prefer types over interfaces for component props as React hooks is a functional programming approach.
Focus areas to test
Component owners should verify the change to types does not affect the shape of the component's properties.