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

Strongly Typed Svelte Component Interface #37

Merged
merged 6 commits into from
Oct 26, 2020

Conversation

dummdidumm
Copy link
Member

@benmccann
Copy link
Member

One thing I noticed in Sapper is that the codebase never actually got type-checked despite using TypeScript because it was using the sucrase plugin, which skips type checking. A lot of the types were wrong as a result

It looks like svelte mostly uses sucrase as well, though uses the typescript plugin when publishing. If we're going to start publishing more types, a good first step would be to use the typescript plugin at least when running the tests on the CI server so that we can make sure when we check in code that we're conforming to the types we're writing. In Sapper I used rollup-plugin-typescript2 because it's twice as fast and you can toggle type-checking on and off depending on which npm task you're running (vs toggling between @rollup/plugin-sucrase and @rollup/plugin-typescript based on whether you want typechecking)

Also, if we're going to type the components, one thing to mention is that I think we would need separate type definitions for the DOM and SSR Components.

@dummdidumm
Copy link
Member Author

It looks like svelte mostly uses sucrase as well, though uses the typescript plugin when publishing. If we're going to start publishing more types, a good first step would be to use the typescript plugin at least when running the tests on the CI server so that we can make sure when we check in code that we're conforming to the types we're writing.

I looked at the tests and they are all using JS only, so no point in using TS for type checking there. Since the build does type checking, I think we're save.

Also, if we're going to type the components, one thing to mention is that I think we would need separate type definitions for the DOM and SSR Components.

This RFC is only about enhancing the typing in such a way that you can use to author types. Adding the type definition for SSR could be just adding a static "render" method to the type definition.

dummdidumm pushed a commit to dummdidumm/svelte that referenced this pull request Sep 20, 2020
@benmccann
Copy link
Member

Since the build does type checking, I think we're save.

I think the build only does type-checking when cutting a release though? It'd be nice to do it more often to catch errors before then

@benmccann
Copy link
Member

I converted a lot of Svelte's tests to TypeScript here: sveltejs/svelte#5433

@nateshmbhat
Copy link

any ETA on when this can be merged ?

@orta
Copy link
Contributor

orta commented Oct 12, 2020

Re:

The $$prop_def/$$events_def/$$slot_def leak.

I'm not sure how I see those leaking? They look completely internal to the language server to me from this RFC. Well, I guess you could look inside the svelte .d.ts to see them.

My assumptions: As ever, so far I've managed to make a svelte app with 2 components, so consider my take naive. However, I did read the code for carbon-components-svelte who is already using the $$ variables ( sneaky but cool @metonym )

I assume that every .svelte file would have an equivalent .d.ts: e.g.

// Answer.svelte
<script>
	export let answer;
</script>

<p>The answer is {answer}</p>

Would need something like:

// Answer.d.ts
import { SvelteComponent } from 'svelte';
export default class Answer extends SvelteComponent<{ answer: string }, { }, { }> {}

This would also get scooped up probably into a root module export which would look something like:

import { SvelteComponent } from 'svelte';

module "testing" {
   export class Answer extends SvelteComponent<{ answer: string }, { }, { }> {}
   export class Question extends SvelteComponent<{ question: string, subtitle: string }, { }, { }> {}
}

Which feels like what this RFC proposes - so it gets a 👍🏻 from me

Where I think this RFC could still do with a bit of work: I think this could be mostly tool powered too, could a rollup plugin exist for generating the svelte types by asking for info (in this format) from the language service?

@dummdidumm
Copy link
Member Author

Re:

The $$prop_def/$$events_def/$$slot_def leak.

I'm not sure how I see those leaking? They look completely internal to the language server to me from this RFC. Well, I guess you could look inside the svelte .d.ts to see them.

True. My worries were/are that those are purely for type checking and don't exist at runtime. But they would turn up in the autocompletion when someone uses the the instance of that class type in a script tag. But we could in language-tools filter out these completions. At that point it's like you said, someone would have to look this up to actually find it.

Where I think this RFC could still do with a bit of work: I think this could be mostly tool powered too, could a rollup plugin exist for generating the svelte types by asking for info (in this format) from the language service?

This is a second step after this one is agreed on (which looks like it almost is). As you said, I envision some kind of small package which could do this, which then could get wrapped by a rollup plugin.

@orta
Copy link
Contributor

orta commented Oct 23, 2020

Alright, looks like we've got general consensus that this is the way forwards 👍🏻 - I'll merge tomorrow unless we get any other feedback

@nateshmbhat
Copy link

Waiting patiently to be merged 🧘‍♂️

@firefish5000
Copy link

firefish5000 commented Oct 24, 2020

Props extends {} = any

Any reason not to use

Props extends Record<string,any> = any

instead? Just to ensure type errors are caught while typeing the defs so if someone types something like

Svelte2TsxComponent<'name' ,2 ,(evt: any)=>void>

it will be caught as an error right away rather than waiting for someone (possibly downstream) to try to use the component and report the error.

I have been using this for a few weeks now as well and the only changes I made were that and use of unknown instead of any, but that probably should not be done here since, iirc, it forces you to add declarations for any prop you use.

@dummdidumm
Copy link
Member Author

In the implementing PR I actually did Props extends Record<string, any> = any, I'll update the RFC to match it.

@orta
Copy link
Contributor

orta commented Oct 26, 2020

Agree, good feedback 👍🏻 OK - I"m going to hit the merge button. Thanks all!

@orta orta merged commit 29ddd9d into sveltejs:master Oct 26, 2020
@dummdidumm dummdidumm deleted the svelte-component-interface branch October 26, 2020 15:45
@TheComputerM
Copy link

Where should be the documentation on how to use this?

@nateshmbhat
Copy link

By when can this be published on npm?

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

Successfully merging this pull request may close these issues.

6 participants