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

limited use of [index:string] #10

Closed
donnut opened this issue Apr 29, 2015 · 5 comments
Closed

limited use of [index:string] #10

donnut opened this issue Apr 29, 2015 · 5 comments

Comments

@donnut
Copy link
Collaborator

donnut commented Apr 29, 2015

Currently some function definitions like propEq use the index signature to type object arguments. This seems of limited use, because the supplied object needs to have this index signature explicitly defined. For example.
One of the type signatures of propEq is

propEq<T>(name: string, val: T, obj: {[index:string]: T}): boolean;

Using this function with var obj = {a: 1, b: 2} like

propEq('a', 1, obj); // Type error

is correct, however TypeScript complains with:
argument of type 'string' is not assignable to parameter of type 'number' pointing to the a parameter.
This is because obj does not have a index signature. If we add this signature to obj explicitly the type error disappears:

var obj: {[index: string]: number} = {a: 1, b: 2}
propEq('a', 1, obj); // OK

This reason for this behavior is explained in http://stackoverflow.com/questions/22077023/why-cant-i-indirectly-return-an-object-literal-to-satisfy-an-index-signature-re#22077024

I think the requirement that only objects with an index signature can be used is too strict and that we therefore should limit the use of index signature to those cases where it is obviously (but I can not think of one at the moment).

Any thoughts on this issue?

@donnut donnut changed the title limited us [index:string] limited use of [index:string] Apr 29, 2015
@yazla
Copy link

yazla commented Feb 23, 2016

I have just trapped on this, and it confused me a lot. I expected "props" to have the same signature as "prop" except for input args. Why isn't it so? I believe loosing a bit of type safety is a affordable trade off here. At least there could be all possible overloads to choose from.
So as for me "props" should look like this.

props<T>(ps: string[]): (obj: any) => T[];

@jcristovao
Copy link
Collaborator

Hey, I'm sorry for chiming in, but I think there are a lot of issues here:

  1. The issue originally report by @donnut seems to no longer apply as of typescript 1.8, as indicated by the referenced stackoverflow question. Hence, we can (and should?) have the [index:string]

1.a) I really don't understand the third to six lines of the type signature in propEq:

    propEq<T>(name: string, val: T, obj: any): boolean;
    propEq<T>(name: number, val: T, obj: any): boolean;
    propEq<T>(name: string, val: T): (...args: any[]) => boolean;
    propEq<T>(name: number, val: T): (...args: any[]) => boolean;
    propEq<T>(name: string): (val: T, ...args: any[]) => boolean;
    propEq<T>(name: number): (val: T, ...args: any[]) => boolean;

Shouldn't it be:

    propEq<T>(name: string, val: T, obj: any): boolean;
    propEq<T>(name: number, val: T, obj: any): boolean;
    propEq<T>(name: string, val: T): (obj:any]) => boolean;
    propEq<T>(name: number, val: T): (obj:any) => boolean;
    propEq<T>(name: string): (val: T, obj:any) => boolean;
    propEq<T>(name: number): (val: T, obj:any) => boolean;

Or better yet, obj:{[index:string]: any } ?

  1. I totally agree with @yazla that props and prop should have related signatures, which is not the case, and it is confusing. But given the new capabilities of the typescript compiler I mentioned above, I actually think it is prop which needs to be changed.

What do you think?

@jcristovao
Copy link
Collaborator

Hum, I stand corrected, this seems to only be fixed on typescript 1.9, which is now bleeding edge / alpha. So, I guess if we are still some months away of having this PR fixed in a more 'production ready' environment :(

@donnut
Copy link
Collaborator Author

donnut commented Feb 25, 2016

That explains why my quick try wasn't able to reproduce it.

@KiaraGrouwstra
Copy link
Member

At this point, TS 2.0 is stable, so the original issue should no longer be a concern. propEq has been adjusted in line with @jcristovao's request by now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants