-
-
Notifications
You must be signed in to change notification settings - Fork 6
Fix type signature of update and mapEntries #9
base: master
Are you sure you want to change the base?
Conversation
Interesting, it seems to be an undocumented feature of |
I imagine we should also update the other instances of |
update(index: number, notSetValue: T, updater: (value: T | undefined) => T): this; | ||
update(index: number, updater: (value: T | undefined) => T): this; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how I feel about this change. TypeScript doesn't return T | undefined
for arrays and I'm wondering if this is similar. I feel like this would be inconvenient if the user knows that the type is not undefined
. I would be interested in seeing common use cases of the update
function.
@conartist6 What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So one easy improvement is that if notSetValue
is given then the value
passed to updater
will never be undefined
.
I think immutable should strive to be more correct than plain array access, and that is what most of the types, e.g. get(), do.
@@ -1345,7 +1345,7 @@ declare module Immutable { | |||
* @see Collection.Keyed.mapEntries | |||
*/ | |||
mapEntries<KM, VM>( | |||
mapper: (entry: [K, V], index: number, iter: this) => [KM, VM], | |||
mapper: (entry: [K, V], index: number, iter: this) => [KM, VM] | undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should document the filtering behavior?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, yes.
Superseeded by immutable-js#1842 |
Porting this PR from the unforked repo.