Skip to content
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

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

Open
2 tasks done
george-cz opened this issue Mar 23, 2023 · 15 comments · May be fixed by #31754
Open
2 tasks done

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

george-cz opened this issue Mar 23, 2023 · 15 comments · May be fixed by #31754

Comments

@george-cz
Copy link
Contributor

george-cz commented Mar 23, 2023

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

  • Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
  • The provided reproduction is a minimal reproducible example of the bug.
@layershifter
Copy link
Member

Duplicate of #23033?

@george-cz
Copy link
Contributor Author

@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.

@bsunderhus
Copy link
Contributor

bsunderhus commented Mar 23, 2023

@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 #23033 (comment).

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 bsunderhus changed the title [Bug]: slot cannot be passed a data attribute [Bug]: slot cannot be passed a data-* attribute Mar 23, 2023
@Hotell
Copy link
Contributor

Hotell commented Mar 24, 2023

added more context on the issue #23033 (comment)

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

@Hotell Hotell added Status: Blocked Resolution blocked by another issue Area: Typescript labels Mar 24, 2023
@microsoft-github-policy-service

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.

1 similar comment
@microsoft-github-policy-service

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.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Resolution: Soft Close Soft closing inactive issues over a certain period label Aug 25, 2023
@microsoft-github-policy-service

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.

1 similar comment
@microsoft-github-policy-service

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.

@layershifter layershifter reopened this Aug 28, 2023
@layershifter layershifter removed the Resolution: Soft Close Soft closing inactive issues over a certain period label Aug 28, 2023
@miroslavstastny
Copy link
Member

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

@bsunderhus
Copy link
Contributor

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 😟

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.

@microsoft-github-policy-service microsoft-github-policy-service bot added the Resolution: Soft Close Soft closing inactive issues over a certain period label Mar 3, 2024
@layershifter layershifter reopened this Mar 4, 2024
@layershifter layershifter removed the Resolution: Soft Close Soft closing inactive issues over a certain period label Mar 4, 2024
@miroslavstastny
Copy link
Member

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.

@bsunderhus
Copy link
Contributor

bsunderhus commented Jun 10, 2024

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 bsunderhus removed the Status: Blocked Resolution blocked by another issue label Jun 10, 2024
@bsunderhus
Copy link
Contributor

bsunderhus commented Jun 10, 2024

This problem sums up on two TS features:

  1. Excess Property Check (EPC)
  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

@DanielRosenwasser
Copy link
Member

I opened up microsoft/TypeScript#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,
>;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants