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
Simplify prop merging with slots #18721
Simplify prop merging with slots #18721
Conversation
Asset size changes
Over Tolerance (1024 B) Over Baseline Below Baseline New Removed 1 kB = 1000 B Baseline commit: 27cc926310490c4291a44b2e399547a95aa3dc84 (build) |
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
Avatar | mount | 927 | 917 | 5000 | |
BaseButton | mount | 1039 | 1076 | 5000 | |
Breadcrumb | mount | 2896 | 2922 | 1000 | |
ButtonNext | mount | 610 | 581 | 5000 | |
Checkbox | mount | 1707 | 1711 | 5000 | |
CheckboxBase | mount | 1516 | 1468 | 5000 | |
ChoiceGroup | mount | 5360 | 5335 | 5000 | |
ComboBox | mount | 1069 | 1112 | 1000 | |
CommandBar | mount | 11407 | 11355 | 1000 | |
ContextualMenu | mount | 7015 | 6986 | 1000 | |
DefaultButton | mount | 1303 | 1308 | 5000 | |
DetailsRow | mount | 4184 | 4153 | 5000 | |
DetailsRowFast | mount | 4286 | 4319 | 5000 | |
DetailsRowNoStyles | mount | 3904 | 4018 | 5000 | |
Dialog | mount | 2386 | 2293 | 1000 | |
DocumentCardTitle | mount | 172 | 158 | 1000 | |
Dropdown | mount | 3673 | 3714 | 5000 | |
FluentProviderNext | mount | 7466 | 7502 | 5000 | |
FocusTrapZone | mount | 1971 | 1974 | 5000 | |
FocusZone | mount | 1921 | 2001 | 5000 | |
IconButton | mount | 1983 | 2034 | 5000 | |
Label | mount | 378 | 375 | 5000 | |
Layer | mount | 2077 | 2099 | 5000 | |
Link | mount | 532 | 529 | 5000 | |
MakeStyles | mount | 2013 | 2026 | 50000 | |
MenuButton | mount | 1736 | 1668 | 5000 | |
MessageBar | mount | 2198 | 2232 | 5000 | |
Nav | mount | 3680 | 3781 | 1000 | |
OverflowSet | mount | 1159 | 1174 | 5000 | |
Panel | mount | 2271 | 2267 | 1000 | |
Persona | mount | 910 | 913 | 1000 | |
Pivot | mount | 1582 | 1646 | 1000 | |
PrimaryButton | mount | 1486 | 1438 | 5000 | |
Rating | mount | 8972 | 8933 | 5000 | |
SearchBox | mount | 1497 | 1533 | 5000 | |
Shimmer | mount | 2879 | 2901 | 5000 | |
Slider | mount | 2193 | 2254 | 5000 | |
SpinButton | mount | 5644 | 5633 | 5000 | |
Spinner | mount | 458 | 473 | 5000 | |
SplitButton | mount | 3548 | 3633 | 5000 | |
Stack | mount | 563 | 566 | 5000 | |
StackWithIntrinsicChildren | mount | 1819 | 1778 | 5000 | |
StackWithTextChildren | mount | 5221 | 5282 | 5000 | |
SwatchColorPicker | mount | 11555 | 11546 | 5000 | |
Tabs | mount | 1600 | 1574 | 1000 | |
TagPicker | mount | 2813 | 2776 | 5000 | |
TeachingBubble | mount | 13157 | 13086 | 5000 | |
Text | mount | 494 | 471 | 5000 | |
TextField | mount | 1591 | 1545 | 5000 | |
ThemeProvider | mount | 1327 | 1326 | 5000 | |
ThemeProvider | virtual-rerender | 672 | 672 | 5000 | |
Toggle | mount | 912 | 927 | 5000 | |
buttonNative | mount | 133 | 123 | 5000 |
Perf Analysis (@fluentui/react-northstar
)
Potential regressions comparing to master
Scenario | Current PR Ticks | Baseline Ticks | Ratio | Regression Analysis |
---|---|---|---|---|
PortalMinimalPerf.default | 199 | 180 | 1.11:1 | analysis |
Perf tests with no regressions
Scenario | Current PR Ticks | Baseline Ticks | Ratio |
---|---|---|---|
AttachmentMinimalPerf.default | 196 | 177 | 1.11:1 |
FormMinimalPerf.default | 525 | 476 | 1.1:1 |
LayoutMinimalPerf.default | 451 | 412 | 1.09:1 |
AnimationMinimalPerf.default | 503 | 464 | 1.08:1 |
DropdownManyItemsPerf.default | 837 | 777 | 1.08:1 |
RefMinimalPerf.default | 270 | 250 | 1.08:1 |
ImageMinimalPerf.default | 474 | 443 | 1.07:1 |
SkeletonMinimalPerf.default | 446 | 415 | 1.07:1 |
TreeWith60ListItems.default | 207 | 194 | 1.07:1 |
ButtonSlotsPerf.default | 655 | 616 | 1.06:1 |
LabelMinimalPerf.default | 466 | 441 | 1.06:1 |
BoxMinimalPerf.default | 424 | 405 | 1.05:1 |
RadioGroupMinimalPerf.default | 549 | 522 | 1.05:1 |
ChatMinimalPerf.default | 792 | 759 | 1.04:1 |
ChatWithPopoverPerf.default | 426 | 411 | 1.04:1 |
SliderMinimalPerf.default | 1797 | 1724 | 1.04:1 |
StatusMinimalPerf.default | 824 | 791 | 1.04:1 |
IconMinimalPerf.default | 739 | 713 | 1.04:1 |
CarouselMinimalPerf.default | 550 | 536 | 1.03:1 |
HeaderSlotsPerf.default | 919 | 890 | 1.03:1 |
ItemLayoutMinimalPerf.default | 1456 | 1408 | 1.03:1 |
ListMinimalPerf.default | 607 | 591 | 1.03:1 |
MenuButtonMinimalPerf.default | 1863 | 1811 | 1.03:1 |
SegmentMinimalPerf.default | 418 | 407 | 1.03:1 |
ChatDuplicateMessagesPerf.default | 344 | 336 | 1.02:1 |
FlexMinimalPerf.default | 330 | 324 | 1.02:1 |
ListNestedPerf.default | 662 | 652 | 1.02:1 |
PopupMinimalPerf.default | 636 | 626 | 1.02:1 |
SplitButtonMinimalPerf.default | 4309 | 4244 | 1.02:1 |
TextAreaMinimalPerf.default | 605 | 594 | 1.02:1 |
AlertMinimalPerf.default | 329 | 326 | 1.01:1 |
AvatarMinimalPerf.default | 234 | 232 | 1.01:1 |
ButtonMinimalPerf.default | 206 | 203 | 1.01:1 |
CardMinimalPerf.default | 655 | 651 | 1.01:1 |
DialogMinimalPerf.default | 881 | 875 | 1.01:1 |
DividerMinimalPerf.default | 421 | 416 | 1.01:1 |
DropdownMinimalPerf.default | 3412 | 3378 | 1.01:1 |
InputMinimalPerf.default | 1430 | 1412 | 1.01:1 |
ListWith60ListItems.default | 745 | 740 | 1.01:1 |
MenuMinimalPerf.default | 995 | 985 | 1.01:1 |
RosterPerf.default | 1332 | 1313 | 1.01:1 |
ProviderMinimalPerf.default | 1137 | 1131 | 1.01:1 |
TableManyItemsPerf.default | 2263 | 2248 | 1.01:1 |
TextMinimalPerf.default | 420 | 415 | 1.01:1 |
CustomToolbarPrototype.default | 4335 | 4299 | 1.01:1 |
TooltipMinimalPerf.default | 1151 | 1136 | 1.01:1 |
AttachmentSlotsPerf.default | 1210 | 1209 | 1:1 |
CheckboxMinimalPerf.default | 3102 | 3089 | 1:1 |
EmbedMinimalPerf.default | 4679 | 4695 | 1:1 |
LoaderMinimalPerf.default | 802 | 803 | 1:1 |
TreeMinimalPerf.default | 937 | 940 | 1:1 |
VideoMinimalPerf.default | 764 | 765 | 1:1 |
ButtonOverridesMissPerf.default | 1920 | 1944 | 0.99:1 |
DatepickerMinimalPerf.default | 6256 | 6296 | 0.99:1 |
GridMinimalPerf.default | 398 | 401 | 0.99:1 |
ListCommonPerf.default | 744 | 749 | 0.99:1 |
ProviderMergeThemesPerf.default | 1814 | 1835 | 0.99:1 |
ReactionMinimalPerf.default | 450 | 454 | 0.99:1 |
ToolbarMinimalPerf.default | 1115 | 1127 | 0.99:1 |
HeaderMinimalPerf.default | 419 | 426 | 0.98:1 |
TableMinimalPerf.default | 474 | 487 | 0.97:1 |
AccordionMinimalPerf.default | 174 | 191 | 0.91:1 |
f0646fb
to
c2088cb
Compare
b0a070f
to
f830176
Compare
3fdb70c
to
5202cb6
Compare
7e3a28b
to
ed72839
Compare
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 4d0eac3:
|
@bsunderhus Could you add an example of what using the new APIs looks like in one component? |
WDYM by adding an example @ecraig12345 ?! I've implemented this functionality for both |
Co-authored-by: ling1726 <lingfangao@hotmail.com>
Co-authored-by: ling1726 <lingfangao@hotmail.com>
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
🎉 Handy links: |
packages/react-accordion/src/components/Accordion/Accordion.types.ts
Outdated
Show resolved
Hide resolved
const { as, children } = typedState[name]; | ||
|
||
const slot = getSlot(typedState.components ? typedState.components[name] : undefined, as) as Slots[typeof name]; |
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.
According to the RFC (https://github.com/microsoft/fluentui/blob/master/rfcs/convergence/simplify-prop-merging.md#do-not-support-as-prop-for-slots-that-render-native-dom-elements) this part is redundant. It should be just:
const slot = typedState.components[name] || 'div'
slotProps[name] = | ||
typeof slots[name] === 'string' | ||
? (omit(typedState[name], ['as']) as ComponentState<SlotProps>[keyof SlotProps]) | ||
: typedState[name]; |
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.
We also don't need to filter out as
prop as it should not be a thing for native elements.
| ObjectShorthandProps<Props>; | ||
|
||
export type ObjectShorthandProps<Props extends { children?: React.ReactNode } = {}> = Props & | ||
Pick<ComponentProps, 'as'> & { |
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.
as
should be added only if the component itself supports it, we can remove Pick<>
from there
Pull request checklist
$ yarn change
Description of changes
Implements changes proposed by the RFC #18642
To sum up the RFC:
This RFC proposes a set of changes in three major steps:
mergeProps
and usage of theas
prop, so that JSX elements in slots can be configured using shorthandas
prop.mergeProps
in favour of object spreading over deep merge.PREVIEW 📃
Problems so far in this implementation
JSX.IntrinsicElements
is harmful. Need to find a way to force developer to pass down the way Typings instead of letting Typescript compiler infer things by itselfroot
.root
is completely alien to the new system of typings, we should consider using an approach of declaring it the same way all the other slots are declared in state.CompoundButton
andAccordionHeader
break the internal logic ofgetSlots
to decide if a slot should or should not be rendered (this is not a problem of this implementation! this is legacy).To addres incomplete problems new RFC's might be proposed since they'll create breaking changes