Skip to content

fix: support record in Object.keys #28899

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

Conversation

christophehurpeau
Copy link

@christophehurpeau christophehurpeau commented Dec 7, 2018

  • There is an associated issue that is labeled
    'Bug' or 'help wanted' or is in the Community milestone
  • Code is up-to-date with the master branch
  • You've successfully run jake runtests locally
  • You've signed the CLA
  • There are new or updated unit tests validating the change

This PR fixes an issue when strictFunctionTypes: true. This code failed:

type K = 'foo' | 'bar'
const record: Record<K, boolean> = { foo: true, bar: false };
Object.keys(record).forEach((key: K) => {
}); 

with the error:

Argument of type '(key: K) => void' is not assignable to parameter of type '(value: string, index: number, array: string[]) => void'.
  Types of parameters 'key' and 'value' are incompatible.
    Type 'string' is not assignable to type 'K'.

@msftclas
Copy link

msftclas commented Dec 7, 2018

CLA assistant check
All CLA requirements met.

@christophehurpeau christophehurpeau force-pushed the fix/object-keys-with-record branch from bf2082e to 146ed2f Compare December 7, 2018 12:22
Object.keys(record).forEach((key: K) => {
>Object.keys(record).forEach((key: K) => {}) : void
>Object.keys(record).forEach : (callbackfn: (value: string, index: number, array: string[]) => void, thisArg?: any) => void
>Object.keys(record) : string[]
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't look like this is using the right overload. Your code is only compiling because you can assign a function (i.e. (key: K) => {}) whose argument is a subset of type A to a function whose argument is A. For example, this is valid:

const abc: (def: number | string) => void = (def: number) => {}

Copy link
Author

Choose a reason for hiding this comment

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

It appears that it was using src/lib/es5.d.ts, not lib/lib.es5.d.ts. Now it is Object.keys(record) : K[] !

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the change should only be in src. lib is auto-generated from that folder.

@christophehurpeau christophehurpeau force-pushed the fix/object-keys-with-record branch from 0523e1f to 2db95f6 Compare December 9, 2018 14:21
@@ -409,10 +409,10 @@ export enum PubSubRecordIsStoredInRedisAsA {
>new Set(Object.keys(soFar) as (keyof SO_FAR)[]) : Set<keyof SO_FAR>
>Set : SetConstructor
>Object.keys(soFar) as (keyof SO_FAR)[] : (keyof SO_FAR)[]
>Object.keys(soFar) : string[]
>Object.keys : (o: {}) => string[]
>Object.keys(soFar) : never[]
Copy link
Author

Choose a reason for hiding this comment

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

why did this changed to never ?

Copy link
Member

Choose a reason for hiding this comment

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

{} has no properties, so keyof will return the empty union (i.e. `never)

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Dec 11, 2018

This won't work because the object could have keys the static type isn't describing, so you can get values at runtime which you didn't expect.

See other discussion about trying to tie in Object.keys and keyof together: #12870, #13971, #24243, #25832, #26901

@DanielRosenwasser DanielRosenwasser added the Duplicate An existing issue was already created label Dec 11, 2018
@typescript-bot
Copy link
Collaborator

This issue has been marked as a duplicate and has seen no activity in the last day. It has been closed automatic house-keeping purposes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Duplicate An existing issue was already created
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants