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

Suggestion: Allow declaring class members within a constructor #766

Closed
nycdotnet opened this issue Sep 28, 2014 · 24 comments
Closed

Suggestion: Allow declaring class members within a constructor #766

nycdotnet opened this issue Sep 28, 2014 · 24 comments
Labels
Suggestion An idea for TypeScript Too Complex An issue which adding support for may be too complex for the value it adds

Comments

@nycdotnet
Copy link

Problem Statement: In TypeScript 1.1, there are two basic ways to initialize complex properties with strong typing via a parameterized class constructor. Both have disadvantages. I will illustrate this with a simple example that uses Knockout, but this issue applies to any other JavaScript library that implements complex objects via initializer functions.

"Method 1" is to instantiate the properties outside of the constructor using a throw-away value, and then to set the desired value inside the constructor (see RightTriangle1 below). This method has the advantage of being rather simple to program/maintain because TypeScript automatically infers the property's type. However, this has the potential to cause ripple effects or performance issues at runtime because the code to instantiate each property is run at least twice.

class RightTriangle1 {
    public height = ko.observable(0);
    public width = ko.observable(0);
    public hypotenuse = ko.computed(() => {
        console.log("hypotenuse 1 calculated");
        return Math.sqrt(this.height() * this.height() + this.width() * this.width());
    });
    constructor(height: number, width: number) {
        this.height(height);
        this.width(width);
    }
}
var rt1 = new RightTriangle1(3, 4);
console.log("hypotenuse 1: " + rt1.hypotenuse());  // 5

Knockout happens to have an advanced feature to defer execution of a computed, but that's beside the point. Other libraries may not support such a feature and it's just one more thing for the developer to keep track of.

"Method 2" is to declare the properties with an explicit type outside of the constructor, and then to instantiate them inside the constructor (see RightTriangle2 below). This method has the advantage of eliminating the runtime double-instantiation issue, but it also eliminates the convenience of having TypeScript infer the types.

Method 2 has the drawback of extra work if a type is ever changed, but much worse than that, it places the burden on the developer to figure out what the type of each member should be. For trivial members (string, number, etc.) or libraries you've written yourself, this is merely busy-work, however for complex types used by a JavaScript library it is sometimes difficult to determine unless the developer is quite familiar with the library's definition. Lastly, this is just the sort of syntax that would make a JavaScript developer who was new to TypeScript think, "This is exactly what I was afraid of! Look at all that extra code I'm forced to write that just gets thrown away in the end!"

class RightTriangle2 {
    public height : KnockoutObservable<number>;
    public width: KnockoutObservable<number>;
    public hypotenuse: KnockoutComputed<number>;
    constructor(height: number, width: number) {
        this.height = ko.observable(height);
        this.width = ko.observable(width);
        this.hypotenuse = ko.computed(() => {
            console.log("hypotenuse 2 calculated");
            return Math.sqrt(this.height() * this.height() + this.width() * this.width());
        });
    }
}
var rt2 = new RightTriangle2(6, 8);
console.log("hypotenuse 2: " + rt2.hypotenuse()); // 10

In summary of the problem, "Method 1" begs performance or logic problems, and "Method 2" is too wordy.

Proposal: "Method 3" Fairly early on in the public life of TypeScript, the team added the ability to indicate retained fields by placing visibility modifiers on constructor parameters. This feature is awesome, but it only helps with trivial properties. I would like to propose adding a new feature to TypeScript to allow this idiom within the body of a constructor as well. This would permit compiling the code in the RightTriangle3 example below. Note the public modifiers on height, width, and hypotenuse.

class RightTriangle3 {
    constructor(height: number, width: number) {
        public this.height = ko.observable(height);
        public this.width = ko.observable(width);
        public this.hypotenuse = ko.computed(() => {
            console.log("hypotenuse 3 calculated");
            return Math.sqrt(this.height() * this.height() + this.width() * this.width());
        });
    }
}
var rt3 = new RightTriangle3(9, 12);
console.log("hypotenuse 3: " + rt3.hypotenuse()); // 15

With this proposal:

  • There are no breaking changes; code that compiled in TypeScript 1.0 works without any modification in this proposed future version.
  • The RightTriangle3 class would have the identical JavaScript emit as RightTriangle2.
  • The RightTriangle3 TypeScript compile-time type interface would also be identical to RightTriangle2.
  • The developer is not required to separate the declaration of the property from its instantiation just to get strong typing when using a parameterized constructor.
  • The developer is not required to update the type of a property if the class interface ever changes - they can just update the initializer code.
  • The TypeScript code in RightTriangle3 is the most succinct of the three. RightTriangle1 is 12 lines long (besides being "wrong"), RightTriangle2 is 13, RightTriangle3 is just 10. Additional observable or computed properties in RightTriangle1 and RightTriangle2 add two lines of code each; with RightTriangle3 only one line of code is added each.
  • The TypeScript code in RightTriangle3 is still very close to the emitted JavaScript (and therefore unlikely to create future incompatibility issues).
  • The new syntax is also very similar to the existing TypeScript parameter property declaration shorthand.
  • The pattern demonstrated in RightTriangle3 enables a simple cut+paste migration path to implement a constructor (the developer would just have to paste in "this." on each line). With TypeScript 1.0, implementing a constructor is a lot more work. For example if converting RightTriangle1 to RightTriangle2, the developer must determine and explicitly type out each member declaration with its type before migrating the instantiation code.

Specifics:
Inside a class constructor function only, a visibility modifier (public, private, or protected) may modify a property on the instance variable this. If so, the statement is considered a declaration of that property on the class and the type is inferred according to the normal TypeScript rules. The following example class A is legal under this proposal:

class A {
    constructor(property3: boolean) {
        public this.property1;  //any
        private this.property2 : string;  //string
        protected this.property3 = property3;  //boolean
    }
}

The above code is identical to this TypeScript 1.1 code:

class A {
    public property1;
    private property2: string;
    constructor(protected property3: boolean) {
    }
}

Use of a visibility modifier is otherwise not permitted within the body of a constructor. For example, class B below would be a syntax error because property1 is not qualified as a property of this. This condition should cause a new emit-preventing type error along the lines of Visibility modifiers within a constructor may only modify properties of "this".

class B {
    constructor(val: string) {
        public property1 = val;
    }
}

Use of a visibility modifier is similarly not permitted within a normal function member. For example, class C below would be a syntax error, because in this proposal class properties can only be declared in this manner within the constructor. This condition should cause a new emit-preventing type error along the lines of Visibility modifier declarations are only legal within the parameter list and body of a class constructor.

class C {
    doSomething() {
        public this.property1: string = "test";
    }
}

Use of the static modifier is similarly not permitted within a normal function member or the constructor. For example, class D below would be a syntax error because under this proposal, static class properties can only be declared using the existing TypeScript 1.0 syntax (within the body of the class). This condition should cause a new emit-preventing type error along the lines of Shorthand visibility declarations may not be used on static properties.

class D {
    constructor(val: boolean) {
        public static property1: string = "test";
    }
}

What happens if a declaration occurs inside of a branch?
For the purpose of the TypeScript type system, any branching, looping, etc. inside the constructor should be ignored. Furthermore, the JavaScript should be emitted exactly as if the public, private, or protected modifier were not present. Under this proposal, TypeScript should consider class E below to be valid. At compile time, TypeScript will consider class E to have a property called description of type string (even though at runtime the initialization code would never execute).

class E {
    constructor() {
        if (1 === 0) {
            public this.description = "";
        }
    }
}

What happens if there is a duplicate declaration and the types match?
This should not prevent the emit. The second and successive duplicates should raise error TS2300: Duplicate identifier 'PROPERTY_NAME_HERE'. just like how existing duplicate declarations work.

What happens if there is a duplicate, but one is a subtype of the other, or even if the types do not match?
The same type resolution logic should be applied if the duplicates occurred as simple type declarations in the class body (outside the constructor). The second and successive duplicates should raise error TS2300: Duplicate identifier 'PROPERTY_NAME_HERE'.. This should not prevent the emit. This may incidentally result in downstream error TS2339: Property 'PROPERTY_NAME_HERE' does not exist on type 'THE_PROPERTY_TYPE'. wherever sub-properties of the property are used (just like in TypeScript 1.0).

In all other scenarios (such as with inheritance chains, subtypes, supertypes, assignability relationships, and generics), the same behavior as a class written like RightTriangle2 in TypeScript 1.0 should be used when evaluating what should happen with a class written like RightTriangle3 using the new shorthand syntax.

I believe that this syntax fits in nicely with both the goals and non-goals of TypeScript and I hope that you'll consider it or something similar. Thanks for reading this.

@basarat
Copy link
Contributor

basarat commented Sep 28, 2014

👍 Also provides an easy syntax to put members directly on this instead of prototype

@RyanCavanaugh
Copy link
Member

I would note that you can use parameter property syntax to get something very close to this already:

class Foo {

    // Actual overloads
    constructor(n: string);
    // Constructor impl + member fields
    constructor(n: string,
        public bar = 'thing',
        public baz = 200,
        private qua = []
    ) {
        // constructor body
    }
}

@RyanCavanaugh
Copy link
Member

You can reference it by name

class Foo {
    // Actual overloads
    constructor(n: string);
    // Constructor impl + member fields
    constructor(n: string,
        public bar = 'thing' + n
    ) {
        // constructor body
    }
}

@nycdotnet
Copy link
Author

Well, you learn something new every day. Thanks Ryan!

This code does work in TypeScript 1.0.

class RightTriangle4 {
    constructor(height: number, width: number)
    constructor(_height: number, _width: number,
        public height = ko.observable(_height),
        public width = ko.observable(_width),
        public hypotenuse = ko.computed(() => {
            console.log("hypotenuse 4 calculated");
            return Math.sqrt(height() * height() + width() * width());
            })
        ) {}
}
var rt4 = new RightTriangle4(12, 16);
console.log("hypotenuse 4: " + rt4.hypotenuse()); // 20

I am not sure that this syntax looks as nice as the suggestion, and something just screams "wrong!" about doing actual work inside a method signature, in particular arguments referencing each other, but this certainly works in TypeScript 1.0.

@nycdotnet
Copy link
Author

@RyanCavanaugh Thanks so much for the suggestion. After working with this a bit, I intend to revise the original post considering this as a third form that works, but is problematic. In this case, the main issue is that function arguments referencing each other and executing code inside a method signature toes the line of violating the "principle of least astonishment".

I know that this is not a priority for the team at this time, but would you consider a pull request for the original suggestion?

@danquirk
Copy link
Member

danquirk commented Oct 8, 2014

I see where you're coming from but ultimately this is adding complexity and syntax for a pattern that people are generally ok with in a variety of languages. With the 3rd proposal the compiler and users have to walk the constructor body to try to understand the public surface area of a type (trading brevity for clarity in some cases) and there needs to be some interaction with the existing parameter properties among other things. We just have a limited budget for syntax and other changes and the value here just isn't that high.

@danquirk danquirk closed this as completed Oct 8, 2014
@danquirk danquirk added Suggestion An idea for TypeScript Too Complex An issue which adding support for may be too complex for the value it adds Declined The issue was declined as something which matches the TypeScript vision labels Oct 8, 2014
@nycdotnet
Copy link
Author

@danquirk Thanks for the consideration.

@DanielRosenwasser
Copy link
Member

@danquirk, were you leaning towards reopening this issue?

@danquirk
Copy link
Member

danquirk commented Apr 6, 2015

@DanielRosenwasser not really. This doesn't solve the problem of 'my ES6 code is an error in TypeScript'. It still requires new syntax at which point I don't think it's adding enough value over the current syntax options once you've decided it's worth the effort to augment your code to satisfy a TypeScript rule. If we were actually solving the ES6 problem that'd be interesting (ex inferring from this.foo assignments in the constructor to implicitly infer the shape of your type) or else people might just lean towards the future ES class syntax like we propose now https://gist.github.com/jeffmo/054df782c05639da2adb

@nycdotnet
Copy link
Author

I still appreciate the consideration, Dan. I'm definitely interested to hear how the constructor signature short-hand snuck into the language if this isn't adding enough value for you. Do you regret adding that?

PS: This is not intended to be snarky just curious.

@RyanCavanaugh
Copy link
Member

FWIW class parameter properties have existed in TypeScript basically forever (i.e. before I joined the team which was well before we went public).

Given the historical context of how much the ES6 class design was moving around, I think it was the right decision. The proposed syntax is very similar (identical?) to the 'harmony' class proposal that was popular at the time (http://wiki.ecmascript.org/doku.php?id=harmony:classes#proposal_history). Had TypeScript implemented some sort of constructor-body semantics and then had ES6 land on the harmony proposal with slightly different semantics, that would have been a disaster. Constraining those keywords to parameter positions was a good trade-off for avoiding conflicting with a future ES6 standard while still adding a ton of convenience to the language.

@nycdotnet
Copy link
Author

This is really interesting. I would love to hear more details about how TypeScript really came about some day and the early days of the project. Sometime when you folks aren't busy (lol), that would be a pretty cool Channel 9 video or blog post (or .NET Rocks podcast or something).

In the meantime, I suppose that I have to admit that this proposal is entirely fixable via IDE refactoring tools, so perhaps I can switch to begging for "implement this" to be added as a Roslyn refactoring to undeclared properties on this in a constructor. I'll have to look into that once I get VS 2015 going and work with Bas to get it into atom-typescript.

@jaesung2061
Copy link

jaesung2061 commented May 26, 2016

Hey, I'm having a compiler error where it tells me property x does not exist on type {}.

export class Auth {
    constructor(private api: Api, private session = {}) {
        //
    }

    login(credentials: any) {
        return this.api.post('auth', credentials)
            .do((response) => {
                this.session = {
                    user:  response.user,
                    token:  response.token
                }
            });
    }
}

The compiler keeps giving me these errors:

ERROR in ./angular/app/shared/auth/auth.service.ts
(23,46): error TS2339: Property 'user' does not exist on type '{}'.

ERROR in ./angular/app/shared/auth/auth.service.ts
(24,47): error TS2339: Property 'token' does not exist on type '{}'.

I've tried many different variations, but all keep throwing the same error. For example:

private session: {user: any, token: string} = {user: {}, token: ''};
constructor(private api: Api) {
    //
}

What's the issue here?

@mhegazy
Copy link
Contributor

mhegazy commented May 26, 2016

@jaesung2061 this error is caused by the type of response. i do not know what is the definition of do to be able to tell you why you are getting this error. in general SO is a good place for such questions.

@jaesung2061
Copy link

@mhegazy You were right. I added an any type anotation to response and it fixed the issue. I was thinking it was the session object and never thought about the response object. Thanks.

@mmkal
Copy link

mmkal commented Mar 31, 2017

@RyanCavanaugh @danquirk this was labeled 'Too Complex' and declined in 2014. But this is still an issue and keeps popping up: #6373 #2606 #2393 #12613

Something very close to this looks like it went into 'Salsa': #4955

@danquirk I don't agree that this is just about brevity - it's about how painful it is upgrading an es6 project to TypeScript. Anecdotally, I've found this to be the cause of more than half of initial errors when upgrading projects. @nycdotnet still have any interest in this?

Any chance this making it into the language proper could get looked at again?

@nycdotnet
Copy link
Author

Wow 2 1/2 years ago - amazing how time flies.

I still feel that taking maximum advantage of type inference is the correct way to use TypeScript. I still feel that doing work inside a method signature is icky (as in RightTriangle4, above). Since the time I wrote this, ES6 has become mainstream - not in small part due to the efforts of the TypeScript team. As far as I am aware, the syntax I proposed does not collide with ES6 or any likely future official ECMAScript syntax. I agree with you, @mmkal, that having to provide type definitions is a tedious and non-value-added task that hits you hard in the face on day 1 of using TypeScript (assuming not using --allowJs).

So in the end, I feel that this proposal is still sound and would be a welcome addition to the language, HOWEVER - it is also something that could be implemented by tooling in an editor (which would produce RightTriangle2).

This proposal was from the days before VS Code and when atom-typescript was ~ v0.5, so writing that tooling would have also been the job of the TypeScript team then. Now, we're approaching the time when it's plausible a community member could do it for either of those two editors, but it remains that it does not exist. It would be awesome for it to exist - either as part of the language or as a tool.

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Apr 1, 2017

@nycdotnet

I still feel that taking maximum advantage of type inference is the correct way to use TypeScript.

I strongly agree with this.
Every day I see code like

class MyComponent {
  names: Array<string> = ['hello', 'world']; // redundant
}

or worse

class MyComponent {
  names: any = ['hello', 'world']; // depressing.
}

It is amazing how the poorly coded official tutorials of one popular framework can have such a negative impact on the use of a language.

However, looking over your original proposal, and I might be missing something here, it seems that there would be no new syntax required to implement it. It would simply imply tracking assignments to this altering the shape of the class along the way.

What I mean is that this

class RightTriangle3 {
    constructor(height: number, width: number) {
        public this.height = ko.observable(height);
        public this.width = ko.observable(width);
        public this.hypotenuse = ko.computed(() => {
            console.log("hypotenuse 3 calculated");
            return Math.sqrt(this.height() * this.height() + this.width() * this.width());
        });
    }
}
var rt3 = new RightTriangle3(9, 12);
console.log("hypotenuse 3: " + rt3.hypotenuse()); // 15

could just as well be this

class RightTriangle3 {
    constructor(height: number, width: number) {
        this.height = ko.observable(height);
        this.width = ko.observable(width);
        this.hypotenuse = ko.computed(() => {
            console.log("hypotenuse 3 calculated");
            return Math.sqrt(this.height() * this.height() + this.width() * this.width());
        });
    }
}
var rt3 = new RightTriangle3(9, 12);
console.log("hypotenuse 3: " + rt3.hypotenuse()); // 15

The effect on the JS code would be identical, in fact that is a valid JavaScript constructor body.

The Salsa language service exhibits this behavior.

@nycdotnet
Copy link
Author

I get why they don't allow that by default. Allowing implicit declarations - even just in constructors - opens the door to classes of bugs that TypeScript is intended to prevent. This proposed syntax was a way to make such declarations explicit. Perhaps an --allowImplicitMemberDeclarations switch (to mimic Salsa) is a better solution, however that would continue to add to the present constellation of switches, which is another day 1 problem (can't blame TS team for this one, though).

@aluanhaddad
Copy link
Contributor

I understand the desire to be explicit but wanted to point out that it doesn't require new syntax. New syntax in value position is a showstopper.

@nycdotnet
Copy link
Author

fair enough.

@RyanCavanaugh RyanCavanaugh added In Discussion Not yet reached consensus and removed Declined The issue was declined as something which matches the TypeScript vision Too Complex An issue which adding support for may be too complex for the value it adds labels Apr 3, 2017
@RyanCavanaugh
Copy link
Member

I think this deserves another go-around. We infer from this assignments in JS and considered doing this for TS classes with zero local property declarations. But without a global quickfix, I think we're potentially making class-based ES6 -> TS migration too difficult (ironically)

@RyanCavanaugh RyanCavanaugh reopened this Apr 3, 2017
@RyanCavanaugh RyanCavanaugh added Too Complex An issue which adding support for may be too complex for the value it adds and removed In Discussion Not yet reached consensus labels Apr 24, 2017
@RyanCavanaugh
Copy link
Member

Thought about it again but outcome is basically the same ( #766 (comment) ).

Particularly, ES-Next public field initializers are coming down the pipe, so constructor-initialized properties should be getting less common at some point in the future.

@nycdotnet
Copy link
Author

Thank you for reconsidering it.

@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Suggestion An idea for TypeScript Too Complex An issue which adding support for may be too complex for the value it adds
Projects
None yet
Development

No branches or pull requests

9 participants