Skip to content

Adds ability to optionalize class getters #16344

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

Closed
wants to merge 3 commits into from

Conversation

bradenhs
Copy link

@bradenhs bradenhs commented Jun 8, 2017

Fixes #14417

This PR makes it to possible to optionalize class getters in the same way class methods can be marked optional:

class Person {
    firstName: string;
    lastName: string;

    get fullName?() {
        return this.firstName + " " + this.lastName;
    }
}

const me: Person = {
    firstName: "foo",
    lastName: "bar"
}

The above code should now work whereas before there was a syntax error. This is my first TypeScript PR so I hope I covered everything. This PR should handle:

  • Updating the declaration emit
  • Ensuring strictNullChecks works as expected with optional class getters
  • Ability to omit optional class getter body

Hopefully I didn't miss anything, but I am of course more than happy to fix any issues.

@DanielRosenwasser
Copy link
Member

Hey @bradenhs thanks for the PR! One thing that I'd like to ask is whether you could break your tests into multiple files. It seems like there's a lot of stuff you're trying to test (including intentional syntax errors), so I think it'd be a good idea to break that into multiple granular tests.

@bradenhs
Copy link
Author

No problem. I'm traveling now but should have some time within the next week or so to make that change. I would love any other feedback in the meantime.

@microsoft microsoft deleted a comment from msftclas Sep 27, 2017
@Dashue
Copy link

Dashue commented Oct 4, 2017

Would love to see this functionality as well, any updates?

@bradenhs
Copy link
Author

bradenhs commented Oct 4, 2017

Started first post-college job this summer. Don't have as much coding free time as I once had :) But I have plans to get back to this again before the year is out.

@jeanpfs
Copy link

jeanpfs commented May 17, 2018

Any updates?

@mhegazy mhegazy requested a review from a user May 17, 2018 16:15
@mhegazy
Copy link
Contributor

mhegazy commented May 17, 2018

@Andy-MS and @weswigham can you please review

@mhegazy mhegazy requested a review from weswigham May 17, 2018 16:16
@bradenhs
Copy link
Author

So I've been putting off crossing the finish line with this for a while but would definitely love any additional reviews! It might give me the motivation to finish this up.

@ghost
Copy link

ghost commented May 17, 2018

Why only get? Seems like set should be allowed to be optional as well.

@bradenhs
Copy link
Author

So bear with me as I try to remember my thinking on this. I believe there are two reasonable choices for how to implement this:

  1. Whenever get is optional set must be marked optional as well. set cannot be marked optional if get is not marked optional.
  2. Only allow marking get as optional. set can never be marked optional.

I went with the second of those choices because marking set as optional seemed weird. I wasn't sure what set someProperty?(value) { ... really communicates. The only advantage of option 1 in my eyes is that if someone sees that set is optional they'll know for sure that the corresponding get is optional as well. To be honest, I don't have a strong opinion on this either way.

@ghost
Copy link

ghost commented May 17, 2018

If I understand the PR correctly, its effect is that when a class is used as an interface, the implementer doesn't have to provide a getter. So, what if you have:

class C {
    get x?() { return 0; }
    set x(value: number) { ... }
}
const impl: C = { }; // Should error?

Shouldn't I need both the getter and setter to be optional for compilation to succeed? After all, the following is a compile error today:

class C {
    set x(value: number) { }
}
const impl: C = {}; // Error, needs a setter for 'x'

@bradenhs
Copy link
Author

FYI: I probably won't get to this within the next couple weeks. I still want to finish it up but it'll be slow going.

@bradenhs
Copy link
Author

To anyone who wants to take this PR over feel free to do so. I'd still like to finish it up but evidently it's not a priority because I'm not making time for it :) I may get to it eventually but don't want to keep someone else from tackling this if they're interested!

@RyanCavanaugh
Copy link
Member

@bradenhs thanks for the work so far! Will let someone else pick this up if they're able to.

@ericcarino
Copy link

Thanks for the work @bradenhs . It sucks that no one has the time to finish this off (like myself). This feature would have come in handy for me.

@bigzoo
Copy link

bigzoo commented Feb 10, 2020

@RyanCavanaugh could you put down what needs to be added to get this work completed? Willing to take it up

@dapug
Copy link

dapug commented Oct 14, 2021

How can this possibly have been abandoned? Seems absolutely fundamental, essential. What is the work-around?

@artem-v-shamsutdinov
Copy link

Here is what I did to get around this:
class Example {

constructor() {
delete this.val
Object.defineProperty(this, 'val', {
get() {
return whatever
},
set(
val: string
) {
// do whatever
}
});
}

val?: string
}

@DougSPol
Copy link

Ping - too bad this wasn't merged, super want this ability

@abhinandan-chakraborty
Copy link

when will this get solved?

@pishangujeniya
Copy link

Don't know why this was not prioritized. 😞

@MnariMohamed
Copy link

2024 and this is still not fixed

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.

Cannot Optionalize Class Getters