-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
chore(react-utilities): slot API react v17/18 support #31548
Draft
bsunderhus
wants to merge
9
commits into
microsoft:master
Choose a base branch
from
bsunderhus:react-utilities/chore--slot-API-react-v17/18-support
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
chore(react-utilities): slot API react v17/18 support #31548
bsunderhus
wants to merge
9
commits into
microsoft:master
from
bsunderhus:react-utilities/chore--slot-API-react-v17/18-support
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
Perf Analysis (
|
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
FluentProviderWithTheme | virtual-rerender | 32 | 38 | 10 | Possible regression |
All results
Scenario | Render type | Master Ticks | PR Ticks | Iterations | Status |
---|---|---|---|---|---|
Avatar | mount | 630 | 622 | 5000 | |
Button | mount | 303 | 309 | 5000 | |
Field | mount | 1158 | 1128 | 5000 | |
FluentProvider | mount | 683 | 704 | 5000 | |
FluentProviderWithTheme | mount | 84 | 85 | 10 | |
FluentProviderWithTheme | virtual-rerender | 32 | 38 | 10 | Possible regression |
FluentProviderWithTheme | virtual-rerender-with-unmount | 81 | 77 | 10 | |
MakeStyles | mount | 865 | 871 | 50000 | |
Persona | mount | 1786 | 1737 | 5000 | |
SpinButton | mount | 1369 | 1421 | 5000 | |
SwatchPicker | mount | 1663 | 1640 | 5000 |
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. |
📊 Bundle size reportUnchanged fixtures
|
fabricteam
reviewed
Jun 3, 2024
3421071
to
9a444bd
Compare
3279340
to
a54bb34
Compare
c906ba8
to
fcc5a3b
Compare
0e6f5ba
to
b7ee301
Compare
a558e3a
to
d0da8f9
Compare
d0da8f9
to
60621f6
Compare
b37c3df
to
ce2cfbb
Compare
ce2cfbb
to
4b3bba1
Compare
1 task
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.
Previous Behavior
Slots API breaks on v18! 💣
ReactNode
The
ReactNode
type became more specific (there was a type leak onReactFragment
type where{}
was a valid type, and{}
in typescript allows any of the primitive types butnull
orundefined
more info about this, here's a playground), and due to that specificity our declaration of what a slot is starts breaking on@types/react@18
Here's a playground showcasing the error
RefAttributes
React.RefAttributes
is theref
property. In react v17 that would be equivalent to:Now in react v18 it became:
LegacyRef
includes the support to usestring
as a valid reference value. This will break into our API 💣💣New Behavior
SlotPropsDataType
in favor ofUnknownSlotProps
https://github.com/microsoft/fluentui/pull/31548/files#diff-06ee7aed2b4bc7589d4c744c7c6649fbee2ff0129915f344768e361a6cfb4dcdR5-R10UnknownSlotProps
- its type is not equivalent to the actual signature of a slot. We do not have any restriction thatchildren
should beReactNode | undefined
,children
can be any value. The only verification we have is if children is a function than its considered a render function, but we do not limit the children type in any way, in fact some of our components modifychildren
signature to be more specific like:fluentui/packages/react-components/react-field/src/components/Field/Field.types.ts
Line 63 in 5a8d7a2
fluentui/packages/react-components/react-utilities/src/trigger/types.ts
Line 28 in ee9ca8f
fluentui/packages/react-components/react-message-bar/src/components/MessageBarGroup/MessageBarGroup.types.ts
Line 12 in 40e6a37
fluentui/packages/react-components/react-popover/src/components/Popover/Popover.types.ts
Line 28 in 4cfa006
SlotShorthandValue
to supportIterable<ReactNode>
instead ofReactNode[]
(v18 modification)WithSlotRenderFunction
to properly include render function method (Fixes Can't use children render function in slots without Typescript errors #27090)WithoutSlotRenderFunction
SlotPropsDataType
does not have the requirement ofchildren?: ReactNode
the render function signature will leak, and this is a mistake in two scenarios:ComponentProps
should not have a render functionSlotComponentType
properties should not have a render functionVoidFunctionComponent
fromSlot
generic signature (it is deprecated and redundant type, ComponentType covers the same use case)cloneTriggerTree
updates signature to properly assume the possibility of more than justReactNode
fromchildren
value.ForwardRefComponent
andgetIntrinsicElementProps
updates to stop usingReact.RefAttributes
(in the v18 upgrade this type now supportLegacyRef
, which we do not support)What are the Breaking Changes here?
UnknownSlotProps
and now depends onSlotPropsDataType
became less strictComponentProps
now explicitly removes render function signatures from primary slot (this was already supposed to be like that, it's a bugfix IMO)ComponentState
explicitly removes render function signatures to ensure things were as they we beforeTechnically, if the client is using
Slot
,ComponentProps
,ComponentState
, anslot.*
there'll be no breaking changes (as we are just moving verifications from one type to another) but if that is not the case then we could have some false positives due to the wider signature ofSlotPropsDataType
Related Issue(s)