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

Support type-checking of computed properties for constants and Symbols #5579

Closed
IgorMinar opened this issue Nov 9, 2015 · 34 comments · Fixed by #15473
Closed

Support type-checking of computed properties for constants and Symbols #5579

IgorMinar opened this issue Nov 9, 2015 · 34 comments · Fixed by #15473
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript

Comments

@IgorMinar
Copy link

On Angular we are considering using ES6 computed prototype methods as part of Angular’s component api but we realized that typescript doesn’t allow us to express this in a type-safe way.

I’m wondering if this is a feature that could be added to typescript or if there is a fundamental issue that would prevent typescript from supporting this. Example:

{Component, onInit, OnInit} from ‘angular2/core';

@Component({
  selector: 'my-component'
})
class MyComponent implements OnInit {
  [onInit](param1, param2){ return something; }
}

Where onInit is a const string or ES Symbol, and OnInit is an interface with the signature of the [onInit] method. onInit is a an optional life-cycle hook that Angular will call if the component has this property name.

The reason why we find this api style attractive is that it removes the possibility of name-collisions, which means that we are free to add support for more hooks in the future without affecting any existing components.

PS: ES Observables are already using Symbols to define the observable interface, so this feature would allow creation of Observable interface. I expect more libs will want to take advantage of this api style as computed property and Symbol support becomes better.

Related issue: #4653

@mhegazy mhegazy added the Suggestion An idea for TypeScript label Nov 10, 2015
@mhegazy
Copy link
Contributor

mhegazy commented Nov 10, 2015

#4653 is much simpler to do. in fact @vladima has a change out for this one.

Using non-literals is a different issue. there is a more elaborate discussion here: #2012

@mhegazy mhegazy added the In Discussion Not yet reached consensus label Nov 10, 2015
@ahejlsberg
Copy link
Member

@IgorMinar The tricky issue here is that during the binding phase of compilation we need to know the spelling of every symbol (so we can build symbol tables). However, we can't know the actual spelling of the string or symbol that onInit references without resolving that expression, which requires binding information, which is what we're in the process of collecting. So, to support full checking of user defined symbols we'd have to introducing some sort of iterative partial binding which would be a fairly disruptive change. It currently works for built-in symbols only because we specially recognize symbols of the form [System.XXX], but it isn't clear that we could do something similar for libraries in general.

@benlesh
Copy link

benlesh commented Apr 14, 2016

This seems directly related to an issue I have here: #8099

@benlesh
Copy link

benlesh commented Apr 14, 2016

@ahejlsberg would it be possible to type this with an annotation like [Symbol.for('xyz')]? Since Symbol.for() creates and returns a single static reference, this could be "type checked" so to speak.

I think there's an obvious problem with dealing with [randomSymbol] properties, in that Symbol('foo') === Symbol('foo') is false.

A weirder problem exists around the polyfilling of symbols with "@@mysymbol"... which depending on the platform is interchanged with a symbol as a property key. so { ["@@mysymbol"]: () => {} } should match { [Symbol.for('mysymbol')]: () => void }.

@DanielRosenwasser
Copy link
Member

If there was something like a generic primitive type, symbol<"xyz"> would be an interesting possibility. Default type arguments could also make symbol equivalent to symbol<any>. I dunno, just an idea.

@kitsonk
Copy link
Contributor

kitsonk commented Apr 20, 2016

Coming in with something related... @ahejlsberg you say that the compiler has a bit of a challenge with resolving the types that aren't on the global Symbol. Even though clearly I don't know the inner working, I wonder if that is strictly still true. For Dojo 2 we produced a Symbol shim which doesn't modify the global scope if there is no Symbol present (though it does fill in missing "well knowns"). Then other modules, instead of relying upon the global Symbol reference the module (therefore minimising issues other code running in the same environment).

When we target es5 everything works perfectly fine. The compiler handles everything and is even aware of the primitive type of symbol. It is only when targeting es6 does the error 'Symbol' reference does not refer to the global Symbol constructor object. present itself.

@mhegazy
Copy link
Contributor

mhegazy commented Apr 20, 2016

The way it works today is by checking the text of the name to identify the property, that is exactelly "Symbol.iterator". if you change any thing about this it is not found. this is why it is important to know that you are using the global "Symbol" and not a local Symbol. in --t es5, the definition of Symbol does not exist, so the check does not happen. but that does not mean it "works". for instance, see:

import sym from './Symbol';
var i: Iterable<string>;
i[sym.iterator] // not the same as Symbol.iterator.

import Symbol from `./Symbol`;
const iteratorSymbol = Symbol.iterator;
i[sym.iterator] // still not the same as Symbol.iterator.

This issue tracks making it work, ideally by knowing the "identity" of a symbol and use that to define and access properties.

there is more relevant discussion on why it works this way and what is missing in #2012

@benlesh
Copy link

benlesh commented Apr 20, 2016

the global "Symbol" and not a local Symbol

It's probably worth disambiguating this a bit:

  1. Symbol('description'): A local symbol with a description, always a different reference.
  2. Symbol.for('name'): A global symbol looked up by name and created if it doesn't exist
  3. Symbol.iterable, Symbol.toStringTag, et al: Global symbols that should always be present.
  4. "@@whatever": A string property name used as a place holder, generally for number 2 above, in environments where Symbol doesn't exist (legacy browsers, etc).

I don't think we can track the first type there. The rest, though, should be doable.

@kitsonk
Copy link
Contributor

kitsonk commented Apr 21, 2016

Ok, I understand now. I guess though, it is a but "surprising" that unlike most features in TypeScript which have some sort of the "trust me, I know what I am doing" flag. This particular one doesn't seem to have such an option.

@shlomiassaf
Copy link

Partially related...
To really boost the usage of Symbol I would love to see dot notation support.

The developer will define the symbol using a special TS syntax. TypeScript will convert it to an index assignment in the transpilled code.

const mySymbol = Symbol('My Symbol!!!');

class MyClass {
  @mySymbol(value1: string): number { // also supports modifiers.
     // do something...
  }
}

let myClass = new MyClass();
myClass.@mySymbol('abc'); // <- Intellisense + return type number.

Of course there are issues, this is just a quick sample for an idea:

  1. No reference to mySymbol outside of the module
  2. TypeScript unique syntax (like this in functions)
  3. Might cause confusion with decorators at the class declaration level.
  4. Usage when set in 3rd party package imported to a project???

Since (1) is a big issue TypeScript can work with Symbol.for() and auto set the key using module id + the name of the class + name of the property/method and maybe a TS unique prefix. Or just a random string typescript tracks.

class MyClass {
  @mySymbol(value1: string): number { // also supports modifiers.
     // do something...
  }
}

Becomes:

var MyClass = (function () {
    function MyClass() {
    }
    MyClass.prototype[Symbol.for('TS_SYMBOL: 12.MyClass.mySymbol')] = function (value1) {
        // do something...
    };
    return MyClass;
}());

As for (4), open for discussion :)

Anyway, if this is possible it will make Symbols a great feature that can be used in public API's.
Imagine a data access object and the model as one class. having @save() operations and other hooks/methods/validation etc without compromising due to naming collisions with the model and not have to use a getters/setters

@tomdale
Copy link

tomdale commented Aug 19, 2016

I am running into this limitation in my current project. Specifically, I am implementing a JSON serializer that needs to retrieve information about models, such as their ID, attributes, relationships, etc.

Instead of hard-coding the serializer to a particular model library, I wanted to support a serialization protocol object that any model instance can implement, using symbols. Unfortunately the inability to define an interface using a non-built-in Symbol is stymying that somewhat.

The code looks something like this:

namespace JSONAPISerializer {
  export const PROTOCOL = Symbol("serializer-protocol");

  export interface SerializerProtocol {
    getType(): string;
    getID(): string | number;
  }

  export interface Serializable {
    [PROTOCOL]: SerializerProtocol;
  }
}

I want to be able to write a Model class that implements the serialization protocol:

import { PROTOCOL, Serializable } from "json-serializer";

class Model implements Serializable {
  [PROTOCOL] = {
    getType() { ... },
    getId(): { ... }
  }
}

In my case, I am happy to have the constraint of the symbol being const—redefining symbols at runtime seems like a bad idea anyway.

@calebmer
Copy link

The implications of this proposal and cross-class privacy interests me. In some more complex systems it may be nice to have a module with multiple classes that can call certain methods on each other, but not allow consumers access to those methods. A hidden symbol (hidden from consumers at least) could be an interesting solution to this. Take the following code:

namespace X {
  const privateMethod = Symbol()

  export class A {
    b: B

    constructor (b) {
       this.b = b
    }

    b (): string {
      return this.b[privateMethod]()
    }
  }

  export class B {
    [privateMethod] (): string {
      return 'yay!'
    }
  }
}

const b = new X.B()
const a = new X.A(b)

// Works
a.b()

// Auto-completion shows nothing when typing `b.` or `b[`

@dead-claudia
Copy link

dead-claudia commented Dec 24, 2016

This would make polyfilling proposed well known symbols for proposals like Observables (using Symbol.observable) and async iterators (using Symbol.asyncIterator) possible. Most notably, RxJS 5 itself needs Symbol.observable to properly type Observable.from, because that's used as an interop point, and is mandated in the ES observable proposal. Other libraries that interoperate with observables also are aware of it, like most.from(observable) from most.js.

@acutmore
Copy link
Contributor

@calebmer

A hidden symbol (hidden from consumers at least)

Unique Symbols certainly make access to methods harder but they can't be fully hidden from a consumer. Object.getOwnPropertySymbols will expose an symbols used as properties. They are better suited to prevent clashes without having to namespace properties.

Closures are the only way to block access to something in Javascript. (AFAIK)


+1 For supporting run-time created symbols in interfaces. Well-known symbols and Symbol.for should have the same compiler support so as not to block the functionality of 3rd party libraries such as RxJs.

@whatisaphone
Copy link

This feature would also be helpful when working with React. A minimal example:

import * as React from 'react';
import { ChangeEvent } from 'react';

interface LoginState { username: string; password: string; }

export class Login extends React.Component<{}, LoginState> {
  public state: LoginState = { username: '', password: '' };

  private onChange(event: ChangeEvent<HTMLInputElement>, property: keyof LoginState) {
    this.setState({ [property]: event.target.value });  // this doesn't work!
  }

  public render() {
    return (
      <form>
        <input value={this.state.username}
               onChange={(e) => this.onChange(e, 'username')}/>
        <input type="password"
               value={this.state.password}
               onChange={(e) => this.onChange(e, 'password')}/>
        <input type="submit" value="Login"/>
      </form>
    );
  }
}

Relevant definition of setState:

class Component<P, S> {
    setState<K extends keyof S>(state: Pick<S, K>, callback?: () => any): void;
}

Right now the commented line in onChange does not typecheck. The compiler doesn't realize that a { [k: keyof LoginState]: string } can be assigned to a Pick<LoginState, K extends keyof LoginState>.

We currently bypass this by calling this.setState({ ...this.state, [property]: event.target.value }), but this essentially bypasses the typechecker (e.g. calling this.setState({ ...this.state, foo: 'bar' }) compiles as well). It's also passing more keys than necessary to setState, and making it do unnecessary work.

@mhegazy
Copy link
Contributor

mhegazy commented May 4, 2017

@johnsoft i am afraid this is a different request and not included in this issue.

@whatisaphone
Copy link

@mhegazy You closed issue #15534 as a dup of this one yesterday :). It seems to me it fits within "type-checking of computed properties for constants".

As far as I can tell (and I could be wrong), #15534 is the root cause of the React issue above. What would be the best way for me to get the above issue on the typescript team's radar?

@mhegazy
Copy link
Contributor

mhegazy commented May 4, 2017

i see. this issue are about using a computed property whose type is a single literal type, and that is constant. thus the compiler can make assumptions about the name/key of the property. The example in #15534 made it seem like that is what you were looking for.

The setState example makes it clear what you are looking for. What is needed here is to distribute the type over the union in keyof. i.e. { [property]: event.target.value } would have the type { "username": event.target.value } | { "password": event.target.value }. Do not think this is the same issue. if you want to file a new issue or add more details to #15534 we can reopen it.

@whatisaphone
Copy link

Sure, I'll copy my example over to the previous issue.

@jacobrask
Copy link

I would want to warn against this.setState({ ...this.state, [property]: event.target.value }) because it might not do what you expect due to setState's asynchronous nature.

Another workaround which does not change your JavaScript output is

this.setState({ [property]: event.target.value } as { username: string } | { password: string });

@dead-claudia
Copy link

@jacobrask For the above example, setState works well enough, and in addition, setState is called after the mapped property is resolved, so that's not a concern.

@dead-claudia
Copy link

@johnsoft Your onChange was slightly incorrect, so the types would likely be incorrectly inferred. If you add a generic so TypeScript can pass through the information that property only carries one property name, it might work better. See if this fixes your issue:

import * as React from 'react';
import { ChangeEvent } from 'react';

interface LoginState { username: string; password: string; }

export class Login extends React.Component<{}, LoginState> {
  public state: LoginState = { username: '', password: '' };

  private onChange<K extends keyof LoginState>(event: ChangeEvent<HTMLInputElement>, property: K) {
    this.setState({ [property]: event.target.value });
  }

  public render() {
    return (
      <form>
        <input value={this.state.username}
               onChange={(e) => this.onChange(e, 'username')}/>
        <input type="password"
               value={this.state.password}
               onChange={(e) => this.onChange(e, 'password')}/>
        <input type="submit" value="Login"/>
      </form>
    );
  }
}

@whatisaphone
Copy link

@isiahmeadows Doesn't work, unfortunately.

$ tsc "-v"
Version 2.4.0-dev.20170518
index.tsx(11,19): error TS2345: Argument of type '{ [x: string]: string; }' is not assignable to parameter of type 'Pick<LoginState, "username" | "password">'.
  Property 'username' is missing in type '{ [x: string]: string; }'.
index.tsx(11,21): error TS2464: A computed property name must be of type 'string', 'number', 'symbol', or 'any'.

@dead-claudia
Copy link

@johnsoft The way you were doing it before would've been not type-safe across fields, which is why I suggested the change. I didn't expect it'd solve your problem completely, but it'll make it easier to catch errors (and hit this bug).

@whatisaphone
Copy link

@isiahmeadows Right, I get it. It would matter in the case where username and password were not the same type.

@nbransby
Copy link

nbransby commented Nov 6, 2017

Any update on this? I noticed I still cannot strongly type my non built-in symbol property declarations in 2.6 👎

@mhegazy mhegazy added Fixed A PR has been merged for this issue and removed In Discussion Not yet reached consensus labels Nov 20, 2017
@mhegazy mhegazy added this to the TypeScript 2.7 milestone Nov 20, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Fixed A PR has been merged for this issue Suggestion An idea for TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.