microsoft / TypeScript

TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
https://www.typescriptlang.org
Apache License 2.0
100.66k stars 12.44k forks source link

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

Closed irakliy81 closed 3 years ago

irakliy81 commented 9 years ago

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 commented 9 years ago

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.

irakliy81 commented 9 years ago

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

tkrotoff commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

:wink:

paulwalker commented 8 years ago

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 commented 8 years ago

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

paulwalker commented 8 years ago

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

sccolbert commented 8 years ago

@danquirk That doesn't seem to work according to the playground (or version 1.6.2): http://www.typescriptlang.org/Playground#src=%0A%0Aclass%20Foo%20%7B%0A%0A%20%20get%20items()%3A%20string%5B%5D%20%7B%0A%09%20%20return%20%5B%5D%3B%0A%20%20%7D%0A%20%20%0A%20%20set%20items(value%3A%20any)%20%7B%0A%09%20%20%0A%20%20%7D%0A%7D

jasongrout commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

christianacca commented 8 years ago

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 commented 8 years ago

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

christianacca commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

This would really be a nice feature.

artyil commented 8 years ago

So will this be a feature?

kitsonk commented 8 years ago

@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 commented 8 years ago

@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 commented 8 years ago

I agree.

mhegazy commented 8 years ago

After thinking about this some more. i think the issue here is really the complexity of the implementation. I personally find @Arnavion's example compelling, but the type system today treats getters/setters as regular properties. for this to work, both should have the same value. to support a read/write types would be a large change, i am not sure the utility here would be worth the implementation cost.

sccolbert commented 8 years ago

While I love TypeScript and appreciate all of the effort the team puts into it (really, you folks rock!), I must admit I'm disappointed in this decision. It would be a much better solution than the alternatives of getFoo()/setFoo() or get foo()/set foo()/setFooEx().

RyanCavanaugh commented 8 years ago

Just a short list of problems:

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.

kitsonk commented 8 years ago

@RyanCavanaugh while I agree with you on injected opinion, I can see one counter argument that might just be very TypeScripty... Throwing something weakly typed at a setter, but having something always strongly typed returned, e.g.:

foo.bar = [ '1', 2 ];  // any[]
console.log(foo.bar);  // number[]: [ 1, 2 ]

Though personally I tend to think if you are going to be that magical, best to create a method so that the end developer can clearly understand that what will be bended, folded and mutilated.

ycabon commented 8 years ago

Here is our use case for this feature. Our API introduced a capability that we named Autocasting. The main benefit is a streamlined developer experience who can eliminate the number of classes to import to assign properties for which the type is well defined.

For example a color property can be expressed as a Color instance or as a CSS string like rgba(r, g, b, a) or as an array of 3 or 4 numbers. The property is still typed as instance of Color, as it is the type of what you get when reading the value.

Some info about it: https://developers.arcgis.com/javascript/latest/guide/autocasting/index.html

Our users have been very happy to get that feature, reducing the number of imports needed, and are understanding perfectly that the type changes the line after the assignment.

thorn0 commented 7 years ago

Another example for this issue: https://github.com/gulpjs/vinyl#filebase

file.base = 'd:\\dev';
console.log(file.base); //  'd:\\dev'
file.base = null;
console.log(file.base); //  'd:\\dev\\vinyl' (returns file.cwd)

So the setter is string | null | undefined and the getter is just string. What type should we use in the type definitions for this library? If we use the former, the compiler would require useless null checks everywhere, If we use the latter, we won't be able to assign null to this property.

Elephant-Vessel commented 7 years ago

I have another example where I'd like the getter to return nullable, but where the setter should never allow null as input, stylized as:

class Memory {
    public location: string;
    public time: Date;
    public company: Person[];
}

class Person
{
    private _bestMemoryEver: Memory | null;

    public get bestMemoryEver(): Memory | null { // Might not have one yet
        return this._bestMemoryEver;
    }

    public set bestMemoryEver(memory: Memory) { // But when he/she gets one, it can only be replaced, not removed
        this._bestMemoryEver = memory;
    }
}

var someDude = new Person();
// ...
var bestMemory: Memory | null = someDude.bestMemoryEver;
//...
someDude.bestMemoryEver = null; // Oh no you don't!

I understand that it might be too much work to build some special logic for allowing getters/setters to differ on null, and it's not such a big deal for me, but it'd be nice to have.

aluanhaddad commented 7 years ago

@Elephant-Vessel philosophically I absolutely love the example, it represents the fickle nature of human beings well but I'm not convinced that it would not represent that even better by allowing null (or undefined) to be set. How can I model synapse failure in the system?

Elephant-Vessel commented 7 years ago

@aluanhaddad Why would you want synapse failure? I don't want that kind of bad stuff in my universe ;)

wizarrc commented 7 years ago

Any updates on this? What about having a default value when set to null or undefined?

I don't want the consumer to have to null check beforehand. Currently I have to make the setter a separate method instead, but I would like them to be the same.

Below is what I would like to have:

export class TestClass {
  private _prop?: number;

  get prop(): number {
    // return default value if not defined
    this._prop === undefined ? 0 : this._prop;
  }
  set prop(val: number | undefined) {
    this._prop = val;
  }
}

It seems to me that having the benefit of non-null checking comes with gotchas, and this is one of them. With strict null checking turned off, this is possible, but you don't get the compiler help to prevent null reference exceptions. However, if you want compiler assistance, I feel that should come with more support like having separate definitions for getters and setters with respect to at least nullability if nothing else.

kitsonk commented 7 years ago

The labels on the issue indicate it is a design limitation and the implementation would be considered too complex, which essentially means that if someone has a super compelling reason why this should be the case, it is not going anywhere.

wizarrc commented 7 years ago

@mhegazy @kitsonk I may be biased, but I feel this is a bug that popped up for strict null checking on a common pattern, especially in other curly brace like languages where they don't yet have null checking. A workaround would require the consumer to use the bang operator or check that it is actually never null (which is the point of never being null with using default values).

This breaks down once you add strict null checking because now it's technically different types. I'm not asking for strong different types to be set, but it seems like the design requirements to enable this would also enable strong different types as well.

An alternative design could be used for weak types, such that types like null and undefined would be special cased for interface definitions and d.ts files if they don't want to enable fully different types.

In response to https://github.com/Microsoft/TypeScript/issues/2521#issuecomment-199650959 Here is a proposal design that should be less complex to implement:

export interface Test {
  undefset prop1: number; // property [get] type number and [set] type number | undefined
  nullset prop2: number; // property [get] type number and [set] type number | null
  nilset prop3: number; // property [get] type number and [set] type number | null | undefined
  undefget prop4: number; // property [get] type number | undefined and [set] type number
  nullget prop5: number; // property [get] type number | null and [set] type number
  nilget prop6: number; // property [get] type number | null | undefined and [set] type number
}
thw0rted commented 7 years ago

It looks like there are some people watching this thread that are much more familiar with TypeScript, so maybe somebody who's still paying attention can answer a related question. On this Cesium issue I mentioned the get/set type limitation we're discussing here, and the Cesium folks said the pattern they're following comes from C# -- it's the implicit constructor.

Can TypeScript support implicit constructors? That is, can I say that myThing.foo always returns a Bar, and can be assigned a Bar directly, but can also be assigned a number which will be quietly wrapped in / used to initialize a Bar, as a convenience to the developer? If it's possible to do this by annotating Bar, or maybe specifically saying that "number is assignable to Bar<number>", it would address the use case discussed in the Cesium issue, and also many of the issues raised in this thread.

If not, do I need to suggest implicit constructor support in a separate issue?

thw0rted commented 7 years ago

The more I think / read about it, the more I'm certain that the "implicit constructor pattern" is going to need the feature described in this issue. The only way it's possible in vanilla JS is using object get/set accessors, because that's the only time that nominal assignment (= operator) is actually calling a user-defined function. (Right?) So, I think we really will need

class MyThing{
  set foo(b: Bar<boolean> | boolean);
  get foo(): Bar<boolean>;
}

which it sounds like @RyanCavanaugh thinks is not "sane semantics".

The simple fact is, there's a pretty popular JS library out there that uses this pattern, and it looks like it's difficult if not impossible to describe given existing TS constraints. I hope I'm wrong.

kitsonk commented 7 years ago

JavaScript already allows the pattern you describe. The challenge is that the read and write side of types in TypeScript are, by design, assumed to be the same. There was a modification to the language to disallow assignment (readonly) but there are a few issues that have requested the write only concept, which have been discussed as too complex for little real world value.

IMO, ever since JavaScript allowed accessors, people have potentially created confusing APIs with them. I personally find it confusing that something on assignment magically changes to something else. Implicit anything, especially type conversion, is the bane of JavaScript IMO. It is exactly the flexibility that causes problems. With those type of conversions, where there is magic, I personally like to see methods being called, where it is a bit more explicit to the consumer that some sort of ✨ will occur and make a read only getter to retrieve values.

That doesn't mean real world usage doesn't exist, that is potentially sane and rationale. I guess it comes up to the complexity of splitting the entire type system into two, where types have to be tracked on their read and write conditions. That seems like a very non-trivial scenario.

thw0rted commented 7 years ago

I agree about the :sparkles: here, but I'm not arguing for or against using the pattern, I'm just trying to come along behind code that already uses it and describe the shape of it. It already works the way it works, and TS isn't giving me the tools to describe it.

For better or worse JS has given us the ability to turn assignments into function calls and people are using it. Without the ability to assign different types to set/get pairs, why even have set/get in ambient typings at all? Salsa doesn't have to know that a property is implemented with a getter and setter if it's always going to treat myThing.foo as a member variable of a single type regardless of which side of the assignment it's on. (Obviously, actual TypeScript compilation is another thing altogether.)

aluanhaddad commented 7 years ago

@thw0rted

It looks like there are some people watching this thread that are much more familiar with TypeScript, so maybe somebody who's still paying attention can answer a related question. On this Cesium issue I mentioned the get/set type limitation we're discussing here, and the Cesium folks said the pattern they're following comes from C# -- it's the implicit constructor.

C#'s implicit user defined conversion operators perform static code generation based on the types of values. TypeScript types are erased and do not influence runtime behavior (async/await edge cases for Promise polyfillers not withstanding).

@kitsonk I have to disagree in general with respect to properties. Having spent a fair amount of time with Java, C++, and C#, I absolutely love properties (as seen in C#) because they provide a critical syntactic abstraction (this is somewhat true in JavaScript). They allow for interfaces to segregate read/write capabilities in meaningful ways without using changing syntax. I hate seeing verbs wasted on trivial operations like getX() when getting X can be implicit. IMO confusing APIs, many of which do as you say abuse accessors, stem more from too many setters do magical things.

If I have a readonly, but live view over some data, say a registry, I think readonly properties are very easy to understand.

interface Entry {key: string; value: any;}

export function createRegistry() {
  let entries: Entry[] = [];
  return {
    register(key: string, value: any) {
      entries = [...entries, {key, value}];
    },
    get entries() {
      return [...entries];
    }
  }
}

const registry = createRegistry();

registry.register('hello', '您好');
console.log(registry.entries); //[{key: 'hello', value: '您好'}]
registry.register('goodbye', '再见');
console.log(registry.entries); //[{key: 'hello', value: '您好'}, {key: 'goodbye', value: '再见'}]

Sorry for the tangent, but I love accessors for this and think they are easy to understand but I am willing to be convinced otherwise and readability is my first concern.

juanpablodelatorre commented 7 years ago

When TypeScript limits JavaScript, it becomes more of a nuisance than an advantage. Isn't TypeScript meant to help developers communicate with each other?

Also, setters are called Mutators for a reason. If I wouldn't need any kind of conversion, I wouldn't use a setter, I would set the variable by myself.

disjukr commented 7 years ago

porting javascript project to typescript. i met this problem..

raysuelzer commented 7 years ago

This would be nice to use on angular @Input decorators. Because the value is passed from a template, there would make it much cleaner, in my opinion, to deal with different incoming object types.

Update: this appears to work for me

import { Component, Input } from '@angular/core';
import { flatMap, isString, isArray, isFalsy } from 'lodash';

@Component({
  selector: 'app-error-notification',
  templateUrl: './error-notification.component.html',
})

export class ErrorNotificationComponent {
  private _errors: Array<string> = [];
  constructor() { }
  /**
   * 'errors' is expected to be an input of either a string or an array of strings
   */
  @Input() set errors(errors: Array<string> | any){
      // Caller just passed in a string instead of an array of strings
      if (isString(errors)) {
        this._errors = [errors];
      }
      // Caller passed in array, assuming it is a string array
      if (isArray(errors)) {
        this._errors = errors;
      }
      // Caller passed in something falsy, which means we should clear error list
      if (isFalsy(errors)) {
        this._errors = [];
      }
      // At this point just set it to whatever might have been passed in and let
      // the user debug when it is broken.
      this._errors = errors;
  }

  get errors() {
    return this._errors;
  }
}
lifaon74 commented 6 years ago

So were are we ? I would really like to have the possibility to return a different type with the getter than from the setter. For example:

class Field {
  private _value: string;

  get value(): string {
    return this._value;
  }

  set value(value: any) {
    this._value = String(value);
  }
}

This is what does 99% of the native implementations (if you pass a number to a (input as HTMLInputElement).value it will always return a string. In fact get and set should be considered as methods and should allow some:

set value(value: string);
set value(value: number);
set value(value: any) {
  this._value = String(value);
}
  // AND/OR
set value(value: string | number) {
  this._value = String(value);
}
thw0rted commented 6 years ago

@raysuelzer , when you say your code "works", doesn't ErrorNotificationComponent.errors return a type of Array<string> | any? That means you need type guards every time you use it, when you know it can only ever actually return Array<string>.

@lifaon74 , as far as I know there is no movement on this issue. I think a compelling case has been made -- multiple scenarios presented, tons of legacy JS code that can't be properly described in Typescript because of this -- but the issue is closed. Maybe if you think the facts on the ground have changed, open a new one? I don't know the team's policy on retreading old arguments, but I'd have your back.

kitsonk commented 6 years ago

I don't know the team's policy on retreading old arguments, but I'd have your back.

They will reconsider previously closed subjects. Providing a 👍 at the top of the issue puts credence in that it is meaningful. I believe that it has generally fallen under the "too complex" category because it would mean that type system would have to be bifurcated on every read and write and I suspect the feeling is the effort and cost of putting in that in to satiate what is a valid but somewhat uncommon use case isn't worth it.

My personal feeling is it would be a nice to have, especially for the being able to model existing JavaScript code that uses this pattern effectively.

thw0rted commented 6 years ago

I personally would consider the matter resolved if we came up with some not-totally-onerous workaround for the legacy JS issue. I'm still kind of a TS greenhorn, so maybe my current solution (forced casting or unnecessary type guards) is not an optimal use of existing capabilities?

vcfvct commented 6 years ago

Agree. The setter will be so much limited when type has to be the same... Please enhance.