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

Update type definition of Object.keys #17267

Closed
wants to merge 1 commit into from
Closed

Conversation

kube
Copy link

@kube kube commented Jul 18, 2017

Solves need to explicitly specify type of key when in Strict Mode.

Issue it solves

const someObj = {
  a: 42,
  b: 'Hello'
}

Object.keys(someObj)
  .forEach(key =>
    someObj[key] // ERROR: Element implicitly has an 'any' type because type ... has no index signature.
  )

Solves need to explicitly specify type of `key` when in **Strict Mode**.

## Issue it solves

```ts
const someObj = {
  a: 42,
  b: 'Hello'
}

Object.keys(someObj)
  .forEach(key =>
    someObj[key] // ERROR: Element implicitly has an 'any' type because type ... has no index signature.
  )
```
@msftclas
Copy link

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

@ikatyang
Copy link
Contributor

These files are not meant to be edited by hand. If you need to make modifications, the respective files should be changed within the repository's top-level src directory. Running jake LKG will then appropriately update the files in this directory. (/lib@master#read-this)

---> /src/lib@master#read-this

And also, there is already a similar PR, see #17261 (only modify the type of o).

@kube
Copy link
Author

kube commented Jul 18, 2017

Ok sorry for not reading, and thanks for your comment. :/

PR #17261 does not solve this issue, as it just restricts what can be passed to Object.keys.

My PR is to link the output type of Object.keys to the input type, and I don't think it's possible without a generic type.

@ikatyang
Copy link
Contributor

ikatyang commented Jul 18, 2017

EDIT: overload does nothing in this case.

I think it'd be better to add overload instead of modifying the original one, since Object.keys should always return string[], consider the following case:

declare function keys<A>(o: A): (keyof A)[];
declare const x: object;

keys(x); //=> never[]; <------- should be string[]

@jwbay
Copy link
Contributor

jwbay commented Jul 18, 2017

See - #15295,
#13254,
#12253 (comment)

@@ -236,7 +236,7 @@ interface ObjectConstructor {
* Returns the names of the enumerable properties and methods 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.
*/
keys(o: any): string[];
keys<A>(o: A): (keyof A)[];

Choose a reason for hiding this comment

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

<A extends {}>

The problem with making Object.keys return keyof A is that typescript makes no guarantees that the input object has only the keys that are specified in its type. There may be valid values in the array that are not considered by the type.

@rbuckton rbuckton requested a review from sandersn August 9, 2017 20:49
@rbuckton
Copy link
Member

rbuckton commented Aug 9, 2017

@sandersn, can you take a look?

@sandersn
Copy link
Member

sandersn commented Aug 9, 2017

I think the issue raised in #12253 (comment) still holds: (keyof T)[] is not useful or accurate when Object.keys is called with an object that has more properties than its declared type. I'll close this PR in favour of the simpler #17261 that just has o: {}.

@sandersn sandersn closed this Aug 9, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 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.

7 participants