-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Allow to specify return type of constructor #27594
Comments
Can't you just |
And what if I "forgot" to do that? |
If you really want to force the user to use the interface (even when they're actually creating an instance of the class), then make the constructor private and add a static create-method. |
Yes, I've already mentioned it. But it requires writing some boilerplate code, and my solution is simple and DRY (because for a static create method you should re-type all arguments from constructor). |
I'm reading the linked issues. Here is one of the reasons that was advanced against this:
The source-map library returns a https://github.com/mozilla/source-map/blob/master/lib/source-map-consumer.js#L18-L27 I am also supporting this kind of feature: I want to define constructor overloads to get stronger type inference when relating constructor arguments to the properties in my class. |
My issue was marked as a duplicate of this one and closed, so I'm going to chime in here. What I care about is whether TS allows you to set the type parameters for your own type using the constructor return syntax. To understand why I need this, take a look at http://burningpotato.com/sequins-docs/#/Map. This class type is impossible to represent at the moment without using two (constructor, instance) interfaces to pretend to be a class. There's nothing abnormal going on at runtime though. My problem is not being able to express the type at all, as clearly I've already done that, my beef is that since there's nothing deviant going on, it should be trivial to express such a type to consumers, which clearly it is not. Also to clarify I propose that there are two terms in play here:
I actually think it's beneficial for a class type to only describe a class, not any newable. Interfaces with new keywords seem like an OK way to describe what's going on for non-class newables. It certainly weakens the readability of the language to weaken the definition of a class type to fit any newable. At that point seeing that something is a class doesn't necessarily tell you anything at all. What I could go either way on is allowing a constructor return to return a subtype of the class. This does fulfill the class contract regarding instanceof, and it certainly could be useful in code. I don't need it personally, so I'm not worried. |
I want to add more examples. Let's say you have a class holding an array of items. type Item = {/*some fields*/}
// Wrong! Consumers can do new Api().items = [] by mistake
class Api {
items: ReadonlyArray<Item>
}
// Wrong! Consumers can not mutate `items`, but
// now it's assignable only in constructor
// and that's pretty useless. What if we need to alter the array in .someMethod()?
class Api2 {
readonly items: ReadonlyArray<Item>
}
// Wrong! We can alter the array inside our class, but
// consumers can do new Api3().items.push('whatever')
class Api3 {
readonly items: Array<Item>
}
// OK! But why type the same twice?
// Also, if `_items` is going to be reassigned, we should write a getter for `items`, see:
class Api4 {
_items: Array<Item>
readonly items: ReadonlyArray<Item> = this._items
_items2: Array<Item>
readonly items2: ReadonlyArray<Item> = this._items2
someMethod() {
this._items2 = this.processNewItems()
// Oops, we forgot to update this.items2, now it points to an older version of `_items2`
}
// Possible solutions:
// 1) declare items2 as:
get items2() { return this._items2 }
// 2) use a helper method:
private setItems2(items: ReadonlyArray<Item>) {
this._items2 = items
this.items2 = items
}
// Now the question is: are you ready to write boilerplate code?
}
// Use a private ctor with static factory?
// OK, but you need to re-type all arguments from ctor
class Api5 {
private constructor(
private readonly a: SomeLongNameYouHaveToCopyTwice,
private readonly b: AnotherThing,
c: AndAnother
) {}
// here the `fun` begins:
static create(
a: SomeLongNameYouHaveToCopyTwice,
b: AnotherThing,
c: AndAnother
) {
// all of above just to
return new Api5(a, b, c) as IApi
}
}
// Another attempt
interface IApi {
readonly items: ReadonlyArray<Item>
}
const api = new Api() as IApi
// OK, but this is an optional solution:
// a consumer may write a type cast or may not.
// It's better to enforce this. And now let me introduce suggested solution: // 1) create an interface (IApi) from above
interface IApi {
readonly items: ReadonlyArray<Item>
}
// 2)
class Api {
items: Array<Items> // do whatever you want with items inside a class...
constructor(): IApi // ...but outside it will be readonly
{}
}
const api = new Api() // infers as IApi So, all what I suggest is actually making the following line valid: And it is as simple as this:
Right now I use it like: class SomeClass implements ISomeInterface {
// implementation...
}
export default SomeClass as Interface<ISomeInterface, typeof SomeClass> It works fine, but little too verbose. |
Here's another example that woks in javascript and there are a lot of use-cases where it can be used. class Bar {
constructor(): Promise<this> {
// do some stuff
return new Promise((resolve) => {
return this;
});
}
foo(): string {
return 'foo';
}
}
(async () => {
const a = await new Bar();
a.foo()
})() So specifying constructor type is must have feature for typescript. Yes someone thinks that it's a bad practice #11588 (comment). However decision what's bad or not should be made by developers of app (probably with help of linters). And language should provide tools. And in case of typescript, as it is superset of javascript it should provide the same tools that are available in javascript. And if it doesn't, it couldn't be called "superset", it will be reduced set. |
@SkeLLLa Nothing is preventing you from doing class Bar {
static async create() {
//...
}
} In particular if you want to strip the |
@saintcrawler Wouldn't another potential solution be defining a |
@conartist6 why should I have additional static method, if I class already has a constructor? As a workaround it's okay, but not for usage on regular basis. In js And for "implicit meanings", as I've wrote before it's said on typescript main page it is written that "TypeScript is a typed superset of JavaScript that compiles to plain JavaScript". So by the implicit meaning it stands for that typescript should cover all valid cases and constructions that I could have in javascript. And the case we're talking about is not covered. |
@conartist6 javascript uses If I understand you correctly, you suggest something like this? class MyClass {
private name: string
private address: {country: string, city: string, zip: number}
public get(key: string) {
return this[key]
}
} That case works only for primitive types, i.e. Or do you suggest something like this? class MyClass {
private name: string
getName() { return this.name }
// or with property getter
private _name: string
get name() { return this._name }
private address: {country: string, city: string, zip: number}
getAddress() { return this.address } // you need to use Readonly<T> here
} For me it's just a useless boilerplate code. It may be helpful in plain JS, but in TS it's much easier to use a type system to restrict access to data. As for your example with static create function, here is, again, an excerpt from my big example from previous post: class Api5 {
private constructor(
private readonly a: SomeLongNameYouHaveToCopyTwice,
private readonly b: AnotherThing,
c: AndAnother
) {}
static create(
a: SomeLongNameYouHaveToCopyTwice,
b: AnotherThing,
c: AndAnother
) {
return new Api5(a, b, c)
}
} When a constructor does not use any arguments it's more or less fine (but still a boilerplate), But when you need to pass several arguments (e.g., for Dependency Injection) it's a completely nightmare. And, also as @SkeLLLa noted, it's a valid code from JS perspective. So, I don't see any good arguments againts including this feature. Moreover, it's not that hard to implement for TS-team and it won't be a breaking feature. |
Don't get me wrong, I want the feature too. It's just want a less permissive version of it, because that's what I need. What I'm really suggesting is that if I read the thread above, each of you has your own separate concept of what it means to be a class, and what the |
@saintcrawler Why not make your addresses fully immutable? Addresses don't change too often, do they? With dependency injection I certainly wouldn't want code inside or outside the class to be able to rewire fields set up by the dependency injector. Sorry, I'm not trying to be obstructionist, I just want to understand what you need to do and whether there is really no other way. |
@conartist6 Imagine the following interface: interface IFooLoader {
readonly isLoading: boolean
readonly foos: ReadonlyArray<IFoo>
//...loadFoos(), other members, etc...
} How would you implement that interface with respect to A: class FooLoader implements IFooLoader {
get isLoading() { return this._isLoading }
private _isLoading = false
get foos() { return this._foos }
private _foos = []
//etc...
} B: class FooLoader implements IFooLoader {
isLoading = false
foos = []
private constructor() {}
static create() {
return new FooLoaderB() as IFooLoader
}
//etc...
} C: class FooLoader implements IFooLoader {
isLoading = false
foos = []
constructor() : IFooLoader
{}
//etc...
} Which of these classes is easier to write/maintain? |
Another usecase of this feature that didn't seem to be mentioned above is specialisation. E.g. I want to be able to write this in TS defintions: declare class C<R> {
constructor(source: number): C<Uint8Array>;
constructor(source: R);
...
} and have it behave like normal function overloads. For now the only way to achieve this is via var+interface declarations, which is less pleasant / obvious than a direct syntax would be: interface C<R> {
...
}
declare const C: {
new(source: number): C<Uint8Array>;
new<R>(source: R): C<R>;
}; |
@vladrose I would not approve that code in code review. See: https://reactjs.org/docs/composition-vs-inheritance.html. But also your example doesn't make sense to me, unless there's something I'm not getting. Why are you accessing the return value of |
I just found that out the hard way: new Proxy( {}, {} ) instanceof Proxy; Hopefully, this proceeds and makes it into TS though, |
@00ff0000red I don't think you're commenting in the right place. Proxy is defined as a newable and thus is allowed to specify a return type, and indeed it does. Here is what I came up with for the problem you're describing. Feel free to paste that into a new issue. |
Note that TypeScript also allows This is because TypeScript makes all callables and newables have an implicit Line 298 in 5d6cce5
|
...just saying this completely saved my life. Not sure if it's lack of coffee or sleep but was way over thinking it...type assertion makes sense for the use case. |
There's a lot of "code smell" type comments here, advocating good over bad style, which is great when coaching newcomers. But Typescript is supposed to allow typing Javascript "as she is spoke", i.e. as the JS runs today, so TS should be able to express the types, regardless of whether there are better or worse JS ways of doing it. As far as my limited understanding of the TS project goals go, being able to express the types of JS in the wild is pretty high on the list. So we should talk about how to express this in TS code, not say "my code doesn't do that so you don't need to have TS type it correctly". Right? For my own use case I want to use a class returning a proxy to generically wrap a database entity schema object, where the return type gets its keys from the schema keys, but with different return signatures, trapped by the proxy. While this absolutely works 100%, TS cannot express this without a factory function or the weird type assertion workarounds above, which is just silly. My consumers should be able to go |
I would agree in general. But you need to remember that the resources of the TypeScript team are finite. How common this practice is and whether there are alternative more suitable approaches must be considered when evaluating the priority and effort needed for this functionality. |
I think the community could write a pull request, but we never got a thumbs up that what we're proposing is acceptable. |
In current TypeScript, there is an implicit type of
(Just wanted to add the above in case anyone coming from Google is wondering whether they can return the class in the constructor, ie for chaining.) |
I write a js code with .d.ts types and have a Redis client wrapped with Proxy. @RReverser helped a lot with that ts trick. |
I just ran into this issue where I need to proxy my class instances like this: interface ProxyFoo {
bar: string;
}
class Foo {
constructor() {
return new Proxy(this, ...) as Foo & ProxyFoo;
}
}
const foo = new Foo();
foo.bar; // Error: property "bar" does not exist on type "Foo" The "bar" property is definitely available via my Proxy. Typescript should acknowledge whatever is returned from the constructor. This is mostly an advanced feature to be used by library maintainers and is not very likely to trip up your average developer. |
+1 i really need it for constructor overload export class Some<isTrue> {
constructor(value: boolean): Some<true>
constructor(): Some<false>
constructor(value?: boolean) {}
} |
Search Terms
constructor return type
Suggestion
Add an ability to specify return type of a class constructor.
Right now the compiler produces the following error: "Type annotation cannot appear on a constructor declaration."
Use Cases
Consider this example:
Current behavior:
Suggested behavior:
Of course, there are several ways to achieve this result (like using private constructor with factory function, or using a type converter like this:
type AsInterface<T, C> = C extends new (...args: infer A) => T ? new (...args: A) => T : never
But I think that the proposed feature is more concise
Checklist
My suggestion meets these guidelines:
The text was updated successfully, but these errors were encountered: