Add ForwardRefComponent helper type #19923
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Can you maybe paste here what are the mismatches found when applying it to |
|
For Link the first conflict it shows is with For Text: |
|
All the |
📊 Bundle size reportUnchanged fixtures
|
Asset size changesSize Auditor did not detect a change in bundle size for any component! Baseline commit: 8952fa63a35532efafbcb5fd7290875fe838caeb (build) |
|
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 49289b5:
|
Perf Analysis (
|
| Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
|---|---|---|---|---|---|
| Avatar | mount | 778 | 786 | 5000 | |
| BaseButton | mount | 789 | 777 | 5000 | |
| Breadcrumb | mount | 2270 | 2252 | 1000 | |
| ButtonNext | mount | 385 | 392 | 5000 | |
| Checkbox | mount | 1287 | 1277 | 5000 | |
| CheckboxBase | mount | 1095 | 1102 | 5000 | |
| ChoiceGroup | mount | 4016 | 4046 | 5000 | |
| ComboBox | mount | 828 | 822 | 1000 | |
| CommandBar | mount | 9179 | 8829 | 1000 | |
| ContextualMenu | mount | 5608 | 5524 | 1000 | |
| DefaultButton | mount | 944 | 963 | 5000 | |
| DetailsRow | mount | 3154 | 3267 | 5000 | |
| DetailsRowFast | mount | 6199 | 3214 | 5000 | |
| DetailsRowNoStyles | mount | 3019 | 2993 | 5000 | |
| Dialog | mount | 2111 | 2076 | 1000 | |
| DocumentCardTitle | mount | 136 | 136 | 1000 | |
| Dropdown | mount | 2725 | 2752 | 5000 | |
| FluentProviderNext | mount | 6278 | 6806 | 5000 | |
| FluentProviderWithTheme | mount | 303 | 298 | 10 | |
| FluentProviderWithTheme | virtual-rerender | 81 | 79 | 10 | |
| FluentProviderWithTheme | virtual-rerender-with-unmount | 350 | 343 | 10 | |
| FocusTrapZone | mount | 1555 | 1536 | 5000 | |
| FocusZone | mount | 1548 | 1558 | 5000 | |
| IconButton | mount | 1487 | 1481 | 5000 | |
| Label | mount | 293 | 290 | 5000 | |
| Layer | mount | 2547 | 2517 | 5000 | |
| Link | mount | 396 | 400 | 5000 | |
| MakeStyles | mount | 1572 | 1569 | 50000 | |
| MenuButton | mount | 1259 | 1246 | 5000 | |
| MessageBar | mount | 1729 | 1749 | 5000 | |
| Nav | mount | 2831 | 2815 | 1000 | |
| OverflowSet | mount | 946 | 948 | 5000 | |
| Panel | mount | 2023 | 1977 | 1000 | |
| Persona | mount | 715 | 712 | 1000 | |
| Pivot | mount | 1193 | 1227 | 1000 | |
| PrimaryButton | mount | 1097 | 1078 | 5000 | |
| Rating | mount | 6582 | 6588 | 5000 | |
| SearchBox | mount | 1132 | 1136 | 5000 | |
| Shimmer | mount | 2167 | 2139 | 5000 | |
| Slider | mount | 1676 | 1658 | 5000 | |
| SpinButton | mount | 4296 | 4322 | 5000 | |
| Spinner | mount | 360 | 347 | 5000 | |
| SplitButton | mount | 2727 | 2711 | 5000 | |
| Stack | mount | 417 | 423 | 5000 | |
| StackWithIntrinsicChildren | mount | 1484 | 1474 | 5000 | |
| StackWithTextChildren | mount | 3966 | 3956 | 5000 | |
| SwatchColorPicker | mount | 9009 | 8907 | 5000 | |
| Tabs | mount | 1218 | 1208 | 1000 | |
| TagPicker | mount | 2274 | 2267 | 5000 | |
| TeachingBubble | mount | 11014 | 11101 | 5000 | |
| Text | mount | 354 | 357 | 5000 | |
| TextField | mount | 1200 | 1172 | 5000 | |
| ThemeProvider | mount | 1007 | 998 | 5000 | |
| ThemeProvider | virtual-rerender | 517 | 512 | 5000 | |
| ThemeProvider | virtual-rerender-with-unmount | 1600 | 1620 | 5000 | |
| Toggle | mount | 680 | 687 | 5000 | |
| buttonNative | mount | 98 | 92 | 5000 |
Perf Analysis (@fluentui/react-northstar)
Potential regressions comparing to master
| Scenario | Current PR Ticks | Baseline Ticks | Ratio | Regression Analysis |
|---|---|---|---|---|
| AlertMinimalPerf.default | 232 | 242 | 0.96:1 | analysis |
Perf tests with no regressions
| Scenario | Current PR Ticks | Baseline Ticks | Ratio |
|---|---|---|---|
| FlexMinimalPerf.default | 257 | 239 | 1.08:1 |
| AvatarMinimalPerf.default | 177 | 166 | 1.07:1 |
| PortalMinimalPerf.default | 158 | 149 | 1.06:1 |
| ListWith60ListItems.default | 567 | 542 | 1.05:1 |
| AttachmentSlotsPerf.default | 939 | 901 | 1.04:1 |
| ButtonSlotsPerf.default | 484 | 467 | 1.04:1 |
| ChatWithPopoverPerf.default | 338 | 324 | 1.04:1 |
| HeaderSlotsPerf.default | 648 | 625 | 1.04:1 |
| ReactionMinimalPerf.default | 324 | 312 | 1.04:1 |
| RefMinimalPerf.default | 206 | 198 | 1.04:1 |
| SegmentMinimalPerf.default | 301 | 290 | 1.04:1 |
| ButtonMinimalPerf.default | 152 | 147 | 1.03:1 |
| CardMinimalPerf.default | 476 | 462 | 1.03:1 |
| StatusMinimalPerf.default | 582 | 567 | 1.03:1 |
| IconMinimalPerf.default | 530 | 517 | 1.03:1 |
| TextAreaMinimalPerf.default | 435 | 424 | 1.03:1 |
| BoxMinimalPerf.default | 305 | 298 | 1.02:1 |
| ChatDuplicateMessagesPerf.default | 264 | 260 | 1.02:1 |
| CheckboxMinimalPerf.default | 2365 | 2328 | 1.02:1 |
| EmbedMinimalPerf.default | 3694 | 3622 | 1.02:1 |
| ImageMinimalPerf.default | 316 | 311 | 1.02:1 |
| MenuMinimalPerf.default | 730 | 715 | 1.02:1 |
| PopupMinimalPerf.default | 509 | 500 | 1.02:1 |
| RadioGroupMinimalPerf.default | 381 | 373 | 1.02:1 |
| TableMinimalPerf.default | 352 | 345 | 1.02:1 |
| VideoMinimalPerf.default | 545 | 534 | 1.02:1 |
| ButtonOverridesMissPerf.default | 1531 | 1510 | 1.01:1 |
| CarouselMinimalPerf.default | 394 | 389 | 1.01:1 |
| ChatMinimalPerf.default | 546 | 540 | 1.01:1 |
| DialogMinimalPerf.default | 640 | 633 | 1.01:1 |
| ItemLayoutMinimalPerf.default | 1023 | 1013 | 1.01:1 |
| LabelMinimalPerf.default | 325 | 322 | 1.01:1 |
| MenuButtonMinimalPerf.default | 1403 | 1395 | 1.01:1 |
| RosterPerf.default | 1013 | 1005 | 1.01:1 |
| SplitButtonMinimalPerf.default | 3649 | 3614 | 1.01:1 |
| TextMinimalPerf.default | 300 | 296 | 1.01:1 |
| ToolbarMinimalPerf.default | 805 | 797 | 1.01:1 |
| TreeMinimalPerf.default | 687 | 680 | 1.01:1 |
| TreeWith60ListItems.default | 154 | 152 | 1.01:1 |
| DropdownMinimalPerf.default | 2726 | 2727 | 1:1 |
| FormMinimalPerf.default | 349 | 348 | 1:1 |
| HeaderMinimalPerf.default | 308 | 309 | 1:1 |
| InputMinimalPerf.default | 1122 | 1117 | 1:1 |
| LoaderMinimalPerf.default | 597 | 598 | 1:1 |
| ProviderMergeThemesPerf.default | 1483 | 1484 | 1:1 |
| ProviderMinimalPerf.default | 981 | 985 | 1:1 |
| SliderMinimalPerf.default | 1461 | 1464 | 1:1 |
| CustomToolbarPrototype.default | 3595 | 3613 | 1:1 |
| AnimationMinimalPerf.default | 351 | 353 | 0.99:1 |
| DividerMinimalPerf.default | 304 | 306 | 0.99:1 |
| DropdownManyItemsPerf.default | 570 | 576 | 0.99:1 |
| ListMinimalPerf.default | 434 | 437 | 0.99:1 |
| SkeletonMinimalPerf.default | 301 | 304 | 0.99:1 |
| TableManyItemsPerf.default | 1593 | 1608 | 0.99:1 |
| TooltipMinimalPerf.default | 877 | 885 | 0.99:1 |
| GridMinimalPerf.default | 286 | 291 | 0.98:1 |
| ListCommonPerf.default | 542 | 551 | 0.98:1 |
| AccordionMinimalPerf.default | 133 | 137 | 0.97:1 |
| DatepickerMinimalPerf.default | 4730 | 4860 | 0.97:1 |
| ListNestedPerf.default | 476 | 491 | 0.97:1 |
| LayoutMinimalPerf.default | 306 | 318 | 0.96:1 |
| AttachmentMinimalPerf.default | 134 | 143 | 0.94:1 |
khmakoto
approved these changes
Sep 23, 2021
| @@ -2,9 +2,10 @@ import * as React from 'react'; | |||
| import { render } from '@testing-library/react'; | |||
| import { Text } from './Text'; | |||
| import { isConformant } from '../../common/isConformant'; | |||
| import { TextProps } from './Text.types'; | |||
There was a problem hiding this comment.
This should probably be:
Suggested change
| import { TextProps } from './Text.types'; | |
| import type { TextProps } from './Text.types'; |
| */ | ||
| export type ForwardRefComponent<Props> = SomeEventName extends keyof Props | ||
| ? Required<Props>[SomeEventName] extends React.PointerEventHandler<infer Element> | ||
| ? React.ForwardRefExoticComponent<Props & React.RefAttributes<Element>> |
There was a problem hiding this comment.
I believe Props should be updated like this unless we can always ensure there won't be any ref passed in.
Suggested change
| ? React.ForwardRefExoticComponent<Props & React.RefAttributes<Element>> | |
| ? React.ForwardRefExoticComponent<React.PropsWithoutRef<Props> & React.RefAttributes<Element>> |
There was a problem hiding this comment.
I was trying to avoid PropsWithoutRef because IIRC the odd way it's defined (either React.PropsWithoutRef or the variant in this file) increased the likelihood of type explosion. If components using this type follow our current patterns, a ref shouldn't be included, though of course there's no guarantee of that.
| /** | ||
| * Return type for `React.forwardRef`, including inference of the proper typing for the ref. | ||
| */ | ||
| export type ForwardRefComponent<Props> = SomeEventName extends keyof Props |
There was a problem hiding this comment.
Could we possibly define the type like this? ForwardRefComponent<Props = {}, RefElement extends Element = Element>. (this is just an example, I am sure the RefElement type would have to be better specified to make React happy)
This would allow us to infer the RefElement type if none was passed in as we are doing currently, but also allow easier type casting for polymorphic components.
Example might look like this:
type Props = {
as?: "input" | "textarea" // very simplified example
value: string
}
const InputOrTextArea: ForwardRefComponent<Props, HTMLInputElement | HTMLTextAreaElement> = React.forwardRef(...)This can then be better inferred later on by the end user.
There was a problem hiding this comment.
Maybe RefElement extends Element = unknown and then go to full inference if RefElement extends unknown? Though I'm still not 100% sure that will fix the type mismatch.
|
|
||
| /** | ||
| * Input component | ||
| */ | ||
| export const Input = React.forwardRef<HTMLSpanElement, InputProps>((props, ref) => { |
There was a problem hiding this comment.
Is the HTMLSpanElement no longer needed because InputSlots.root is a span slot? Or does InputProps need to intersect HTMLSpanElement?
| import type { IntrinsicShorthandProps } from '@fluentui/react-utilities'; | ||
| import * as React_2 from 'react'; | ||
|
|
||
| // @public | ||
| export const Input: React_2.ForwardRefExoticComponent<Pick<InputProps, "input" | "slot" | "style" | "title" | "key" | "defaultChecked" | "defaultValue" | "suppressContentEditableWarning" | "suppressHydrationWarning" | "accessKey" | "className" | "contentEditable" | "contextMenu" | "dir" | "draggable" | "hidden" | "id" | "lang" | "placeholder" | "spellCheck" | "tabIndex" | "translate" | "radioGroup" | "role" | "about" | "datatype" | "inlist" | "prefix" | "property" | "resource" | "typeof" | "vocab" | "autoCapitalize" | "autoCorrect" | "autoSave" | "color" | "itemProp" | "itemScope" | "itemType" | "itemID" | "itemRef" | "results" | "security" | "unselectable" | "inputMode" | "is" | "aria-activedescendant" | "aria-atomic" | "aria-autocomplete" | "aria-busy" | "aria-checked" | "aria-colcount" | "aria-colindex" | "aria-colspan" | "aria-controls" | "aria-current" | "aria-describedby" | "aria-details" | "aria-disabled" | "aria-dropeffect" | "aria-errormessage" | "aria-expanded" | "aria-flowto" | "aria-grabbed" | "aria-haspopup" | "aria-hidden" | "aria-invalid" | "aria-keyshortcuts" | "aria-label" | "aria-labelledby" | "aria-level" | "aria-live" | "aria-modal" | "aria-multiline" | "aria-multiselectable" | "aria-orientation" | "aria-owns" | "aria-placeholder" | "aria-posinset" | "aria-pressed" | "aria-readonly" | "aria-relevant" | "aria-required" | "aria-roledescription" | "aria-rowcount" | "aria-rowindex" | "aria-rowspan" | "aria-selected" | "aria-setsize" | "aria-sort" | "aria-valuemax" | "aria-valuemin" | "aria-valuenow" | "aria-valuetext" | "dangerouslySetInnerHTML" | "onCopy" | "onCopyCapture" | "onCut" | "onCutCapture" | "onPaste" | "onPasteCapture" | "onCompositionEnd" | "onCompositionEndCapture" | "onCompositionStart" | "onCompositionStartCapture" | "onCompositionUpdate" | "onCompositionUpdateCapture" | "onFocus" | "onFocusCapture" | "onBlur" | "onBlurCapture" | "onChange" | "onChangeCapture" | "onBeforeInput" | "onBeforeInputCapture" | "onInput" | "onInputCapture" | "onReset" | "onResetCapture" | "onSubmit" | "onSubmitCapture" | "onInvalid" | "onInvalidCapture" | "onLoad" | "onLoadCapture" | "onError" | "onErrorCapture" | "onKeyDown" | "onKeyDownCapture" | "onKeyPress" | "onKeyPressCapture" | "onKeyUp" | "onKeyUpCapture" | "onAbort" | "onAbortCapture" | "onCanPlay" | "onCanPlayCapture" | "onCanPlayThrough" | "onCanPlayThroughCapture" | "onDurationChange" | "onDurationChangeCapture" | "onEmptied" | "onEmptiedCapture" | "onEncrypted" | "onEncryptedCapture" | "onEnded" | "onEndedCapture" | "onLoadedData" | "onLoadedDataCapture" | "onLoadedMetadata" | "onLoadedMetadataCapture" | "onLoadStart" | "onLoadStartCapture" | "onPause" | "onPauseCapture" | "onPlay" | "onPlayCapture" | "onPlaying" | "onPlayingCapture" | "onProgress" | "onProgressCapture" | "onRateChange" | "onRateChangeCapture" | "onSeeked" | "onSeekedCapture" | "onSeeking" | "onSeekingCapture" | "onStalled" | "onStalledCapture" | "onSuspend" | "onSuspendCapture" | "onTimeUpdate" | "onTimeUpdateCapture" | "onVolumeChange" | "onVolumeChangeCapture" | "onWaiting" | "onWaitingCapture" | "onAuxClick" | "onAuxClickCapture" | "onClick" | "onClickCapture" | "onContextMenu" | "onContextMenuCapture" | "onDoubleClick" | "onDoubleClickCapture" | "onDrag" | "onDragCapture" | "onDragEnd" | "onDragEndCapture" | "onDragEnter" | "onDragEnterCapture" | "onDragExit" | "onDragExitCapture" | "onDragLeave" | "onDragLeaveCapture" | "onDragOver" | "onDragOverCapture" | "onDragStart" | "onDragStartCapture" | "onDrop" | "onDropCapture" | "onMouseDown" | "onMouseDownCapture" | "onMouseEnter" | "onMouseLeave" | "onMouseMove" | "onMouseMoveCapture" | "onMouseOut" | "onMouseOutCapture" | "onMouseOver" | "onMouseOverCapture" | "onMouseUp" | "onMouseUpCapture" | "onSelect" | "onSelectCapture" | "onTouchCancel" | "onTouchCancelCapture" | "onTouchEnd" | "onTouchEndCapture" | "onTouchMove" | "onTouchMoveCapture" | "onTouchStart" | "onTouchStartCapture" | "onPointerDown" | "onPointerDownCapture" | "onPointerMove" | "onPointerMoveCapture" | "onPointerUp" | "onPointerUpCapture" | "onPointerCancel" | "onPointerCancelCapture" | "onPointerEnter" | "onPointerEnterCapture" | "onPointerLeave" | "onPointerLeaveCapture" | "onPointerOver" | "onPointerOverCapture" | "onPointerOut" | "onPointerOutCapture" | "onGotPointerCapture" | "onGotPointerCaptureCapture" | "onLostPointerCapture" | "onLostPointerCaptureCapture" | "onScroll" | "onScrollCapture" | "onWheel" | "onWheelCapture" | "onAnimationStart" | "onAnimationStartCapture" | "onAnimationEnd" | "onAnimationEndCapture" | "onAnimationIteration" | "onAnimationIterationCapture" | "onTransitionEnd" | "onTransitionEndCapture" | "css" | "size" | "inputWrapper" | "bookendBefore" | "bookendAfter" | "insideStart" | "insideEnd" | "as" | "appearance" | "inline"> & React_2.RefAttributes<HTMLSpanElement>>; | ||
| export const Input: ForwardRefComponent<InputProps>; | ||
|
|
||
| // @public (undocumented) | ||
| export interface InputCommons { |
There was a problem hiding this comment.
The Input interfaces should be types. Maybe not part of this PR though.
| const state = useLink(props, ref); | ||
|
|
||
| useLinkStyles(state); | ||
|
|
||
| return renderLink(state); | ||
| }); | ||
| // Work around some small mismatches in inferred types which don't matter in practice | ||
| }) as ForwardRefComponent<LinkProps>; |
There was a problem hiding this comment.
In the past I've had some bad experiences with the type inference for ISelection with DetailsList where new Selection has to be case to ISelection and somehow the constructor required params didn't match. When I see a type declared and an cast at the end, it causes a bit of worry. I think more detail about which inferred types are mismatching would help here.
There was a problem hiding this comment.
It's in #19923 (comment). The issue here is more with the forwardRef return type, not our custom type.
There was a problem hiding this comment.
(Side note: see #13411 for issues with Selection constructor types...multiple people tried to fix it and couldn't come up with anything that fully worked 😢)
There was a problem hiding this comment.
Will this cast be required for all components with polymorphic as in the future ?
It seems that this might be a source of technical debt down the line
There was a problem hiding this comment.
Currently yes, because I haven't been able to figure out a way to make it work with the way forwardRef manipulates the returned props type. You or others are welcome to try too if you want.
There was a problem hiding this comment.
That's a valid point, I wasn't thinking about another use case besides forwardRef as it's the way we're implementing internally, but it's valid that this might not be true for users
There was a problem hiding this comment.
Seems like this casting is a common problem when dealing with union types on Props across libraries that try to follow this similar pattern
I guess there's no good solution besides accepting for now that a casting is required.
There was a problem hiding this comment.
@ecraig12345 Turns out that this is a problem already solved in the internals of @types/react, they updated the implementation of PropsWithoutRef which was breaking union types when forwardRef was invoked, all we need to do is upgrade our @types/react (and probably @types/react-dom) and this will be solved
There was a problem hiding this comment.
@bsunderhus Is the fix you're looking at present in @types/react 16, or only 17?
In latest 16.x I'm seeing this:
type PropsWithoutRef<P> =
// Just Pick would be sufficient for this, but I'm trying to avoid unnecessary mapping over union types
// https://github.com/Microsoft/TypeScript/issues/28339
'ref' extends keyof P
? Pick<P, Exclude<keyof P, 'ref'>>
: P;vs. this in latest 17.x:
// Pick would not be sufficient for this. We'd like to avoid unnecessary mapping and need a distributive conditional to support unions.
// see: https://www.typescriptlang.org/docs/handbook/2/conditional-types.html#distributive-conditional-types
// https://github.com/Microsoft/TypeScript/issues/28339
P extends any ? ('ref' extends keyof P ? Pick<P, Exclude<keyof P, 'ref'>> : P) : P;There was a problem hiding this comment.
Meant to add: if it's only in 17, we unfortunately can't take that because we haven't upgraded to 17 yet. Though we could potentially try submitting a similar fix for 16 to DefinitelyTyped.
| @@ -86,6 +86,11 @@ export const divProperties: Record<string, number>; | |||
| // @public | |||
| export const formProperties: Record<string, number>; | |||
|
|
|||
| // Warning: (ae-forgotten-export) The symbol "SomeEventName" needs to be exported by the entry point index.d.ts | |||
There was a problem hiding this comment.
Yes, I intentionally didn't export it since we really really don't want anybody ever using that type directly (it could be exported with an @internal annotation and a big warning if the missing type creates problems, but I don't believe it should in our supported typescript versions)
| ? React.ForwardRefExoticComponent<Props & React.RefAttributes<Element>> | ||
| : never | ||
| : never; | ||
| // A definition like this would also work, but typescript is more likely to unnecessarily expand |
There was a problem hiding this comment.
This was actually intentional (at least for the PR, possibly to leave it there) as an example of something else that someone might be inclined to try changing it to in the future and why it may not work as well.
bsunderhus
approved these changes
Sep 30, 2021
1 task
andrefcdias
approved these changes
Oct 1, 2021
This was referenced Oct 4, 2021
1 task
mlp73
pushed a commit
to mlp73/fluentui
that referenced
this pull request
Jan 17, 2022
* Add ForwardRefComponent helper type * Change files * update hack event name
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull request checklist
$ yarn changeDescription of changes
This PR has an alternate approach of improving typings for
forwardRefcomponents (compared to #19812): define aForwardRefComponenttype and explicitly declare it as the type of each component.This works well with the majority of components which don't allow overriding
as(see changes to Input). For Link and Text which can be rendered as multiple element types, there's a slight mismatch with the type returned byforwardRefcurrently requiring a cast. This isn't ideal and I'd definitely appreciate ideas for better handling, but it doesn't seem to make a difference in the overall type checking in practice.