-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Improve Typescript definition for Map #1841
Changes from 24 commits
47c78b4
0543432
b5d874a
8cf0983
f7c1f33
9ec983f
4d248af
720b847
c762557
3705794
82e957c
43f6590
72f60e9
b58b9d6
b0faf97
42170d6
790773a
3e160f8
a26d4ec
d112b0c
f225394
89b83a0
0d57333
d627ad3
0d5d6c8
ee1eca1
47be7ee
0e9da4d
ad0356b
beeac64
def6f09
c302576
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,8 +60,11 @@ describe('Map', () => { | |
const l = List([List(['a', 'A']), List(['b', 'B']), List(['c', 'C'])]); | ||
const m = Map(l); | ||
expect(m.size).toBe(3); | ||
// @ts-expect-error -- Not supported by typescript since 4.0.0 https://github.com/immutable-js/immutable-js/pull/1626 | ||
expect(m.get('a')).toBe('A'); | ||
// @ts-expect-error -- Not supported by typescript since 4.0.0 https://github.com/immutable-js/immutable-js/pull/1626 | ||
expect(m.get('b')).toBe('B'); | ||
// @ts-expect-error -- Not supported by typescript since 4.0.0 https://github.com/immutable-js/immutable-js/pull/1626 | ||
expect(m.get('c')).toBe('C'); | ||
}); | ||
|
||
|
@@ -97,6 +100,7 @@ describe('Map', () => { | |
it('accepts non-collection array-like objects as keyed collections', () => { | ||
const m = Map({ length: 3, 1: 'one' }); | ||
expect(m.get('length')).toBe(3); | ||
// @ts-expect-error -- type error, but the API is tolerante | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should not be a type error. Similarly, we should ensure that I filed an issue about this a while back and it was closed as "Working as intended" despite being clearly incorrect. 🤷 - microsoft/TypeScript#45170 (comment) A workaround type to fix this looks like: type ObjectKeys<T> = { [K in keyof T]: K extends number ? `${K}` : K }[keyof T] |
||
expect(m.get('1')).toBe('one'); | ||
expect(m.toJS()).toEqual({ length: 3, 1: 'one' }); | ||
}); | ||
|
@@ -413,7 +417,7 @@ describe('Map', () => { | |
}); | ||
|
||
it('chained mutations does not result in new empty map instance', () => { | ||
const v1 = Map({ x: 1 }); | ||
const v1 = Map<{ x?: number; y?: number }>({ x: 1 }); | ||
const v2 = v1.withMutations(v => v.set('y', 2).delete('x').delete('y')); | ||
expect(v2).toBe(Map()); | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,8 +23,8 @@ describe('merge', () => { | |
}); | ||
|
||
it('merges two maps with a merge function', () => { | ||
const m1 = Map({ a: 1, b: 2, c: 3 }); | ||
const m2 = Map({ d: 10, b: 20, e: 30 }); | ||
const m1 = Map<string, number>({ a: 1, b: 2, c: 3 }); | ||
const m2 = Map<string, number>({ d: 10, b: 20, e: 30 }); | ||
expect(m1.mergeWith((a: any, b: any) => a + b, m2)).toEqual( | ||
Map({ a: 1, b: 22, c: 3, d: 10, e: 30 }) | ||
); | ||
|
@@ -38,8 +38,8 @@ describe('merge', () => { | |
}); | ||
|
||
it('provides key as the third argument of merge function', () => { | ||
const m1 = Map({ id: 'temp', b: 2, c: 3 }); | ||
const m2 = Map({ id: 10, b: 20, e: 30 }); | ||
const m1 = Map<string, string | number>({ id: 'temp', b: 2, c: 3 }); | ||
const m2 = Map<string, number>({ id: 10, b: 20, e: 30 }); | ||
const add = (a: any, b: any) => a + b; | ||
expect( | ||
m1.mergeWith((a, b, key) => (key !== 'id' ? add(a, b) : b), m2) | ||
|
@@ -152,12 +152,12 @@ describe('merge', () => { | |
|
||
it('merges map entries with List and Set values', () => { | ||
const initial = Map({ | ||
a: Map({ x: 10, y: 20 }), | ||
a: Map<string, number>({ x: 10, y: 20 }), | ||
b: List([1, 2, 3]), | ||
c: Set([1, 2, 3]), | ||
}); | ||
const additions = Map({ | ||
a: Map({ y: 50, z: 100 }), | ||
a: Map<string, number>({ y: 50, z: 100 }), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does this need an explicit type? Shouldn't The code written here without explicit types looks correct, and I'd be surprised to find a type error There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar feedback for all of the tests in this - are we missing something about the |
||
b: List([4, 5, 6]), | ||
c: Set([4, 5, 6]), | ||
}); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -760,9 +760,93 @@ declare namespace Immutable { | |
* not altered. | ||
*/ | ||
function Map<K, V>(collection?: Iterable<[K, V]>): Map<K, V>; | ||
function Map<R extends { [key in string | number | symbol]: unknown }>( | ||
obj: R | ||
): MapFromObject<R>; | ||
function Map<V>(obj: { [key: string]: V }): Map<string, V>; | ||
function Map<K extends string | symbol, V>(obj: { [P in K]?: V }): Map<K, V>; | ||
|
||
/** | ||
* Represent a Map constructed by an object | ||
* | ||
* @ignore | ||
*/ | ||
interface MapFromObject< | ||
R extends { [key in string | number | symbol]: unknown } | ||
> extends Map<keyof R, R[keyof R]> { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This extends is nice - I'm curious if it's providing the capability we need though given the test cases above |
||
/** | ||
* Returns the value associated with the provided key, or notSetValue if | ||
* the Collection does not contain this key. | ||
* | ||
* Note: it is possible a key may be associated with an `undefined` value, | ||
* so if `notSetValue` is not provided and this method returns `undefined`, | ||
* that does not guarantee the key was not found. | ||
*/ | ||
get<K extends keyof R>(key: K, notSetValue?: unknown): R[K]; | ||
get<NSV>(key: string, notSetValue: NSV): NSV; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be |
||
|
||
// https://github.com/microsoft/TypeScript/pull/39094 | ||
getIn<P extends ReadonlyArray<string | number | symbol>>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A nested structure may have keys that are not I think this needs to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I do not know why, but if I do that, calling |
||
searchKeyPath: [...P], | ||
notSetValue?: unknown | ||
): RetrievePath<R, P>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Part of what has made typing |
||
|
||
set<K extends keyof R>(key: K, value: R[K]): this; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. similar to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @EduardoAraujoB if key is not in Map({}).set('a', 'a'); // output Map({ a: 'a' }) @leebyron As said on slack I prefered the solution of replicating TypeScript objects: you need to define the interface before: const o = { a: 'a' };
o.b = 'b'; // Property 'b' does not exist on type '{ a: string; }'. |
||
|
||
update(updater: (value: this) => this): this; | ||
update<K extends keyof R>(key: K, updater: (value: R[K]) => R[K]): this; | ||
update<K extends keyof R, NSV>( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. similar feedback to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tried a fix for this, thanks! |
||
key: K, | ||
notSetValue: NSV, | ||
updater: (value: R[K]) => NSV | ||
): this; | ||
|
||
// Possible best type is MapFromObject<Omit<R, K>> but Omit seems to broke other function calls | ||
// and generate recursion error with other methods (update, merge, etc.) until those functions are defined in MapFromObject | ||
delete<K extends keyof R>( | ||
key: K | ||
): Extract<R[K], undefined> extends never ? never : this; | ||
remove<K extends keyof R>( | ||
key: K | ||
): Extract<R[K], undefined> extends never ? never : this; | ||
} | ||
|
||
// Loosely based off of this work. | ||
// https://github.com/immutable-js/immutable-js/issues/1462#issuecomment-584123268 | ||
|
||
/** @ignore */ | ||
type GetMapType<S> = S extends MapFromObject<infer T> ? T : S; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This implies that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that it will just return the type There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. On something that is not a I thing that: MapFromObject<{ m: Map<string, number>; l: List<number>; }>
GetMapType<m['m']> // Map<string, number>
GetMapType<m['l']> // List<number> Reading this particular line, the function only extract the initial object of a MapFromObject, that's all. So in a chain: MapFromObject<{
m: Map<string, List<number>>
}> Calling |
||
|
||
/** @ignore */ | ||
type Head<T extends ReadonlyArray<any>> = T extends [ | ||
infer H, | ||
...Array<unknown> | ||
] | ||
? H | ||
: never; | ||
|
||
/** @ignore */ | ||
type Tail<T extends ReadonlyArray<any>> = T extends [unknown, ...infer I] | ||
? I | ||
: Array<never>; | ||
|
||
/** @ignore */ | ||
type RetrievePathReducer< | ||
T, | ||
C, | ||
L extends ReadonlyArray<any> | ||
> = C extends keyof GetMapType<T> | ||
? L extends [] | ||
? GetMapType<T>[C] | ||
: RetrievePathReducer<GetMapType<T>[C], Head<L>, Tail<L>> | ||
: never; | ||
|
||
/** @ignore */ | ||
type RetrievePath< | ||
R, | ||
P extends ReadonlyArray<string | number | symbol> | ||
> = P extends [] ? P : RetrievePathReducer<R, Head<P>, Tail<P>>; | ||
|
||
interface Map<K, V> extends Collection.Keyed<K, V> { | ||
/** | ||
* The number of entries in this Map. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
import { Map, List } from 'immutable'; | ||
import { Map, List, MapFromObject } from 'immutable'; | ||
|
||
{ | ||
// #constructor | ||
|
@@ -16,13 +16,23 @@ import { Map, List } from 'immutable'; | |
Map([['a', 'a']]); | ||
|
||
// $ExpectType Map<number, string> | ||
Map( | ||
List<[number, string]>([[1, 'a']]) | ||
); | ||
Map(List<[number, string]>([[1, 'a']])); | ||
|
||
// $ExpectType Map<string, number> | ||
// $ExpectType MapFromObject<{ a: number; }> | ||
Map({ a: 1 }); | ||
|
||
// $ExpectType MapFromObject<{ a: number; b: string; }> | ||
Map({ a: 1, b: 'b' }); | ||
|
||
// $ExpectType MapFromObject<{ a: MapFromObject<{ b: MapFromObject<{ c: number; }>; }>; }> | ||
Map({ a: Map({ b: Map({ c: 3 }) }) }); | ||
|
||
// $ExpectError | ||
Map<{ a: string }>({ a: 1 }); | ||
|
||
// $ExpectError | ||
Map<{ a: string }>({ a: 'a', b: 'b' }); | ||
|
||
// No longer works in typescript@>=3.9 | ||
// // $ExpectError - TypeScript does not support Lists as tuples | ||
// Map(List([List(['a', 'b'])])); | ||
|
@@ -61,6 +71,31 @@ import { Map, List } from 'immutable'; | |
|
||
// $ExpectError | ||
Map<number, number>().get<number>(4, 'a'); | ||
|
||
// $ExpectType number | ||
Map({ a: 4, b: true }).get('a'); | ||
|
||
// $ExpectType boolean | ||
Map({ a: 4, b: true }).get('b'); | ||
|
||
// $ExpectType boolean | ||
Map({ a: Map({ b: true }) }) | ||
.get('a') | ||
.get('b'); | ||
|
||
// $ExpectError | ||
Map({ a: 4 }).get('b'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add |
||
} | ||
jdeniau marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
{ | ||
// Minimum TypeScript Version: 4.1 | ||
// #getIn | ||
|
||
// $ExpectType number | ||
Map({ a: 4, b: true }).getIn(['a']); | ||
|
||
// $ExpectType number | ||
Map({ a: Map({ b: Map({ c: Map({ d: 4 }) }) }) }).getIn(['a', 'b', 'c', 'd']); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add getIn tests where nested Map are constructed by other means (eg not only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunatly, it's only working for |
||
|
||
{ | ||
|
@@ -80,6 +115,28 @@ import { Map, List } from 'immutable'; | |
|
||
// $ExpectType Map<number, string | number> | ||
Map<number, number | string>().set(0, 'a'); | ||
|
||
// $ExpectError | ||
Map({ a: 1 }).set('b', 'b'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm surprised by this, I'd expect either There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See comment above about the consistency with plain objects in TS |
||
|
||
// $ExpectType MapFromObject<{ a: number; b?: string | undefined; }> | ||
Map<{ a: number; b?: string; }>({ a: 1 }).set('b', 'b'); | ||
|
||
// $ExpectType MapFromObject<{ a: number; b?: string | undefined; }> | ||
Map<{ a: number; b?: string; }>({ a: 1 }).set('b', undefined); | ||
|
||
// $ExpectType number | ||
Map<{ a: number; b?: string }>({ a: 1 }).set('b', 'b').get('a'); | ||
|
||
// $ExpectType string | undefined | ||
Map<{ a: number; b?: string }>({ a: 1 }).set('b', 'b').get('b'); | ||
|
||
let customer = Map<{ phone: string | number }>({ | ||
phone: 'bar', | ||
}); | ||
|
||
// $ExpectType MapFromObject<{ phone: string | number; }> | ||
customer = customer.set('phone', 8); | ||
} | ||
|
||
{ | ||
|
@@ -97,6 +154,21 @@ import { Map, List } from 'immutable'; | |
|
||
// $ExpectError | ||
Map<number, number>().delete('a'); | ||
|
||
// $ExpectType never | ||
Map({ a: 1, b: 'b' }).delete('b'); | ||
|
||
// $ExpectType MapFromObject<{ a: number; b?: string | undefined; }> | ||
Map<{ a: number; b?: string; }>({ a: 1, b: 'b' }).delete('b'); | ||
|
||
// $ExpectType MapFromObject<{ a?: number | undefined; b?: string | undefined; }> | ||
Map<{ a?: number; b?: string; }>({ a: 1, b: 'b' }).remove('b').delete('a'); | ||
|
||
// $ExpectType number | ||
Map<{ a: number; b?: string; }>({ a: 1, b: 'b' }).remove('b').get('a'); | ||
|
||
// $ExpectType: string | undefined | ||
Map<{ a: number; b?: string; }>({ a: 1, b: 'b' }).remove('b').get('b'); | ||
} | ||
|
||
{ | ||
|
@@ -176,6 +248,24 @@ import { Map, List } from 'immutable'; | |
|
||
// $ExpectError | ||
Map<number, number>().update(1, 10, (v: number | undefined) => v + 'a'); | ||
|
||
// $ExpectError | ||
Map({ a: 1, b: 'b' }).update('c', (v) => v); | ||
|
||
// $ExpectType MapFromObject<{ a: number; b: string; }> | ||
Map({ a: 1, b: 'b' }).update('b', (v) => v.toUpperCase()); | ||
|
||
// $ExpectType MapFromObject<{ a: number; b: string; }> | ||
Map({ a: 1, b: 'b' }).update('b', 'NSV', (v) => v.toUpperCase()); | ||
|
||
// $ExpectError | ||
Map({ a: 1, b: 'b' }).update((v) => ({ a: 'a' })); | ||
|
||
// $ExpectType MapFromObject<{ a: number; b: string; }> | ||
Map({ a: 1, b: 'b' }).update((v) => v.set('a', 2).set('b', 'B')); | ||
|
||
// $ExpectError | ||
Map({ a: 1, b: 'b' }).update((v) => v.set('c', 'c')); | ||
} | ||
|
||
{ | ||
|
@@ -280,20 +370,14 @@ import { Map, List } from 'immutable'; | |
// #flatMap | ||
|
||
// $ExpectType Map<number, number> | ||
Map< | ||
number, | ||
number | ||
>().flatMap((value: number, key: number, iter: Map<number, number>) => [ | ||
[0, 1], | ||
]); | ||
Map<number, number>().flatMap( | ||
(value: number, key: number, iter: Map<number, number>) => [[0, 1]] | ||
); | ||
|
||
// $ExpectType Map<string, string> | ||
Map< | ||
number, | ||
number | ||
>().flatMap((value: number, key: number, iter: Map<number, number>) => [ | ||
['a', 'b'], | ||
]); | ||
Map<number, number>().flatMap( | ||
(value: number, key: number, iter: Map<number, number>) => [['a', 'b']] | ||
); | ||
|
||
// $ExpectType Map<number, number> | ||
Map<number, number>().flatMap<number, number>( | ||
|
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 confused - is this not currently failing? Why does this PR cause this to start producing errors?