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: Use declare class where possible #858

Closed
wants to merge 2 commits into from

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented May 21, 2020

This changes emit for interfaces:

interface Foo : Bar {
	constructor();
};
Foo includes Baz;

From:

declare interface Foo extends Bar, Baz {}
declare var Foo: {
	readonly prototype: Foo;
	new(): Foo;
}

To:

declare interface Foo extends Baz {};
declare class Foo extends Bar {
	constructor();
]

Which has the added benefit of supporting declaration merging for static methods and properties.


It also prevents static properties from breaking // $ExpectType on constructors, since it just becomes // $ExpecType typeof Foo.


This shouldn’t be a breaking change as existing consumers will keep working just fine, this just opens up new ways to extends these definitions.


@ExE-Boss ExE-Boss requested a review from sandersn as a code owner May 21, 2020 16:33
@ExE-Boss ExE-Boss force-pushed the feat/prefer-declare-class branch 2 times, most recently from ea1e436 to e55795c Compare May 21, 2020 16:48
@@ -120,6 +120,16 @@ function isEventHandler(p: Browser.Property) {
return typeof p["event-handler"] === "string";
}

const newToConstructorRegExp = /^new(?:<.+?>)?(\(.+?\))(?:: ?[^)]+)?$/u;
function convertNewToConstructor(signature: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems only for manual JSON data, in that case I think those should be fixed instead.

@sandersn
Copy link
Member

This is a possibly gigantic breaking change. I created a PR on Typescript, microsoft/TypeScript#39044, so we can run DT and other tests on it.

@sandersn
Copy link
Member

We discussed this in the design meeting and decided not to take this change.

  • The DOM types are not really classes.
  • Static-side merging still only works with namespaces, which is not discoverable (and fewer people over time know how to use namespaces at all).
  • So far, for cases where we want to enable static-side merging, we manually introduce an interface for the static side. We would take a PR that generates these interfaces for all class-like types.

@sandersn sandersn closed this Jun 15, 2020
@saschanaz
Copy link
Contributor

Added a comment: microsoft/TypeScript#39054 (comment)

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.

3 participants