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 get/set accessors to be of different types #2521

Closed
irakliy81 opened this issue Mar 26, 2015 · 163 comments
Closed

Suggestion: allow get/set accessors to be of different types #2521

irakliy81 opened this issue Mar 26, 2015 · 163 comments
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript

Comments

@irakliy81
Copy link

It would be great if there was a way to relax the current constraint of requiring get/set accessors to have the same type. this would be helpful in a situation like this:

class MyClass {

    private _myDate: moment.Moment;

    get myDate(): moment.Moment {
        return this._myDate;
    }

    set myDate(value: Date | moment.Moment) {
        this._myDate = moment(value);
    }
}

Currently, this does not seems to be possible, and I have to resort to something like this:

class MyClass {

    private _myDate: moment.Moment;

    get myDate(): moment.Moment {
        return this._myDate;
    }

    set myDate(value: moment.Moment) {
        assert.fail('Setter for myDate is not available. Please use: setMyDate() instead');
    }

    setMyDate(value: Date | moment.Moment) {
        this._myDate = moment(value);
    }
}

This is far from ideal, and the code would be much cleaner if different types would be allowed.

Thanks!

@danquirk
Copy link
Member

I can see how this would be nice here (and this has been requested before although I can't find the issue now) but it's not possible whether or not the utility is enough to warrant it. The problem is that accessors are not surfaced in .d.ts any differently than normal properties since they appear the same from that perspective. Meaning there's no differentiation between the getter and setter so there's no way to a) require an implementation use an accessor rather than a single instance member and b) specify the difference in types between the getter and setter.

@danquirk danquirk added the By Design Deprecated - use "Working as Intended" or "Design Limitation" instead label Mar 26, 2015
@irakliy81
Copy link
Author

Thank you for the quick reply, Dan. I'll follow the less elegant way. Thanks for the great work!

@tkrotoff
Copy link

A JavaScript getter and setter with different types is perfectly valid and works great and I believe it is this feature's main advantage/purpose. Having to provide a setMyDate() just to please TypeScript ruins it.

Think also about pure JS libraries that will follow this pattern: the .d.ts will have to expose an union or any.

The problem is that accessors are not surfaced in .d.ts any differently than normal properties

Then this limitation should be fixed and this issue should stay open:

// MyClass.d.ts

// Instead of generating:
declare class MyClass {
  myDate: moment.Moment;
}

// Should generate:
declare class MyClass {
  get myDate(): moment.Moment;
  set myDate(value: Date | moment.Moment);
}

// Or shorter syntax:
declare class MyClass {
  myDate: (get: moment.Moment, set: Date | moment.Moment);
  // and 'fooBar: string' being a shorthand for 'fooBar: (get: string, set: string)'
}

@RyanCavanaugh
Copy link
Member

I realize this is just an opinion, but writing a setter such that a.x === y is not true immediately after a.x = y; is a giant red flag. How do consumers of a library like that know which properties have magic side effects and which don't?

@tkrotoff
Copy link

How do [you] know which properties have magic side effects and which don't?

In JavaScript it can be un-intuitive, in TypeScript the tools (IDE + compiler) will complain. Why do we love TypeScript again? :)

@kitsonk
Copy link
Contributor

kitsonk commented Jun 11, 2015

A JavaScript getter and setter with different types is perfectly valid and works great and I believe it is this feature's main advantage/purpose.

This is arguing that JavaScript is weakly typed, so TypeScript should be weakly typed. :-S

@tkrotoff
Copy link

This is arguing that JavaScript is weakly typed, so TypeScript should be weakly typed

C# allows it and that does not make this language weakly typed C# does not allow get/set of different types.

@kitsonk
Copy link
Contributor

kitsonk commented Jun 12, 2015

C# allows it and that does not make this language weakly typed C# does not allow get/set of different types.

😉

@paulwalker
Copy link

No one is arguing (as far as I can see) that accessors should be weakly typed, they are arguing that we should have the flexibility to define the type(s).

Often it is needed to copy a plain old object to object instances.

    get fields(): Field[] {
      return this._fields;
    }

    set fields(value: any[]) {
      this._fields = value.map(Field.fromJson);
    }

That is much nicer than the alternative and allows my setter to encapsulate the exact type of logic setters are made for.

@danquirk
Copy link
Member

@paulwalker the pattern you use there (a setter taking an any and a getter returning a more specific type) is valid today.

@paulwalker
Copy link

@danquirk Great to know, thank you! It looks like I just needed to update my IDE plugin compiler for ST.

@sccolbert
Copy link

@jasongrout
Copy link

I just tested with typescript@next (Version 1.8.0-dev.20151102), and also have an error.

~$ tsc --version
message TS6029: Version 1.8.0-dev.20151102
~$ cat a.ts
class A {
    get something(): number {return 5;}
    set something(x: any) {}
}

~$ tsc -t es5 a.ts
a.ts(2,2): error TS2380: 'get' and 'set' accessor must have the same type.
a.ts(3,2): error TS2380: 'get' and 'set' accessor must have the same type.

@paulwalker
Copy link

Ironically, after updating my Sublime linter, it no longer threw an error, which is using TypeScript 1.7.x. I've been assuming it is a forthcoming enhancement in 1.7+, so perhaps 1.8.0 regressed.

@christianacca
Copy link

Even with the version of visual studio code (0.10.5 (December 2015)) that supports typescript 1.7.5 and with typescript 1.7.5 installed globally on my machine this is still a problem:

image

So what version this will be supported?
Thanks

@RyanCavanaugh
Copy link
Member

I think Dan was mistaken. The getter and the setter must be of identical type.

@christianacca
Copy link

shame. would have been a nice feature for when writing page objects for use in protractor tests.

I would have been able to write a protractor test:

po.email = "c.c@gmail.com";
expect(po.email).toBe("c.c@gmail.com");

... by authoring a page object:

class PageObject {
    get email(): webdriver.promise.Promise<string> {
        return element(by.model("ctrl.user.email")).getAttribute("value")
    }
    set email(value: any) {
        element(by.model("ctrl.user.email")).clear().sendKeys(value);
    }
}

@RyanCavanaugh
Copy link
Member

What about that code requires the setter to be of type any ?

@christianacca
Copy link

The getter will return a webdriver.promise.Promise<string> yet the setter I want to assign a string value.

Maybe the following longer form of the protractor test makes it clearer:

po.email = "c.c@gmail.com";
var currentEmail : webdriver.promise.Promise<string> = po.email;
expect(currentEmail).toBe("c.c@gmail.com")

@Arnavion
Copy link
Contributor

@RyanCavanaugh With the introduction of null annotations, this prevents code that allows calling a setter with null to set it to some default value.

class Style {
    private _width: number = 5;

    // `: number | null` encumbers callers with unnecessary `!`
    get width(): number {
        return this._width;
    }

    // `: number` prevents callers from passing in null
    set width(newWidth: number | null) {
        if (newWidth === null) {
            this._width = 5;
        }
        else {
            this._width = newWidth;
        }
    }
}

Could you consider atleast allowing the types to differ in the presence of | null and | undefined ?

@MaklaCof
Copy link

MaklaCof commented Aug 9, 2016

This would really be a nice feature.

@artyil
Copy link

artyil commented Sep 18, 2016

So will this be a feature?

@kitsonk
Copy link
Contributor

kitsonk commented Sep 19, 2016

@artyil it is closed and tagged By Design which indicates that currently there is not any plans to add it. If you have a compelling use case which you think overrides the concerns expressed above, you can feel free to make your case and additional feedback may make the core team reconsider their position.

@paulwalker
Copy link

@kitsonk I think more than enough compelling use cases have been provided in the above comments. While the current design follows a common pattern of most other typed languages with this restriction, it is unnecessary and overly restrictive in the context of Javascript. While it is by design, the design is wrong.

@MaklaCof
Copy link

I agree.

@mhegazy mhegazy reopened this Sep 19, 2016
@mhegazy mhegazy removed the By Design Deprecated - use "Working as Intended" or "Design Limitation" instead label Sep 19, 2016
RyanCavanaugh added a commit to RyanCavanaugh/TypeScript that referenced this issue Mar 24, 2021
@somebody1234
Copy link

Honestly, I really try to refrain from being prescriptive here about how to write code, but I truly object to the idea that this code

foo.bar = "hello";
console.log(foo.bar);

should ever print anything than "hello" in a language that attempts to have sane semantics. Properties should have behavior that is indistinguishable from fields to external observers.

While this may be a valid point, in reality this is not the case.
Or more like, sure, it shouldn't, but there's a difference between "we don't recommend this but sure" and "we don't recommend this so just don't do it"

@ricmoo
Copy link

ricmoo commented Mar 25, 2021

This is a common pattern in DOM though, no? foo.innerText = 5 and then a console.log(foo.innerText)? It allows an object to do its own coercion on its properties. I want to be able to assign an object to a req.headers, and have it converted to a Headers object.

@somebody1234
Copy link

somebody1234 commented Mar 25, 2021

Indeed, this (dom setters converting types) been mentioned above multiple times I believe

@somebody1234
Copy link

@shicks re: narrowing...
I feel like it should definitely only narrow if it's a subtype.
If it's merely overlapping, I feel like you can't really be sure that the get is returning one of those types (since it'd probably rely on an earlier set to enter a certain type of value?)
You'd be able to use as anyway, not an ideal solution but without examples I'm not sure it's good default behavior

@RyanCavanaugh
Copy link
Member

There's a Playground build for PR 42425 available for trying out and I'd appreciate any feedback on how it meets folks' needs here. Thanks! See comments in #42425 on how this works.

@thw0rted
Copy link

Ryan, is the team planning to refine the DOM defs (in lib.dom.d.ts) if your PR goes through? Is that even possible using the current build process? It'd be a breaking change, but only for code that isn't technically correct (the best kind of correct).

@RyanCavanaugh
Copy link
Member

We'll take feedback on the DOM if this goes in. I think we'd still be keeping a pretty tight bar on what's allowed there; most of the DOM has coercing setters where the coercion is in the vein of similarly-disallowed coercions like "5" * "11". More "obvious" coercions like string -> URL seem like the kind of thing we'd be open to.

@hopeless-programmer-online
Copy link

Any updates?

In the following code example:

class vec2 {
  x : number
  y : number

  dot(other : vec2) : number
}
class vec3 {
  get xy() : vec2
  set xy(xy : { x : number, y : number })
}

we are forced to return only { x : number, y : number } on get x() or to accept only vec2 on set x().
While the ideal approach would be to get fully operable vec2 with all its methods and to set anything that has x and y.

@thw0rted
Copy link

thw0rted commented Apr 16, 2021

In #42425, Ryan said "First, the type of the getter must be assignable to the type of the setter. In other words, the assignment obj.x = obj.x; must be legal assuming obj.x is writable at all.... These prevent novel unsoundness from occurring and makes this feature unimpactful from a type relational perspective, which limits its risk." It sounds like the incoming feature will not support your use case.

That said, look immediately above your post: #43662 was just opened, which points out that "[Element#style] is one where you can’t just make the set type string | CSSStyleDeclaration because assigning a CSSStyleDeclaration back to the accessor doesn’t work". Maybe follow / upvote that issue and watch for more discussion there?

@pckhoi
Copy link

pckhoi commented Apr 20, 2021

There is no reason to think that separately typed getter/setter should not be used in a greenfield project. JavaScript isn't statically typed and it has its own benefits: more terse and expressive syntax. Not having to remember 200 different method names that do a single thing with slightly different input is a great thing! This is even more powerful when taking into account new ES features such as proxy. This is just very valuable to library author.

While we're at it how about adding differently typed getter/setter to interface also? E.g.

interface SeriesGetter {
  get [prop:string]: Series
  set [prop:string]: number | string | null | undefined | Series
}

Only reason why I want this on interface is because this is the only way for proxy to work in typescript.

@shicks
Copy link
Contributor

shicks commented Apr 20, 2021

In the following code example:

class vec2 {
  x : number
  y : number

  dot(other : vec2) : number
}
class vec3 {
  get xy() : vec2
  set xy(xy : { x : number, y : number })
}

we are forced to return only { x : number, y : number } on get x() or to accept only vec2 on set x().
While the ideal approach would be to get fully operable vec2 with all its methods and to set anything that has x and y.

This already works in the proposed PR. See this example.

While we're at it how about adding differently typed getter/setter to interface also? E.g.

interface SeriesGetter {
  get [prop:string]: Series
  set [prop:string]: number | string | null | undefined | Series
}

Only reason why I want this on interface is because this is the only way for proxy to work in typescript.

To be pedantic, it does work on interfaces (see example). What doesn't work is index signatures, which honestly I'm okay with, since it's impossible to write such a getter/setter pair without proxies, and by the time you're talking about proxies, static typing is a bit of a stretch.

@pckhoi
Copy link

pckhoi commented Apr 20, 2021

Hey there are value in slapping some typing info onto proxies. Why do we need to be extreme and say either proxy or static typing?

@mike-marcacci
Copy link

I am extremely interested in this because it is a prerequisite for my proposal to fix the major soundness issue with index signatures in a way that has a fairly well-considered migration path (if I can say so myself).

Of course, tracking get/set separately does raise some new questions, especially around type refinement. For example:

type SomeMap = {
  get [key: string](): string | undefined;
  set [key: string](string): string;
}

function(map: SomeMap) {
  map.alpha = "beta";
  const { alpha } = map;
  // Here I would expect alpha to be refined to "beta" because
  // we are treating the object as a "map". This follows current
  // refinement rules, and is what I'm interested in.
}

type Foo =  {
  get alpha(): string | number,
  set alpha(value: string | boolean): string | boolean
}

function(foo: Foo) {
  foo.alpha = true;
  const { alpha } = foo;
  // What value is foo? We can't really refine our type here...
}

While the former scenario is almost certainly the biggest use-case here (and the one I care about), keeping the desired logic breaks the latter; supporting the second case severely limits usefulness of the former without some additional mechanism for describing when these the compiler can make these refinements.

I think perhaps some additional mechanism for this needs to be drawn up.

@thw0rted
Copy link

Mike, you should make a playground example to illustrate what you think is going on. You say that you'd expect const {alpha} to be typed as "beta" (literal) following current refinement rules, but it actually narrows to string.

From that same Playground, even without the new feature you wind up with unsound / incorrect assumptions about property accessors. This is of course dumb:

class Bar {
  private _x: string | number = 1;
  get x(): string | number {
    return this._x;
  }
  set x(val: string | number) {
    if (Number.isInteger(val)) {
      this._x = `${val}`;
    } else {
      this._x = Number(val);
    }
  }
}

const b = new Bar();

b.x = 5
const x1 = b.x; // number
console.log(`number? no, ${typeof x1}`); // logs "string"

b.x = "3";
const x2 = b.x; // string
console.log(`string? no, ${typeof x2}`); // logs "number"

but I'm sure there are more reasonable real-world use cases.

@pckhoi
Copy link

pckhoi commented Apr 20, 2021

I would be happy enough if type refinement simply follow to the letters of what the declaration describe for now. map.alpha can just be string | undefined, foo.alpha can just be string | number.

Maybe extra fine type refinement can be declared somehow in the future but I think it's out of this issue's scope.

@SrBrahma
Copy link

@RyanCavanaugh Close this? #42425

@RyanCavanaugh
Copy link
Member

Yep. Folks can track #43662 for possible loosening of the get/set relation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awaiting More Feedback This means we'd like to hear from more people who would be helped by this feature Suggestion An idea for TypeScript
Projects
Design Meeting Docket
  
Awaiting triage
Development

No branches or pull requests