RFC: standardize event handlers arguments#18974
Conversation
|
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 cf231cd:
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 57a2bc1f8a6ec7859bf4a3509696681d39602561 (build) |
📊 Bundle size reportUnchanged fixtures
|
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 845 | 843 | 5000 | |
| BaseButton | mount | 830 | 859 | 5000 | |
| Breadcrumb | mount | 2529 | 2464 | 1000 | |
| ButtonNext | mount | 393 | 401 | 5000 | |
| Checkbox | mount | 1407 | 1427 | 5000 | |
| CheckboxBase | mount | 1207 | 1200 | 5000 | |
| ChoiceGroup | mount | 4463 | 4444 | 5000 | |
| ComboBox | mount | 932 | 914 | 1000 | |
| CommandBar | mount | 9586 | 9603 | 1000 | |
| ContextualMenu | mount | 5929 | 5916 | 1000 | |
| DefaultButton | mount | 1061 | 1073 | 5000 | |
| DetailsRow | mount | 3497 | 3543 | 5000 | |
| DetailsRowFast | mount | 3566 | 3464 | 5000 | |
| DetailsRowNoStyles | mount | 3310 | 3340 | 5000 | |
| Dialog | mount | 2040 | 2057 | 1000 | |
| DocumentCardTitle | mount | 130 | 132 | 1000 | |
| Dropdown | mount | 3042 | 3029 | 5000 | |
| FluentProviderNext | mount | 7104 | 7127 | 5000 | |
| FocusTrapZone | mount | 1712 | 1716 | 5000 | |
| FocusZone | mount | 1685 | 1674 | 5000 | |
| IconButton | mount | 1581 | 1638 | 5000 | |
| Label | mount | 329 | 321 | 5000 | |
| Layer | mount | 1698 | 1658 | 5000 | |
| Link | mount | 439 | 431 | 5000 | |
| MakeStyles | mount | 1730 | 1728 | 50000 | |
| MenuButton | mount | 1385 | 1387 | 5000 | |
| MessageBar | mount | 1892 | 1898 | 5000 | |
| Nav | mount | 3085 | 3060 | 1000 | |
| OverflowSet | mount | 1005 | 983 | 5000 | |
| Panel | mount | 2023 | 1973 | 1000 | |
| Persona | mount | 789 | 787 | 1000 | |
| Pivot | mount | 1321 | 1326 | 1000 | |
| PrimaryButton | mount | 1193 | 1209 | 5000 | |
| Rating | mount | 7218 | 7200 | 5000 | |
| SearchBox | mount | 1243 | 1229 | 5000 | |
| Shimmer | mount | 2402 | 2395 | 5000 | |
| Slider | mount | 1854 | 1832 | 5000 | |
| SpinButton | mount | 4675 | 4729 | 5000 | |
| Spinner | mount | 403 | 402 | 5000 | |
| SplitButton | mount | 2949 | 2984 | 5000 | |
| Stack | mount | 481 | 468 | 5000 | |
| StackWithIntrinsicChildren | mount | 1448 | 1474 | 5000 | |
| StackWithTextChildren | mount | 4241 | 4264 | 5000 | |
| SwatchColorPicker | mount | 9686 | 9605 | 5000 | |
| Tabs | mount | 1339 | 1330 | 1000 | |
| TagPicker | mount | 2453 | 2472 | 5000 | |
| TeachingBubble | mount | 11224 | 11289 | 5000 | |
| Text | mount | 383 | 393 | 5000 | |
| TextField | mount | 1285 | 1298 | 5000 | |
| ThemeProvider | mount | 1130 | 1127 | 5000 | |
| ThemeProvider | virtual-rerender | 564 | 572 | 5000 | |
| Toggle | mount | 756 | 767 | 5000 | |
| buttonNative | mount | 104 | 114 | 5000 |
Perf Analysis (@fluentui/react-northstar)
Perf tests with no regressions
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| AccordionMinimalPerf.default | 149 | 136 | 1.1:1 |
| ChatWithPopoverPerf.default | 351 | 326 | 1.08:1 |
| BoxMinimalPerf.default | 336 | 315 | 1.07:1 |
| AttachmentMinimalPerf.default | 149 | 141 | 1.06:1 |
| HeaderMinimalPerf.default | 347 | 326 | 1.06:1 |
| AlertMinimalPerf.default | 259 | 247 | 1.05:1 |
| SkeletonMinimalPerf.default | 336 | 321 | 1.05:1 |
| TableMinimalPerf.default | 386 | 369 | 1.05:1 |
| TextMinimalPerf.default | 331 | 316 | 1.05:1 |
| AttachmentSlotsPerf.default | 1021 | 990 | 1.03:1 |
| ButtonMinimalPerf.default | 159 | 154 | 1.03:1 |
| SegmentMinimalPerf.default | 320 | 310 | 1.03:1 |
| TreeWith60ListItems.default | 160 | 155 | 1.03:1 |
| CheckboxMinimalPerf.default | 2593 | 2549 | 1.02:1 |
| FormMinimalPerf.default | 380 | 373 | 1.02:1 |
| GridMinimalPerf.default | 323 | 318 | 1.02:1 |
| ImageMinimalPerf.default | 346 | 339 | 1.02:1 |
| LabelMinimalPerf.default | 370 | 361 | 1.02:1 |
| ListNestedPerf.default | 532 | 524 | 1.02:1 |
| PopupMinimalPerf.default | 568 | 557 | 1.02:1 |
| ProviderMinimalPerf.default | 959 | 940 | 1.02:1 |
| SplitButtonMinimalPerf.default | 3621 | 3565 | 1.02:1 |
| TextAreaMinimalPerf.default | 465 | 456 | 1.02:1 |
| ToolbarMinimalPerf.default | 880 | 865 | 1.02:1 |
| AvatarMinimalPerf.default | 178 | 176 | 1.01:1 |
| CarouselMinimalPerf.default | 434 | 430 | 1.01:1 |
| ChatMinimalPerf.default | 615 | 607 | 1.01:1 |
| DropdownMinimalPerf.default | 2984 | 2952 | 1.01:1 |
| EmbedMinimalPerf.default | 3971 | 3915 | 1.01:1 |
| LoaderMinimalPerf.default | 670 | 665 | 1.01:1 |
| MenuButtonMinimalPerf.default | 1558 | 1546 | 1.01:1 |
| ReactionMinimalPerf.default | 350 | 346 | 1.01:1 |
| RefMinimalPerf.default | 222 | 219 | 1.01:1 |
| StatusMinimalPerf.default | 625 | 617 | 1.01:1 |
| IconMinimalPerf.default | 581 | 574 | 1.01:1 |
| TableManyItemsPerf.default | 1763 | 1752 | 1.01:1 |
| TooltipMinimalPerf.default | 945 | 938 | 1.01:1 |
| TreeMinimalPerf.default | 733 | 726 | 1.01:1 |
| ButtonOverridesMissPerf.default | 1604 | 1608 | 1:1 |
| DialogMinimalPerf.default | 711 | 713 | 1:1 |
| InputMinimalPerf.default | 1190 | 1195 | 1:1 |
| LayoutMinimalPerf.default | 341 | 340 | 1:1 |
| ProviderMergeThemesPerf.default | 1585 | 1589 | 1:1 |
| RadioGroupMinimalPerf.default | 413 | 411 | 1:1 |
| CustomToolbarPrototype.default | 3667 | 3662 | 1:1 |
| CardMinimalPerf.default | 518 | 523 | 0.99:1 |
| DividerMinimalPerf.default | 333 | 336 | 0.99:1 |
| FlexMinimalPerf.default | 264 | 267 | 0.99:1 |
| HeaderSlotsPerf.default | 704 | 708 | 0.99:1 |
| ItemLayoutMinimalPerf.default | 1126 | 1133 | 0.99:1 |
| ListMinimalPerf.default | 481 | 484 | 0.99:1 |
| ListWith60ListItems.default | 596 | 603 | 0.99:1 |
| AnimationMinimalPerf.default | 380 | 387 | 0.98:1 |
| DatepickerMinimalPerf.default | 4989 | 5087 | 0.98:1 |
| DropdownManyItemsPerf.default | 624 | 635 | 0.98:1 |
| MenuMinimalPerf.default | 783 | 803 | 0.98:1 |
| SliderMinimalPerf.default | 1468 | 1494 | 0.98:1 |
| ButtonSlotsPerf.default | 507 | 521 | 0.97:1 |
| ListCommonPerf.default | 558 | 578 | 0.97:1 |
| RosterPerf.default | 1092 | 1124 | 0.97:1 |
| VideoMinimalPerf.default | 575 | 592 | 0.97:1 |
| ChatDuplicateMessagesPerf.default | 271 | 282 | 0.96:1 |
| PortalMinimalPerf.default | 157 | 171 | 0.92:1 |
| ```tsx | ||
| interface ChangeData<TValue, TProps> { | ||
| value: TValue; | ||
| props: TProps; |
There was a problem hiding this comment.
I don't see why parent props are necessary here, this is user input and we do not mutate it so why should we return all of this ?
As you mention later it will also cause some 'smoke' and make the valuable properties harder to find.
The DTO is a 👍 but we should only return data that is pertinent to the change/mutation that is happening on the component for proper encapsulation. Otherwise if consumers decide to forward change handlers internally in their app then you break encapsulation of the component tree
There was a problem hiding this comment.
There are scenarios where the caller has multiple components hooked up to a single onChange callback. Without providing the original props, each instance would need to be closured with the identifiable data to handle them individually.
Also someone who looks at the event handler without reading the details might assume that props.value and value are equal.
One way to clear up some ambiguity might be to call it "originalProps" and pass along the original props. E.g. if value changes from 'a' to 'b', then value would be b, while originalProps.value would be 'a'.
There was a problem hiding this comment.
each instance would need to be closured with the identifiable data to handle them individually
Isn't that a good thing ? in that case your multiple component are encapsulated into a single component and you should only pass on relevant data to its parent.
minor fix in title of option c
ecraig12345
left a comment
ecraig12345
left a comment
There was a problem hiding this comment.
Thanks for doing this! I like the first option.
|
|
||
| #### Cons | ||
|
|
||
| 👎 `data.props.value` and `data.props.defaultValue` are accessible, leaving multiple ways to see value which might be confusing as they'll represent the current _props_ rather than the new value. But it _should be_ obvious that these are user inputs and not the new value. Could consider calling new value `newValue` to be clear, but that seems a bit unpredictable. |
There was a problem hiding this comment.
I don't think this is too big of an issue, especially once people get used to the pattern (since it will be consistent across all components and handlers)
| #### Cons | ||
|
|
||
| 👎 `data.props.value` and `data.props.defaultValue` are accessible, leaving multiple ways to see value which might be confusing as they'll represent the current _props_ rather than the new value. But it _should be_ obvious that these are user inputs and not the new value. Could consider calling new value `newValue` to be clear, but that seems a bit unpredictable. | ||
| 👎 structure is deeply nested, for example `data.props.id` |
There was a problem hiding this comment.
I suspect this won't be too much of an issue either, since accessing data.props should be relatively uncommon. And if they do need to access it multiple times, they can destructure/assign it to a local const.
| props: TProps; | ||
| } | ||
|
|
||
| const onChange = (ev: React.FormEvent, data: ChangeData<string, InputProps>) => { |
There was a problem hiding this comment.
What would be the pattern for typings if a particular event handler needed to add more data properties?
There was a problem hiding this comment.
IMO just extend ChangeData, ChangeData should not be a shared type, in reality it will look like:
// not sure about naming
type InputOnChangeData = {
props: InputProps
value: string
// ... other properties on demand
}There was a problem hiding this comment.
@ecraig12345 based on yours and @behowell feedback, I update this part, now it should be clearer, a0176c6. WDYT?
miroslavstastny
added
v9beta: days
Effort II: Days
Fluent UI react-components (v9)
and removed
v9beta: days
labels
|
|
||
| ```tsx | ||
| interface ChangeData<TValue, TProps> { | ||
| value: TValue; |
There was a problem hiding this comment.
The name value doesn't always make sense and could possibly be confusing in some cases. E.g. for Checkbox, the onChange event happens when the checked prop changes, not the value prop. It could be more natural if either:
- It's named
data.checked, or - The event is named
onCheckedChange
For option 1, this type could be:
type ChangeData<TPropName extends keyof TProps, TProps> = Pick<TProps, TPropName> & { props: TProps; }The event would be:
onChange?: (data: ChangeData<'checked', CheckboxProps>) => void;And it'd be used like:
<Checkbox onChange={data => console.log(data.checked)} />There was a problem hiding this comment.
The name
valuedoesn't always make sense and could possibly be confusing in some cases.
To be clear, there is no plan to use the same ChangeData interface for all components (#18974 (comment)), I will update this section to make clearer. Totally agree that value does not have sense for Checkbox, especially when Checkbox has checked prop.
Good call, thanks for feedback 👍
There was a problem hiding this comment.
@behowell I applied your feedback in a0176c6, is it clearer?
bsunderhus
left a comment
bsunderhus
left a comment
There was a problem hiding this comment.
I'm in favor of Option A.
I kind of don't know how the signature would be in the case of having some handler that is not associated with a native event though (which is quite rare and probably not gonna happen)
|
We had an offline conversation about this RFC with the team:
|
8 participants
All credit there goes to @dzearing, this RFC was originally published in #12588.
We should provide guidance on how events handlers (
onChange,onOpen, etc.) are exposed across components. Today we have a discrepancy between@fluentui/react-northstar(Fluent UI Northstar) ,@fluentui/react(Fluent UI v8) and converged components.🧾 Rendered preview