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

Revert typed SvelteComponent, add SvelteComponentTyped instead #5738

Merged
merged 5 commits into from
Dec 2, 2020

Conversation

dummdidumm
Copy link
Member

const a: typeof SvelteComponent = ASubclassOfSvelteComponent throws errors, which means existing usages are broken. To prevent this, the changes to a typed SvelteComponent in #5431 are reverted. Instead a new SvelteComponentTyped class is added, which should be used for typing instead.

Simon Holthausen added 2 commits December 2, 2020 10:01
`const a: typeof SvelteComponent = ASubclassOfSvelteComponent` throws errors, which means existing usages are broken. To prevent this, the changes to a typed `SvelteComponent` are reverted. Instead a new `SvelteComponentTyped` class is added, which should be used for typing instead.
@benmccann
Copy link
Member

I don't quite understand why there's a SvelteComponent, SvelteComponentDev, and SvelteComponentTyped. Would you be able to update the documentation as well? I think that would help both users with usage and the maintainers in reviewing this PR

@benmccann
Copy link
Member

Maybe add a code comment that they're separated because of microsoft/TypeScript#41770 and a TODO to combine the components in Svelte 4.0.0?

@dummdidumm
Copy link
Member Author

I don't quite understand why there's a SvelteComponent, SvelteComponentDev, and SvelteComponentTyped. Would you be able to update the documentation as well? I think that would help both users with usage and the maintainers in reviewing this PR

Good point, I added some docs.

Maybe add a code comment that they're separated because of microsoft/TypeScript#41770 and a TODO to combine the components in Svelte 4.0.0?

Honestly I'm not sure if this is even a bug, it might also be as designed. I want to wait on a response from the TS team before adding a comment like this.

@benmccann
Copy link
Member

Thanks for adding the comments. Makes sense not to call it a bug for now. The issue might still be useful for reference even if designed that way, but there's enough comments that I think it's fine without too.

Would adding a TODO to combine the classes in 4.0 make sense? I was under the impression that a new Typed class was created to maintain backwards compatibility, but it's not what we would do if starting from scratch.

@dummdidumm
Copy link
Member Author

Would adding a TODO to combine the classes in 4.0 make sense? I was under the impression that a new Typed class was created to maintain backwards compatibility, but it's not what we would do if starting from scratch.

Yes it's for backwards compatibility and ease of use - if it's not a TS error but "works as designed", people would have to write class GenericSvelteComponent extends SvelteComponent {} somewhere and use that for typeof everywhere. I will add a TODO with a link in the code through a regular comment so it doesn't turn up in the JSDoc.

@benmccann
Copy link
Member

This looks good to me. Thanks for hopping on it so quickly

Should we add something in site/content/docs as well?

The only other question I had is whether SvelteComponentTyped might be a bit long of a name, but I couldn't come up with anything better, so am okay with that as a name

@dummdidumm
Copy link
Member Author

Should we add something in site/content/docs as well?

I was thinking about that, too. I first want to see if things turn out well now before making this more public.

The only other question I had is whether SvelteComponentTyped might be a bit long of a name, but I couldn't come up with anything better, so am okay with that as a name

My thoughts as well 😄 I chose it because it matches the postfix-style of SvelteComponentDev.

@dummdidumm
Copy link
Member Author

TS team confirmed that this works as designed, so we need to make with this change.

@Conduitry Conduitry merged commit 79214cc into sveltejs:master Dec 2, 2020
@dummdidumm dummdidumm deleted the fix-component-def branch December 2, 2020 21:32
Copy link

@guidobouman guidobouman left a comment

Choose a reason for hiding this comment

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

One whitespace thing. But mainly: should this not have been a breaking change?

}

super();
}) {
Copy link

@guidobouman guidobouman Dec 4, 2020

Choose a reason for hiding this comment

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

Whitespace.

@@ -11,7 +11,7 @@ It's the last "What's new in Svelte" of the year and there's lots to celebrate!

1. `$$props`, `$$restProps`, and `$$slots` are all now supported in custom web components (**3.29.5**, [Example](https://svelte.dev/repl/ad8e6f39cd20403dacd1be84d71e498d?version=3.29.5)) and `slot` components now support spread props: `<slot {...foo} />` (**3.30.0**)
2. A new `hasContext` lifecycle function makes it easy to check whether a `key` has been set in the context of a parent component (**3.30.0** & **3.30.1**, [Docs](https://svelte.dev/docs#hasContext))
3. `SvelteComponent` is now typed which makes it easier to add typed classes that extend base Svelte Components. Component library and framework authors rejoice! An example: `export class YourComponent extends SvelteComponent<{aProp: boolean}, {click: MouseEvent}, {default: {aSlot: string}}> {}` (**3.30.0**, [RFC](https://github.com/sveltejs/rfcs/pull/37))
3. There is now a new `SvelteComponentTyped` class which makes it easier to add strongly typed components that extend base Svelte components. Component library and framework authors rejoice! An example: `export class YourComponent extends SvelteComponentTyped<{aProp: boolean}, {click: MouseEvent}, {default: {aSlot: string}}> {}` (**3.30.0**, [RFC](https://github.com/sveltejs/rfcs/pull/37))

Choose a reason for hiding this comment

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

Was this broken to begin with? Otherwise this is a breaking change, no? The syntax extends SvelteComponent<...> { ... } ceases to work, right?

Copy link
Member Author

@dummdidumm dummdidumm Dec 4, 2020

Choose a reason for hiding this comment

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

That syntax was introduced in 3.30 but reverted in 3.31 because it introduced a breaking change. const a: typeof SvelteComponent = ASubclassOfSvelteComponent threw errors which it did not before. So yes, the syntax extends SvelteComponent<...> { ... } ceases to work, but it only worked for a few days / in 3.30.0.

@benmccann
Copy link
Member

@dummdidumm it looks like lint might be failing on master because of this PR. Would you be able to take a look?

@Conduitry
Copy link
Member

npm run lint is passing for me on current master. It also looks fine in https://github.com/sveltejs/svelte/runs/1488882690

@benmccann
Copy link
Member

Ah. It was failing for me in #5746, but maybe that's because I bumped the eslint version which made it able to detect the unused variables

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.

4 participants