RFCish: Manual check of vNext props consistency #19775
Conversation
|
|
||
| ## Nit: Explicit component type declaration | ||
|
|
||
| > Some components declare their type explicity, others do not. |
There was a problem hiding this comment.
From @ecraig12345 in HackMD:
At least IMO it's strongly preferable to have explicit type declarations, and this is one of the things I'd like to write a conformance test for. (Also the & React.RefAttributes part isn't needed because it's included in ComponentProps somewhere.) It's a cleaner API from a consumer standpoint and sometimes shows up MUCH cleaner in API reports by preventing unnecessary type expansion.
It can be simplified to: export const Accordion: React.FunctionComponent<AccordionProps> = React.forwardRef((props, ref) => ...)
There was a problem hiding this comment.
from Guest Rodriquez in HackMD:
#19614 (comment)
There was a problem hiding this comment.
from Guest Weaver in HackMD:
Please use at least ForwardRefExoticComponent
|
|
||
| ## Slots and HTML attributes | ||
|
|
||
| > Only Accordian and Menu components use ComponentProps<> with slots currently. |
There was a problem hiding this comment.
from @ecraig12345 in HackMD:
Maybe this should be obvious but I'm not totally clear on what you mean here, sorry. Can you give an example of what it looks like in a component that doesn't use this?
There was a problem hiding this comment.
This isn't a consistency problem per se. I think we want to make sure that the types of each of the slots make sense. As the components using the ComponentPropsCompat are converted to using ComponentProps and start having slots then we will want to check the consistency.
| secondaryContent?: React.HTMLAttributes<HTMLElement>; | ||
| ``` | ||
|
|
||
| ## Compat and HTML attributes |
There was a problem hiding this comment.
from @ecraig12345 in HackMD:
This should also be addressed by the simplified prop merging issues
|
|
||
| ## Property overrides of native properties | ||
|
|
||
| > Several components declare properties that are also declared by extending from the DOM types (e.g. React.HtmlAttributes<T>). |
There was a problem hiding this comment.
from @ecraig12345 in HackMD:
This is an interesting one, and a very good case for conformance testing. There are a couple sub-cases:
- Explicitly declaring (as original type) for documentation purposes => probably fine (example: people get confused if the docs for Link don't explicitly mention
href) - Explicitly declaring as limited sub-type => TBD if this is okay. If we allow it, it should be paired with Omit on the native props.
- Accidental reuse of name for a different purpose => should come up with a different name (example: for Input I wanted to use
sizebut had to rename it since that's a native input prop with different meaning)
|
|
||
| checked, children, color, defaultChecked, disabled, href, image, name, onClick, open, ref, target, type, value | ||
|
|
||
| > Several components use declar properties that are also HTML element names. |
There was a problem hiding this comment.
from @ecraig12345 in HackMD:
Are you aware of any cases where this would cause conflicts?
There was a problem hiding this comment.
I don't think it would cause a compile or runtime issue, but it could confuse users who might think it has to be set to an HTML element.
|
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 fce4728:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 90d9972fef1d2d76b0c28e360b05455a1a602915 (build) |
📊 Bundle size reportUnchanged fixtures
|
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Panel | mount | 2439 | 1430 | 1000 | Possible regression |
All results
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 938 | 939 | 5000 | |
| BaseButton | mount | 960 | 944 | 5000 | |
| Breadcrumb | mount | 2768 | 2783 | 1000 | |
| ButtonNext | mount | 461 | 466 | 5000 | |
| Checkbox | mount | 1578 | 1614 | 5000 | |
| CheckboxBase | mount | 1434 | 1341 | 5000 | |
| ChoiceGroup | mount | 4934 | 4948 | 5000 | |
| ComboBox | mount | 1040 | 1036 | 1000 | |
| CommandBar | mount | 10955 | 10906 | 1000 | |
| ContextualMenu | mount | 7210 | 6936 | 1000 | |
| DefaultButton | mount | 1236 | 1243 | 5000 | |
| DetailsRow | mount | 3984 | 3888 | 5000 | |
| DetailsRowFast | mount | 3890 | 4116 | 5000 | |
| DetailsRowNoStyles | mount | 3698 | 3775 | 5000 | |
| Dialog | mount | 2568 | 2503 | 1000 | |
| DocumentCardTitle | mount | 161 | 153 | 1000 | |
| Dropdown | mount | 3434 | 3388 | 5000 | |
| FluentProviderNext | mount | 7741 | 7779 | 5000 | |
| FluentProviderWithTheme | mount | 372 | 384 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 100 | 101 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 510 | 512 | 10 | |
| FocusTrapZone | mount | 1881 | 1916 | 5000 | |
| FocusZone | mount | 1896 | 1927 | 5000 | |
| IconButton | mount | 1810 | 1839 | 5000 | |
| Label | mount | 353 | 345 | 5000 | |
| Layer | mount | 3139 | 3104 | 5000 | |
| Link | mount | 472 | 481 | 5000 | |
| MakeStyles | mount | 1892 | 1875 | 50000 | |
| MenuButton | mount | 1533 | 1541 | 5000 | |
| MessageBar | mount | 2110 | 2143 | 5000 | |
| Nav | mount | 3436 | 3426 | 1000 | |
| OverflowSet | mount | 1161 | 1148 | 5000 | |
| Panel | mount | 2439 | 1430 | 1000 | Possible regression |
| Persona | mount | 865 | 868 | 1000 | |
| Pivot | mount | 1494 | 1465 | 1000 | |
| PrimaryButton | mount | 1336 | 1338 | 5000 | |
| Rating | mount | 8093 | 8089 | 5000 | |
| SearchBox | mount | 1374 | 1369 | 5000 | |
| Shimmer | mount | 2702 | 2662 | 5000 | |
| Slider | mount | 2044 | 2051 | 5000 | |
| SpinButton | mount | 5262 | 5262 | 5000 | |
| Spinner | mount | 452 | 446 | 5000 | |
| SplitButton | mount | 3323 | 3311 | 5000 | |
| Stack | mount | 522 | 532 | 5000 | |
| StackWithIntrinsicChildren | mount | 1696 | 1698 | 5000 | |
| StackWithTextChildren | mount | 4766 | 4763 | 5000 | |
| SwatchColorPicker | mount | 10941 | 10857 | 5000 | |
| Tabs | mount | 1517 | 1501 | 1000 | |
| TagPicker | mount | 2780 | 2735 | 5000 | |
| TeachingBubble | mount | 13825 | 14162 | 5000 | |
| Text | mount | 459 | 417 | 5000 | |
| TextField | mount | 1446 | 1436 | 5000 | |
| ThemeProvider | mount | 1249 | 1256 | 5000 | |
| ThemeProvider | virtual-rerender | 608 | 627 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 1999 | 1967 | 5000 | |
| Toggle | mount | 856 | 833 | 5000 | |
| buttonNative | mount | 129 | 109 | 5000 |
Perf Analysis (@fluentui/react-northstar)
Perf tests with no regressions
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| AttachmentMinimalPerf.default | 193 | 162 | 1.19:1 |
| AlertMinimalPerf.default | 320 | 278 | 1.15:1 |
| ButtonMinimalPerf.default | 195 | 178 | 1.1:1 |
| RefMinimalPerf.default | 246 | 225 | 1.09:1 |
| TreeWith60ListItems.default | 190 | 174 | 1.09:1 |
| ImageMinimalPerf.default | 393 | 369 | 1.07:1 |
| RadioGroupMinimalPerf.default | 484 | 451 | 1.07:1 |
| ChatDuplicateMessagesPerf.default | 323 | 306 | 1.06:1 |
| AttachmentSlotsPerf.default | 1156 | 1097 | 1.05:1 |
| DividerMinimalPerf.default | 387 | 367 | 1.05:1 |
| DropdownManyItemsPerf.default | 735 | 701 | 1.05:1 |
| FlexMinimalPerf.default | 311 | 297 | 1.05:1 |
| HeaderSlotsPerf.default | 804 | 773 | 1.04:1 |
| ListWith60ListItems.default | 683 | 659 | 1.04:1 |
| ProviderMinimalPerf.default | 1125 | 1081 | 1.04:1 |
| CarouselMinimalPerf.default | 498 | 484 | 1.03:1 |
| FormMinimalPerf.default | 421 | 408 | 1.03:1 |
| LabelMinimalPerf.default | 407 | 395 | 1.03:1 |
| SliderMinimalPerf.default | 1687 | 1643 | 1.03:1 |
| ButtonSlotsPerf.default | 576 | 567 | 1.02:1 |
| EmbedMinimalPerf.default | 4431 | 4357 | 1.02:1 |
| GridMinimalPerf.default | 361 | 354 | 1.02:1 |
| LayoutMinimalPerf.default | 388 | 381 | 1.02:1 |
| ListNestedPerf.default | 587 | 578 | 1.02:1 |
| LoaderMinimalPerf.default | 743 | 726 | 1.02:1 |
| PortalMinimalPerf.default | 188 | 185 | 1.02:1 |
| ReactionMinimalPerf.default | 393 | 384 | 1.02:1 |
| TextMinimalPerf.default | 353 | 346 | 1.02:1 |
| AnimationMinimalPerf.default | 431 | 428 | 1.01:1 |
| DatepickerMinimalPerf.default | 5933 | 5873 | 1.01:1 |
| HeaderMinimalPerf.default | 374 | 370 | 1.01:1 |
| ItemLayoutMinimalPerf.default | 1275 | 1263 | 1.01:1 |
| ListCommonPerf.default | 672 | 668 | 1.01:1 |
| ListMinimalPerf.default | 547 | 543 | 1.01:1 |
| MenuButtonMinimalPerf.default | 1700 | 1677 | 1.01:1 |
| ProviderMergeThemesPerf.default | 1752 | 1735 | 1.01:1 |
| SplitButtonMinimalPerf.default | 4280 | 4246 | 1.01:1 |
| CustomToolbarPrototype.default | 4094 | 4062 | 1.01:1 |
| AccordionMinimalPerf.default | 158 | 158 | 1:1 |
| ButtonOverridesMissPerf.default | 1812 | 1808 | 1:1 |
| ChatWithPopoverPerf.default | 404 | 402 | 1:1 |
| InputMinimalPerf.default | 1379 | 1377 | 1:1 |
| PopupMinimalPerf.default | 608 | 610 | 1:1 |
| SegmentMinimalPerf.default | 355 | 355 | 1:1 |
| TableMinimalPerf.default | 403 | 404 | 1:1 |
| TreeMinimalPerf.default | 811 | 810 | 1:1 |
| VideoMinimalPerf.default | 641 | 644 | 1:1 |
| BoxMinimalPerf.default | 359 | 363 | 0.99:1 |
| ChatMinimalPerf.default | 684 | 689 | 0.99:1 |
| CheckboxMinimalPerf.default | 2858 | 2888 | 0.99:1 |
| DialogMinimalPerf.default | 791 | 795 | 0.99:1 |
| DropdownMinimalPerf.default | 3296 | 3318 | 0.99:1 |
| TableManyItemsPerf.default | 1887 | 1911 | 0.99:1 |
| ToolbarMinimalPerf.default | 935 | 945 | 0.99:1 |
| TooltipMinimalPerf.default | 1031 | 1041 | 0.99:1 |
| AvatarMinimalPerf.default | 200 | 204 | 0.98:1 |
| CardMinimalPerf.default | 553 | 563 | 0.98:1 |
| MenuMinimalPerf.default | 868 | 889 | 0.98:1 |
| SkeletonMinimalPerf.default | 358 | 367 | 0.98:1 |
| IconMinimalPerf.default | 604 | 617 | 0.98:1 |
| StatusMinimalPerf.default | 671 | 690 | 0.97:1 |
| TextAreaMinimalPerf.default | 492 | 506 | 0.97:1 |
| RosterPerf.default | 1169 | 1213 | 0.96:1 |
| PopoverSurfaceProps | ||
|
|
||
| ```ts | ||
| extends ComponentPropsCompat, |
There was a problem hiding this comment.
ComponentPropsCompat has been removed from Popover since #19767
There was a problem hiding this comment.
Good to know. As expected, this document will age out quickly as beta work continues.
|
|
||
| > Some components have mutually exclusive booleans and others use discriminated unions. | ||
| > Some components define a color property that are a set of colors, others are a set semantic names. | ||
| > Some naming is inconsistent (e.g. noArrow, pointing) |
There was a problem hiding this comment.
funnily enough, noArrow was used both in Popover and Tooltip initially, I wonder what requirement resulted in the divergence @behowell ?
There was a problem hiding this comment.
The design for Tooltip was changed to not have an arrow by default, so I inverted the property naming so its default value would still be false.
I think I asked design to follow up about whether Popover's default should also change to not have an arrow, but I'm not sure if that happened.
| AvatarProps | ||
|
|
||
| ```ts | ||
| activeDisplay: 'ring' | 'shadow' | 'glow' | 'ring-shadow' | 'ring-glow'; |
There was a problem hiding this comment.
I'm wondering if there should be two props here: One for that lets you specify 'ring' and maybe future values and another for 'shadow' | 'glow'. Are these specific appearances or is this union trying to cover the permutations of two different things?
There was a problem hiding this comment.
Per chat with Levi, we should break these up if they are enumerating permutations. I'll follow up with Avatar owner to find out.
|
|
||
| **Proposed WorkItems** | ||
|
|
||
| - [ ] Update all vNext component props to use types and intersections. |
There was a problem hiding this comment.
Could you add an example of what the final result would look like in code ?
There was a problem hiding this comment.
I think the best example would be a PR :)
|
|
||
| - Avoid re-declaring a native property and redefining its type. | ||
| - Avoid naming properties that conflict with native prop names. | ||
| - Provide a suffix of 'slot' to properties that provide a slot. |
There was a problem hiding this comment.
I think that this worth a separate RFC as it that affects all components. This a huge breaking change even for beta as there are already customers that are using v9 components.
We should clearly understand motivation, pros and cons of this proposal. This affects not only us, but also customers that will develop reusable components as they should follow similar patterns.
For example, to cons I can add bundle size/memory concern that should be validated first.
JSX props are compiled to JS objects:
<Button iconSlot={<div />} />
// ⬇⬇⬇
React.createElement(Button, { iconSlot: React.createElement('div') })- properties of such objects cannot be mangled by Terser, this means that "iconSlot" will stay in minified JS
- (is it valid?) properties of objects in memory will be longer
So, Slot suffix will add additional 4 bytes to every slot's usage, at least for bundle size. 4 bytes is nothing, but in terms of huge apps with thousands of components/slots usages it's something.
For example, 1k definitions for slots will result in 4000 bytes in minified code, that gives us almost 4kB for nothing ¯_(ツ)_/¯
Yes, GZIP will compress this, but browsers will still parse uncompressed code.
Pull request checklist
$ yarn changeDescription of changes
This is a PR to allow conversation/comments on the vNext props consistency. It isn't intended to be checked in.
Focus areas to test
(optional)