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

[feat] add convenience types ComponentType and ComponentProps #6770

Merged

Conversation

dummdidumm
Copy link
Member

@dummdidumm dummdidumm commented Sep 25, 2021

Eases typing "this is a variable that expects a Svelte component constructor (of a certain shape)". Removes the need for SvelteComponentTyped to be an extra type so it can be deprecated in v4 and removed in v5, and SvelteComponent(Dev) can receive the same generic typings as SvelteComponetTyped in v4.

Also add ComponentProps to get the props a component expects (fixes #7584)

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with [feat], [fix], [chore], or [docs].
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

Eases typing "this is a variable that expects a Svelte component constructor (of a certain shape)". Removes the need for SvelteComponentTyped to be an extra type so it can be deprecated in v4 and removed in v5, and SvelteComponent(Dev) can receive the same generic typings as SvelteComponetTyped in v4.
@dummdidumm dummdidumm added the runtime Changes relating to runtime APIs label Sep 25, 2021
@dummdidumm
Copy link
Member Author

Seems we need to bump the TypeScript version as it's too old to use the new type modifier. If I don't add it, the export is not stripped and it's a runtime error.

@dummdidumm
Copy link
Member Author

I ended up bumping TS, acknowledging the fact that it may introduce accidentally adding type syntax later that is incompatible with TS 3.7 . Doing so technically is a breaking change, although I doubt that anyone is using a TS version this old, especially since everyone using TS with Svelte is required to use a newer version of TS. But because it's a breaking change, I decided to add a short script which rewrites the new type syntax to something valid in 3.7 . @Conduitry hope this is okay for you.

@Conduitry
Copy link
Member

especially since everyone using TS with Svelte is required to use a newer version of TS

@dummdidumm Required by what? SvelteKit? Other tooling?

Is this a breaking change only insofar as someone might be manually interacting with the types using an old version of the TS compiler, separately from what any of the official tooling is already doing?

@dummdidumm
Copy link
Member Author

It's a breaking change for everyone who uses an old TS version which does not understand the export type syntax that would be introduced by this PR, if the postbuild script wouldn't exist. Not exactly sure in what way it would break though (no more intellisense, module typed as any, I don't know). As soon as you use Svelte with TS and import from svelte the break would occur.

@dummdidumm dummdidumm changed the title [feat] add convenience SvelteComponentConstructor type [feat] add convenience types ComponentType and ComponentProps Jun 28, 2022
@dummdidumm
Copy link
Member Author

I updated the PR and adjusted the code. I switched out SvelteComponentConstructor with ComponentType which provides more flexibility (you can also use it to get the constructor type of an existing component) and is easier to grok in my opinion. I also adjusted the type generation code so that the TS bump is no longer needed.

@ivanhofer @ignatiusmb could you have a look at this? More specifically:

  • names of these convenience types good?
  • docs understandable?
  • actually helpful convenience types?

Copy link
Contributor

@ivanhofer ivanhofer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice additions of useful type helpers.
The tsd.js is a smart way to bypass the failing types 👏

src/runtime/index.ts Show resolved Hide resolved
@@ -128,7 +128,7 @@ export interface SvelteComponentDev {
$destroy(): void;
[accessor: string]: any;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file exports an interface SvelteComponentDev and a class with the same name. The types of the interface and the class don't match. It was not added in this PR but I find it confusing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also not part of this PR but wouldn't it be better to combine SvelteComponentDev and SvelteComponentTyped. Currently SvelteComponentTyped is just SvelteComponentDev with some generics that all contain default parameters. When not adding any generics this should result into the same type as SvelteComponentDev

src/runtime/internal/dev.ts Outdated Show resolved Hide resolved
src/runtime/internal/dev.ts Outdated Show resolved Hide resolved
src/runtime/internal/dev.ts Outdated Show resolved Hide resolved
src/runtime/internal/dev.ts Outdated Show resolved Hide resolved
src/runtime/internal/dev.ts Outdated Show resolved Hide resolved
dummdidumm and others added 4 commits June 28, 2022 22:47
Co-authored-by: Hofer Ivan <ivan.hofer@outlook.com>
Co-authored-by: Hofer Ivan <ivan.hofer@outlook.com>
Co-authored-by: Hofer Ivan <ivan.hofer@outlook.com>
Co-authored-by: Hofer Ivan <ivan.hofer@outlook.com>
Copy link
Member

@ignatiusmb ignatiusmb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be a nice addition to the Svelte + TS integration when it gets in!

src/runtime/internal/dev.ts Outdated Show resolved Hide resolved
src/runtime/internal/dev.ts Outdated Show resolved Hide resolved
src/runtime/internal/dev.ts Show resolved Hide resolved
src/runtime/internal/dev.ts Outdated Show resolved Hide resolved
src/runtime/internal/dev.ts Outdated Show resolved Hide resolved
src/runtime/internal/dev.ts Outdated Show resolved Hide resolved
src/runtime/internal/dev.ts Outdated Show resolved Hide resolved
dummdidumm and others added 6 commits June 29, 2022 10:02
Co-authored-by: Ignatius Bagus <ignatius.mbs@gmail.com>
Co-authored-by: Ignatius Bagus <ignatius.mbs@gmail.com>
Co-authored-by: Ignatius Bagus <ignatius.mbs@gmail.com>
dummdidumm and others added 2 commits July 1, 2022 14:31
Co-authored-by: Ignatius Bagus <ignatius.mbs@gmail.com>
@dummdidumm dummdidumm merged commit 6f57571 into sveltejs:master Jul 1, 2022
@dummdidumm dummdidumm deleted the sveltecomponentconstructor-type branch July 1, 2022 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
runtime Changes relating to runtime APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add helper types to extract props a component accepts
4 participants