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

Not all interfaces should be marked as constructable #1162

Open
mykhalov opened this issue Oct 1, 2021 · 7 comments
Open

Not all interfaces should be marked as constructable #1162

mykhalov opened this issue Oct 1, 2021 · 7 comments

Comments

@mykhalov
Copy link

mykhalov commented Oct 1, 2021

E.g. trying to call AbortSignal with new throws Illegal constructor error.

> new AbortSignal()
  Uncaught TypeError: Illegal constructor.

This is not currently handled by the declaration:

declare var AbortSignal: {
    prototype: AbortSignal;
    new(): AbortSignal;
};
@saschanaz
Copy link
Contributor

saschanaz commented Oct 2, 2021

This is because instanceof check requires a constructor member. But never seemingly works:

interface Foo {}

declare var Foo: {
    prototype: Foo;
    new(): never;
};

let foo = {} as Foo;
if (foo instanceof Foo) {
  foo // $Foo
}

@orta does new(): never make sense to fix this?

@orta
Copy link
Contributor

orta commented Oct 4, 2021

We bounced the idea back and forth a bit, and generally think this is more likely to cause more breakages than it cures 👍🏻

@saschanaz
Copy link
Contributor

cause more breakages than it cures

What breakages for example?

@aloisklink
Copy link

This also affects NodeList (which could have prevented a bug: mermaid-js/mermaid#3396)

Maybe it's worth putting in a /** @deprecated */ JSDoc tag to constructors that throw Illegal constructor, so some tools (like ESLint/VS Code) will warn about using them.

@saschanaz
Copy link
Contributor

Sounds good to me but I wonder what TS team thinks about the suggestion, maybe @DanielRosenwasser?

@Skalman
Copy link

Skalman commented May 24, 2024

I just ran into this issue. Adding /** @deprecated */ seems like a good and simple fix.

What do you think, @DanielRosenwasser?

@saschanaz
Copy link
Contributor

@sandersn Do you some context around #1162 (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

No branches or pull requests

5 participants