Skip to content
This repository was archived by the owner on Jul 23, 2021. It is now read-only.

Fix type signature of update and mapEntries #9

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix type signature of update and mapEntries #9

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions type-definitions/Immutable.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,8 +401,8 @@ declare module Immutable {
*
* @see `Map#update`
*/
update(index: number, notSetValue: T, updater: (value: T) => T): this;
update(index: number, updater: (value: T) => T): this;
update(index: number, notSetValue: T, updater: (value: T | undefined) => T): this;
update(index: number, updater: (value: T | undefined) => T): this;
Comment on lines +404 to +405
Copy link

@Methuselah96 Methuselah96 Aug 22, 2020 •
edited
Loading

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?

Copy link
Member Author

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.

update<R>(updater: (value: this) => R): R;

/**
Expand Down Expand Up @@ -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,
Copy link

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably, yes.

context?: unknown
): Map<KM, VM>;

Expand Down