Skip to content

data- attributes should be allowed on components that wrap HTML elements #1825

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

Closed
dummdidumm opened this issue Jan 9, 2023 · 8 comments
Closed
Labels
feature request New feature or request Fixed Fixed in master branch. Pending production release.

Comments

@dummdidumm
Copy link
Member

Description

<script lang="ts">
  import { Button } from 'carbon-components-svelte';
</script>

<Button on:click={() => alert('do thing')} data-cy={'forwarded-to-html-button'}>Button</Button>

produces

Error: Type '{ "data-cy": string; }' is not assignable to type 'ButtonProps'.
  Object literal may only specify known properties, and '"data-cy"' does not exist in type 'ButtonProps'. (ts)

The error is legitimate since indeed the types for the carbon Button component do not include data-cy. But the Button component does forward it to the HTML button through ...$$restProps, and data- attributes are valid for html elements.

Proposed solution

Best is probably to add an index signature to the html typings in svelte/elements which looks like this:

interface HTMLAttributes {
  [key: `data-${string}`]: any;
}

... but we can't do that until Svelte 4 because Svelte 3 has a minimum version requirement on a TS version that doesn't have that feature yet.
Maybe it's possible to add that inside svelte2tsx in the meantime.

Alternatives

No response

Additional Information, eg. Screenshots

No response

@dummdidumm dummdidumm added the feature request New feature or request label Jan 9, 2023
@BeeMargarida
Copy link

This would be amazing, I just hit this problem now

@juane1000
Copy link

juane1000 commented Mar 22, 2023

This seems to be an issue when using svelte actions (directives-like?) and binding custom events on the target element. App compiles fine but vs-code marks it as an error.

@jasonlyu123
Copy link
Member

For a library, users or library authors can enhance this in the userland for now.

declare module 'svelte/elements' {
	interface HTMLAttributes<T> {
		[x:`data-${string}`]: any; 
	}
}

But this only work in the manually typed component because of another problem. In our generated code, we use spread for merging props and export const, which seems to cause TypeScript to remove the index signature. Demo in TypeScript playground.

@dummdidumm
Copy link
Member Author

dummdidumm commented May 12, 2023

Thanks for digging into that! While doing the Svelte PR which changes this in core I was wondering why intellisense wasn't forwarding it. Since it's only possible to add an index signature to the props interface by using $$Props I'm wondering if we can work around this by handling that case differently. Something like props: {..} as $$Props & { ..the type of the export consts, if any }

@rsmarples
Copy link

... but we can't do that until Svelte 4 because Svelte 3 has a minimum version requirement on a TS version that doesn't have that feature yet.

We now have svelte 4 stable version released. Can this be progressed please? It's currently blocking our upgrade.
Thanks!

@dummdidumm
Copy link
Member Author

Minimum reproducible for the issue:

interface Foo {
    x?: true;
    [key: `data-${string}`]: any;
}

const foo: Foo = {
    'data-x': true // works
};
const destructured = {... {} as Foo, ...{} as {}  }
const bar: typeof destructured = {
    'data-x': true // fails
}

It seems that TS does not use index signatures as soon as two separate things are spread onto the same object. But that's what we're doing under the hood for intellisense generation of the prop type. So we need to change that to something else.

dummdidumm added a commit that referenced this issue Sep 8, 2023
Using a type union instead of spread, because the latter does remove index signatures from the resulting type
#1825
dummdidumm added a commit that referenced this issue Sep 11, 2023
Using a type union instead of spread, because the latter does remove index signatures from the resulting type
#1825
@dummdidumm dummdidumm added the Fixed Fixed in master branch. Pending production release. label Sep 11, 2023
@huntabyte
Copy link
Member

huntabyte commented Nov 10, 2023

Hey @dummdidumm, is the [key: `data-${string}`]: supposed to be stripped out on build? Everything works when declaring it inside the project, but as soon as I build, it strips that type from the props in the .d.ts file generated from the component.

If I manually edit the .d.ts file and try to use the component from dist, then all is well, just not really an ideal situation 😂

Minimal reproduction where the only thing I'm specifying as the prop types is that template lit:

Component Def:
CleanShot 2023-11-10 at 15 57 34@2x

Build Output:
CleanShot 2023-11-10 at 15 56 14@2x

@huntabyte
Copy link
Member

Disregard - in the process of creating a repro repo, we realized something in our lockfile was hanging on to an older version of svelte2tsx for dear life!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request Fixed Fixed in master branch. Pending production release.
Projects
None yet
Development

No branches or pull requests

6 participants