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

Add stricter parameter types for Object.values and Object.entries #20553

Merged
merged 4 commits into from
Jan 8, 2018

Conversation

ghost
Copy link

@ghost ghost commented Dec 7, 2017

Fixes #20550

Added | string | number | boolean | symbol to avoid falling into the second overload unnecessarily; Object.values on a primitive returns [] of these so it's effectively a never[] in that case (though we infer {}[]).
I'm also not too happy about returning any from builtin functions. --noImplicitAny won't help you there. The only case I found where the first overload won't match any more is Object.values(() => {}), in which case a return of {}[] is probably better since a function's unlikely to have the properties you intend to get.

@ghost ghost force-pushed the object_values branch from fbc8266 to 4b7f389 Compare December 7, 2017 23:01
@weswigham
Copy link
Member

weswigham commented Dec 8, 2017

@andy-ms We could just define values as values<T>(o: T): T[keyof T][]; (and similarly for entries). These definitions are pre-index types AFAIK, and we can just do better now, I think.

@Jessidhia
Copy link

Jessidhia commented Dec 8, 2017

@weswigham the problem there is the unsoundness because of structural non-sealed types

const a = { a: 'a string', b: 123 }
const b: Pick<typeof a, 'a'> = a
const values = Object.values(b) // string[]; but actually has a number inside too

EDIT: my example actually turns out to already be incorrect today, because of the overload that tries to handle dictionary types.

@ghost
Copy link
Author

ghost commented Dec 8, 2017

@weswigham What would you think of that signature with T extends object? The function will technically work at runtime if you pass in a string, it's not really useful unless you're passing in an object.

@weswigham
Copy link
Member

@Kovensky any time you alias a type you lose information w.r.t. the index operator. But the thing is, I'm pretty sure that definition (at least for T extends object, like @andy-ms suggests), is more practically useful, since you'll get the types for all the fields we do know about (instead of the unsafe any). Honestly, your complaint is just a function of how the keyof type operator works; but the more tightly your types match your behavior, the more accurate it is. You use it on any type you've aliased (thus discarded information) and you'll get results for the alias, not the original type.

@@ -3,25 +3,14 @@ interface ObjectConstructor {
* Returns an array of values of the enumerable properties of an object
* @param o Object that contains the properties and methods. This can be an object that you created or an existing Document Object Model (DOM) object.
*/
values<T>(o: { [s: string]: T } | { [n: number]: T }): T[];
values<T extends object>(o: T): T[keyof T][];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do not use keyof here. same reasoning as #13989 (comment)


/**
* Returns an array of key/values of the enumerable properties of an object
* @param o Object that contains the properties and methods. This can be an object that you created or an existing Document Object Model (DOM) object.
*/
entries(o: any): [string, any][];
entries<T extends object>(o: T): [string, T[keyof T]][];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@ghost
Copy link
Author

ghost commented Jan 8, 2018

@mhegazy @weswigham The original goal was to not accept undefined and null here. How about making this PR just change values(o: any): any[]; to values(o: {}): any[] (and the same for entries) and leaving the rest for later?

@mhegazy
Copy link
Contributor

mhegazy commented Jan 8, 2018

The original goal was to not accept undefined and null here. How about making this PR just change values(o: any): any[]; to values(o: {}): any[] (and the same for entries) and leaving the rest for later?

that would work.

@ghost ghost force-pushed the object_values branch from 4846846 to 4bb3c90 Compare January 8, 2018 20:44
@ghost
Copy link
Author

ghost commented Jan 8, 2018

Waiting for #21075, then will merge from master

@mhegazy mhegazy merged commit 39dfeb0 into master Jan 8, 2018
@mhegazy mhegazy deleted the object_values branch January 8, 2018 23:33
@aboyton
Copy link

aboyton commented Jan 9, 2018

Thanks. Really loving the TypeScript project and how quickly and thoroughly you fix any reported bugs.

@microsoft microsoft locked and limited conversation to collaborators Jul 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants