fluentui icon indicating copy to clipboard operation
fluentui copied to clipboard

[Bug]: slot cannot be passed a data-* attribute

Open george-cz opened this issue 2 years ago • 19 comments

Library

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

System Info

N/A

Are you reporting Accessibility issue?

no

Reproduction

https://codesandbox.io/s/nice-poitras-wsk2c1?file=/example.tsx

Bug Description

Actual Behavior

Setting a "data-something" attribute to a slot results in TS error. The data attribute is actually set on the appropriate DOM element.

Expected Behavior

Data attributes should be allowed in slot.

Logs

No response

Requested priority

High

Products/sites affected

No response

Are you willing to submit a PR to fix?

yes

Validations

  • [X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
  • [X] The provided reproduction is a minimal reproducible example of the bug.

george-cz avatar Mar 23 '23 10:03 george-cz

Duplicate of #23033?

layershifter avatar Mar 23 '23 13:03 layershifter

@layershifter possibly, seems that the #23033 is about the component prop itself, what this one is about is passing down a data attribute to one of the slots of the component.

This might be caused by the same root issue, but I didn't want to make assumptions.

george-cz avatar Mar 23 '23 13:03 george-cz

@layershifter possibly, seems that the #23033 is about the component prop itself, what this one is about is passing down a data attribute to one of the slots of the component.

This might be caused by the same root issue, but I didn't want to make assumptions.

Yes, this is basically the same issue, and as @behowell described, this would be a possible solution https://github.com/microsoft/fluentui/issues/23033#issuecomment-1234931827.

Just to be clear, this is not a slot problem itself, it's a TS "problem" (it's more like a TS bad decision?); In the case down hese TS is not being forgiving when passing properties that don't belong to a specified type, although it allows on the jsx root itself and also on non overlapping props.

https://www.typescriptlang.org/play?#code/JYWwDg9gTgLgBAKjgQwM5wEoFNkGN4BmUEIcARFDvmQNwBQdwAdjFlAXlnAEICuMMCEwAKxMOgDedOHFQAbCDACMALjgS4YMWqVwAvvRnzFAJjUSDdPQ1xDU8PgKFwAvHAAUWiOLWPBIsVQASlcAPnVpOEoYXigmOCZeOTl6SzosAA9IWDhbJns4ABEsDiT4N3cQl3D3SIAePyFImQB6FrgYAAtgdB64CABrFDkuiF4Ac06OgE8wLHQAEwgExX6ANzY5ZDBmuAXkGGQAWgyXMmA8o4Ajfn8yXbaO7vQOYDl0K6xcZF5ULhhZvM9kIAOTwCAbKBbMAAfjg0xwnQANHAQMgBkCmMs-vksLtjMoXBINGR9ocTmQ1OdLjcnEwyCivGAdPprDJWu0un0+oNhqMJlMAXNFssseDIdD8QoYCYiSSyccMpTyBchNdbkIyKzIqFdgBRDLIcByPEyOotRpMXVBehAA

To give a simple solution for the problem in hand, just cast the type and things will work! https://stackblitz.com/edit/v966zs?file=src%2Fexample.tsx

bsunderhus avatar Mar 23 '23 14:03 bsunderhus

added more context on the issue https://github.com/microsoft/fluentui/issues/23033#issuecomment-1483050986

what @bsunderhus suggesting is the only workaround ATM, which comes with its own set of problems.

Hotell avatar Mar 24 '23 16:03 Hotell

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.

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.

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.

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.

An option is to have mutliple .d.ts versions. We would need a build support for that, worth investigating.

miroslavstastny avatar Aug 31 '23 14:08 miroslavstastny

An option is to have mutliple .d.ts versions. We would need a build support for that, worth investigating.

It wouldn't be entirely possible in our current TS version (v3.9). we would need a set of known data- attributes and include them as optional in the slot definition to avoid this to happen. I believe that for now (meanwhile we still don't support TS 4.4) the best solution is to cast 😟

bsunderhus avatar Oct 05 '23 09:10 bsunderhus

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.

This issue is faced by multiple major partners and needs more attention. We need to recheck TS version matrix and discuss the timelines with @Hotell.

miroslavstastny avatar Jun 10 '24 14:06 miroslavstastny

Seems like our current TS version does not support String Template literal type! But assuming we are able to upgrade our min TS to v4.5 we've still encountered another problem while investigating this through our partners:

error TS2590: Expression produces a union type that is too complex to represent. Here's a TS playground showcasing the problem

Seems like a universal solution with Template literal types will not resolve this issue in our case. As we're using unions of types.

bsunderhus avatar Jun 10 '24 16:06 bsunderhus

This problem sums up on two TS features:

  1. Excess Property Check (EPC)
    • https://github.com/Microsoft/TypeScript/issues/12936
  2. Attribute type checking

We would need some sort of feature on TS side to disable EPC (Excess property checks) on object literals passed to slots. This is not supported by language! every object literal declaration is always EPC!

In Flow every object declaration always accepts extra properties, this is called Width subtyping. This is what we would need on TS to make this work properly 🔍

Note: In TS the only exception for EPC is JSX declarations, JSX properties do not need to be EPC if the property declared is not a valid js property name (something with a - for example): image

Since slot properties are not direct jsx properties, but an object, then EPC rules do apply to object literals on slot properties, only way to avoid this issue is:

  1. do not use object literals
  2. cast
  3. open a feature request on TS for supporting something like Width subtyping (maybe a configuration flag like --noExcessPropertyCheck)

Here's a an example of the problem with Button component and icon slot

bsunderhus avatar Jun 10 '24 17:06 bsunderhus

I opened up https://github.com/microsoft/TypeScript/issues/58898, but in the meantime one way to address this is to use this type instead of Pick:

type DistPick<T, K> =
  T extends infer _ ? (K extends keyof T ? Pick<T, K> : never) : never;

I believe the problem is that Pick requires calculating the type keyof T. In this case, T is a union, where each constituent has many members that must be reduced away when finding the effective set of keys. Something about the index signature is causing us to do a lot of work (probably creating intersections of each key with the template string type), and I haven't investigated it.

But to avoid all of that work, DistPick simply takes a key (which may or may not be constrained to the key set of all constituents) and maps a Pick over each type. That will typically create much smaller types than the original inputs.

The subtlety is that if id has a type that differs, you will end up with a union instead of a flat type like Pick would've given you (see an example here). So if that's a problem, you can wrap DistPick in a final Pick.

type DistPick<T, K> = Pick<
  T extends infer _ ? (K extends keyof T ? Pick<T, K> : never) : never,
  K,
>;

DanielRosenwasser avatar Jun 17 '24 20:06 DanielRosenwasser

This issue has not had activity for over 180 days! We're adding Soft close label and will close it soon for house-keeping purposes. Still require assistance? Please add comment - "keep open".

bot keep open

layershifter avatar Dec 16 '24 09:12 layershifter

This issue has not had activity for over 180 days! We're adding Soft close label and will close it soon for house-keeping purposes. Still require assistance? Please add comment - "keep open".

bot keep open

layershifter avatar Jun 16 '25 08:06 layershifter