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

Cannot Optionalize Class Getters #14417

Open
bradenhs opened this issue Mar 2, 2017 · 32 comments
Open

Cannot Optionalize Class Getters #14417

bradenhs opened this issue Mar 2, 2017 · 32 comments
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Milestone

Comments

@bradenhs
Copy link

bradenhs commented Mar 2, 2017

TypeScript Version: 2.2.1

Code

class Person {
  firstName: string;
  lastName: string;

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

Expected behavior:

Since fullName is optional the following should work:

const Me: Person = {
  firstName: 'Foo',
  lastName: 'Bar'
}

Actual behavior:

A syntax error occurs on this line get fullName?() {

I'm not sure if omitting the ability to optionalize getters was by design or not. If it was by design, I'd love to know the reasoning behind it.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 2, 2017

A class is an implementation of a type. if you want to declare an optional property i suggest adding an interface Person:

interface Person {
   firstName: string;
   lastName: string;

  readonly fullName?;
}

@mhegazy mhegazy added the Working as Intended The behavior described is the intended behavior; this is not a bug label Mar 2, 2017
@bradenhs
Copy link
Author

bradenhs commented Mar 2, 2017

Thanks! I'm aware that that's a possibility.

I suppose I just don't understand why the language would disallow marking getters as optional when everything else on a class can be marked as optional. It seems inconsistent. Is there a good reason for this inconsistency? Forgive me if I misunderstand, but the "A class is an implementation of a type" reasoning seems more like a how-the-compiler-is-implemented reason as opposed to a how-the-language-should-work reason. I'm interested in the later.

@mhegazy
Copy link
Contributor

mhegazy commented Mar 2, 2017

Accessors (getters/setters) are just properties. you can declare methods and properties as option. but you can not have an implementation to an optional property or a method.

@bradenhs
Copy link
Author

bradenhs commented Mar 2, 2017

Maybe I misunderstand. You can specify an implementation to an optional class method:

class Person {
  getName?() {
    return 'This is the name';
  }
}

There are no errors with that.

@Venryx
Copy link

Venryx commented Mar 16, 2017

@mhegazy Like bradenhs said, class methods can be made optional, as shown in this merged PR: #8625

class Bar {
    h?() {
        return 2;
    }
}

As can be seen, it's an optional method that also has a body/implementation specified.

That's the same thing we'd like for class getters/setters, like so:

class Bar {
    get h?() {
        return 2;
    }
}

However, this doesn't compile currently, which seems inconsistent when class methods work with it fine.

@RyanCavanaugh RyanCavanaugh removed the Working as Intended The behavior described is the intended behavior; this is not a bug label Mar 16, 2017
@RyanCavanaugh
Copy link
Member

RyanCavanaugh commented Mar 16, 2017

@mhegazy seems inconsistent to allow (bodied) optional methods but not getters. Thoughts?

@mhegazy mhegazy added Suggestion An idea for TypeScript In Discussion Not yet reached consensus labels Mar 16, 2017
@bradenhs
Copy link
Author

If this suggestion is accepted would you guys take a PR for it? (I don't know much about the TypeScript code base but this doesn't seem like it would be a complicated task).

@bradenhs
Copy link
Author

@ahejlsberg I saw that you implemented the ability to optionalize class properties in #8625. Is there a specific reason the ability to optionalize getters was omitted from that PR?

@RyanCavanaugh RyanCavanaugh added Help Wanted You can do this and removed In Discussion Not yet reached consensus labels Apr 24, 2017
@RyanCavanaugh RyanCavanaugh added this to the Community milestone Apr 24, 2017
@RyanCavanaugh
Copy link
Member

Accepting PRs to allow this with similar semantics as optional methods.

@bradenhs
Copy link
Author

bradenhs commented Apr 25, 2017

I think I'll take a stab at this. It would allow for some better patterns in the state management library I'm creating for my CS498R class (nearly graduated!)

@bradenhs
Copy link
Author

bradenhs commented Jun 8, 2017

Finally had some time to go ahead and implement this. Take a look at the PR here #16344.

@hmdhk
Copy link

hmdhk commented Jan 28, 2018

Any updates on this? Can we get the PR merged?

@bradenhs
Copy link
Author

@jahtalab It's been a while and I need to update the PR. I'm taking a couple "hobby programming" days off from work this next month so I may get to it then.

@karol-depka
Copy link

karol-depka commented Jun 16, 2018

To anyone who is skimming through this conversation and wonders why last comment was as much time ago as 29. Jan: the more up-to-date conversation (up to 26 days ago) is on #16344
Interesting history behind this PR, btw :). Is this situation common?

@RyanCavanaugh RyanCavanaugh modified the milestones: Community, Backlog Mar 7, 2019
@inad9300
Copy link

inad9300 commented Apr 12, 2019

Related to this issue, I wish to propose that setters allow optional arguments, as it seems inconsistent when compared to regular methods, e.g.

class C {
    setX(n?: number) {} // Allowed.
    set x(n?: number) {} // Not allowed: "A 'set' accessor cannot have an optional parameter."
}

The main problem being, you cannot allow passing undefined or null to such a setter.

@RyanCavanaugh
Copy link
Member

@inad9300 see #2521

@thw0rted
Copy link

I'm not sure @inad9300 's question is actually about #2521. set x(n: number | undefined) { } is valid and probably what they meant to specify. It's not just that setters can't have optional parameters, it's that setters must always have exactly one parameter (and its type must be the same as the getter, if specified). They also can't have two or ten parameters.

If you think about it for a second, though, it makes sense. The setter is called when an assignment operator is encountered, but a syntactically valid assignment always has exactly one l-value and one r-value, even if that r-value is undefined. Thus, you can't ever call the setter function with anything other than exactly one argument, so it doesn't make sense to support declaring it with anything other than exactly one parameter.

@ahmadalibaloch
Copy link

ahmadalibaloch commented Dec 14, 2019

any update?
I am tired of writing entity.id(), i want entity.id but then creating the entity without id is not possible
const entity = new Entity({name:'xyz}:Entity) // property id is missing

This should be allowed, which is only possible when id can be optional property.

@thw0rted
Copy link

You don't need this to solve your specific use case. You could just declare your class with constructor(other: {name: string}) or constructor(other: Partial<Entity>) or even constructor(other: Omit<Entity, "id">).

@xtianus79
Copy link

@RyanCavanaugh is there any update on this issue? it has literally been years now.

@thw0rted
Copy link

thw0rted commented May 7, 2020

I didn't even notice when I made my previous comments that there are actually at least two issues in this ticket. One, can I declare a class whose getters are optional? (I still don't understand the actual use case here -- if you don't new a class instance from an existing implementation, instanceof doesn't work.) Two, why can't I make the argument to a setter optional? (Setters are only called when doing assignments, and there's always exactly one r-value, even if it's explicit null or undefined.)

@xtianus79 are you trying to get an update on "issue one"? If so, the linked PR #16344 has some back-and-forth but looks like it needs some extra discussion before it gets resolved.

@desmap
Copy link

desmap commented Jun 5, 2020

@thw0rted use case could be to not have to use them in call signatures eg when using classes as interfaces

@thw0rted
Copy link

thw0rted commented Jun 5, 2020

I hadn't thought about the "classes as interfaces" thing because I don't actually do that myself. Are there particular times when it makes more sense than just writing an explicit interface definition? If you're making methods (and getters) optional entirely so that you can use the class as an interface, wouldn't it mean that reads always need conditional access even though you know that, for actual instances of the class, they're always defined?

@mattjanssen
Copy link

mattjanssen commented Nov 23, 2020

Oof. I just ran into this myself. Funny thing is the IDE (PhpStorm) doesn't see get foo?() as a problem 😵

For now I did get foo(): string|null which compiles but doesn't seem to be recognized by the IDE. Oh well.

@david-a
Copy link

david-a commented Jan 6, 2021

would love to see get fullName?() { implemented as well 👍 (or another solution for getters)

@youra-h
Copy link

youra-h commented Jan 10, 2021

would love to see get fullName?() { implemented as well 👍 (or another solution for getters)

Not done since 2017 😵

@carlosalaniz
Copy link

I support this.

would love to see get fullName?() { implemented as well 👍 (or another solution for getters)

@iamchathu
Copy link

Seems this a real issue and still not implemented.

@jonlepage
Copy link

jonlepage commented May 2, 2021

this is so hellish to provide custom api !
We work a lot with getter setter and it adding so complicate thing with ts!

My solution is using Implement with interface , but is make thing complicate.

class Plugin {_uid:string}

interface IPerson {
	plugin?:Partial<Plugin>;
}
class Person implements IPerson {
	static create(param:IPerson){return new Person}

	private __PluginUUID: string = null; 
	get Plugin(): Plugin {
		return findPlugin(this.__PluginUUID)
	}
	set Plugin(data: Plugin) {
		this.__PluginUUID = data._uid;
	}
	set plugin(data: Partial<Plugin>) {
		this.Plugin = new Plugin(data);
	}
}

const foo = Person.create({
	plugin:{_uid:''},
})

@weswigham
Copy link
Member

See this comment for details, but modern class field behavior makes optionalizing getters and setters kind of a tricky territory, since checking for their existence is very awkward. While, relationally, we can probably make up rules for working with them, making them useful in control flow and the like seems much harder; given there's very little apparent demand for it and they seem very awkward to use, maybe we'll just continue to not allow them for now - at least until a better way to handle them in control flow and type guarding becomes apparent (because common assignment patterns potentially throwing would be super annoying).

@bombillazo
Copy link

This feature is sorely needed :/ It's a one-off case that can't even be solved by bypassing this compile error with an option or something.

@SpencerKaiser
Copy link

Just stumbled into this and I'm super disappointed this functionality doesn't exist... it creates some really JavaScript-y errors if you aren't super intentional about checking your potentially undefined getter values ☹️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Discussion Not yet reached consensus Suggestion An idea for TypeScript
Projects
None yet