microsoft / fluentui

Fluent UI web represents a collection of utilities, React components, and web components for building web applications.
https://react.fluentui.dev
Other
18.26k stars 2.71k forks source link

[Bug]: slot API is broken on react v18 #29578

Open bsunderhus opened 10 months ago

bsunderhus commented 10 months ago

Library

React Components / v9 (@fluentui/react-components)

Bug Description

Actual Behavior

Currently the slot API types are not compatible with react v18 types provided by @types/react, many components are breaking due to incompatibility with types that weren't detect previously.

For example, the ImageSlot types does not satisfies SlotPropsRecord which is a requirement for ImageProps:

image

In this case the main reason is the assumption of the requirement of a children attribute which is well defined as React.ReactNode, meanwhile ImageProps actually required children to be never

Expected Behavior

No errors on v18 for the slot API.

bsunderhus commented 10 months ago

So basically while investigating our slot API to ensure compatibility with React v18, it turns out that our major flaws in the types revolves around the children property, some of those problems are:

  1. if there's a children property or not (at the moment it's a well defined children?: React.ReactNode, this is the main reason for the fail on ImageProps for example https://github.com/microsoft/fluentui/issues/29578)
  2. if this children property includes a render function or not

our UnknownSlotProps is the default signature of what a slot should be, and it declares children as an optional property (with a value of React.ReactNode, which is not valid in some cases). If we want our components to be compliant as a slot then they should have that property to follow up on the UnknownSlotProps signature.

Here's also another problems that we fail to address at the moment:

  1. a resolved slot component type (slot.optional(props.slotName, {elementType: 'div'}) this is an example of a resolved slot component type. props.slotName this is an example of an unresolved sot, or a slot shorthand) should remove from its children signature the render function, as once a slot is resolved it should move the render function from children to an internal symbol.
  2. name clashes between slot records and native properties https://github.com/microsoft/fluentui/issues/28693#issuecomment-1659737176 https://github.com/microsoft/fluentui/issues/29596
bsunderhus commented 10 months ago

There are 3 main problems right now on the slot signature:

  1. children should not be a well defined ReactNode as this is not true in every slot (there are void slots) (at the moment slots signature it's a well defined children?: React.ReactNode, this is the main reason for the fail on ImageProps for example https://github.com/microsoft/fluentui/issues/29578)
  2. children should support a render function on the external signature, but it should remove it from the internal one (as soon as we stop defining children as ReactNode we'll have the problem of leaking the render function signature, so this is a problem we're avoiding at the moment by erroneously stating that children is ReactNode)
  3. A v9 component should probably be "slot compliant" (meaning, should a v9 component follow the slot signature to be used as a slot? E.g OverlayDrawer uses Dialog internally as a slot, but Dialog children property does not follow slot signature) https://github.com/microsoft/fluentui/blob/4c36df0e2a7bf9d54d5137fabe74c720bdddebbc/packages/react-components/react-drawer/src/components/DrawerOverlay/useDrawerOverlay.ts#L49-L66

We could consider as an extra problem but not directly related to the slot signature:

  1. name clashing of @types/react HTML elements attributes and declared slots such as content https://github.com/microsoft/fluentui/issues/29596
rgylesbedford commented 9 months ago

I've run into this issue as well when using the slot API. Is there any guidance for temporary workarounds apart from downgrading to React 17?

bsunderhus commented 9 months ago

Sadly there's no workaround at the moment @rgylesbedford, we'll be providing a proper fix once the issues here highlighted have been properly discussed! I'll keep this issue posted on this topic

microsoft-github-policy-service[bot] commented 4 months ago

Because this issue has not had activity for over 150 days, we're automatically closing it for house-keeping purposes.

Still require assistance? Please, create a new issue with up-to date details.

bworline commented 3 months ago

While investigating this I created a repro repo that may help.

In the provided repo, open Test.ts and Test2.ts. After initialization you will see red squiggles saying, "Type 'x' does not satisfy the constraint 'SlotPropsRecord'."

image

Note that this repo has a tsconfig "lib" value of es2020. If changed to "es5" the type error goes away. However, look at the difference in how TypeScript is seeing the types in Test2.ts when hovering over menuListWapper: 'any' is hiding the issue. I've found that a lib value of es2015 or greater (or even just es2015.iterable) will cause the issue to manifest.

lib: es5

image

lib: es2020

image
bsunderhus commented 3 months ago

Thanks for the investigations, this will help mapping the problem even further. Here's another issue linked to this one also https://github.com/microsoft/fluentui/issues/31482

bsunderhus commented 3 months ago

Seems like this solution https://github.com/microsoft/fluentui/pull/29865 isn't enough for React 18. It's still breaking due to not properly removing the content property from the slot, but from the props only