-
Notifications
You must be signed in to change notification settings - Fork 2.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ecraig12345 based on yours and @behowell feedback, I update this part, now it should be clearer, a0176c6. WDYT?
|
||
```tsx | ||
interface ChangeData<TValue, TProps> { | ||
value: TValue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name
value
doesn'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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@behowell I applied your feedback in a0176c6, is it clearer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
|
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