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

Provide augmented HTML typings in SvelteKit itself #10534

Closed
dummdidumm opened this issue Aug 11, 2023 · 2 comments · Fixed by #11222
Closed

Provide augmented HTML typings in SvelteKit itself #10534

dummdidumm opened this issue Aug 11, 2023 · 2 comments · Fixed by #11222

Comments

@dummdidumm
Copy link
Member

Describe the problem

Right now, things like data-sveltekit-preload-data are added inside the Svelte repo's HTML type definitions. With sveltejs/language-tools#2109 adressed we can investigate whether or not it makes sense to have these additional types provided by SvelteKit itself.

Describe the proposed solution

Have a file inside SvelteKit that is loaded through the generated ambient.d.ts file which enhances the typings like this.

declare module "svelte/elements" {
  export interface HTMLAttributes {
    'data-sveltekit-keepfocus'?: true | '' | 'off' | undefined | null;
    // .. etc
  }
}

Alternatives considered

keep the tighter-than-it-needs-to-be-connection to Svelte's typings.

Importance

would make my life easier

Additional Information

I haven't quite figured out if this is a breaking change or not - i.e. is there something stopping us from just shipping this today and then removing the types in a future Svelte version, which the next major version then only supports? It's probably a breaking change because it would mean we need to remove the typings on the Svelte core side, so probably easiest to do along with Svelte 5.

@stephane-vanraes
Copy link
Contributor

I am trying to hook into this approach to limit custom props to specific elements (following the guide here https://github.com/sveltejs/language-tools/blob/master/docs/preprocessors/typescript.md#im-using-an-attributeevent-on-a-dom-element-and-it-throws-a-type-error I made the following code:

declare namespace svelteHTML {
	interface HTMLAttributes {
		variant?: 'primary' | 'secondary';
	}
}
<button variant="primary">Test</button>
<button>Test none</button>
<button variant="tertiary">Test</button>
<p variant="primary">Test</p>

The goal would be that I get two warnings: for variant on the paragraph tag, and one for the value of tertiary (the two last elements in the test).

This does only give an error on tertiary (logically because nowhere I have restricted it to just buttons)

I tried the following so far:

interface HTMLAttributes<HTMLButtonElement>

interface HTMLButtonAttributes

both using declare module "svelte/elements" and declare namespace svelteHtml

@dummdidumm
Copy link
Member Author

dummdidumm commented Dec 7, 2023

Usage of dts-buddy has complicated this, because there's now no file where we can put declare module ".." along with a export {} to make the file non-ambient. It would need to happen in a generated file inside the .svelte-kit folder.

I also realized that the addition of data-${string} to HTMLAttributes in svelte/elements in combination with our type HTMLProps<Property extends string, Override> = Omit<import('svelte/elements').SvelteHTMLElements[Property], keyof Override> & Override; type in language tools means that data attributes that are explicitly specified are no longer type checked properly, the data-${string} somehow overrides them so that any is allowed on all of them.

dummdidumm added a commit that referenced this issue Dec 7, 2023
This makes it possible to delete these from svelte/elements in Svelte 5 and have them controled in SvelteKit, decoupling the two
closes #10534
@benmccann benmccann linked a pull request Dec 9, 2023 that will close this issue
5 tasks
dummdidumm added a commit that referenced this issue Dec 11, 2023
This makes it possible to delete these from svelte/elements in Svelte 5 and have them controled in SvelteKit, decoupling the two
closes #10534
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.

2 participants