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.

Sign up for GitHub

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

Jump to bottom

Improve Typescript definition for Map #1841

Merged
merged 32 commits into from Aug 24, 2023
Merged

Improve Typescript definition for Map #1841

Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
47c78b4
try implementing TS Map as object definition
jdeniau Jun 29, 2021
0543432
working version of ObjectLikeMap with `.get()`
jdeniau Jul 2, 2021
b5d874a
start `getIn` implementation for ObjectLikeMap
jdeniau Jul 7, 2021
8cf0983
try to fix and add set definition
jdeniau Jul 15, 2021
f7c1f33
fix getIn using variadic tuple types
mbullington Jul 16, 2021
9ec983f
try implementing TS Map as object definition
jdeniau Jun 29, 2021
4d248af
working version of ObjectLikeMap with `.get()`
jdeniau Jul 2, 2021
720b847
start `getIn` implementation for ObjectLikeMap
jdeniau Jul 7, 2021
c762557
try to fix and add set definition
jdeniau Jul 15, 2021
3705794
Merge branch 'jd-feat-tsMapObject' of github.com:jdeniau/immutable-js…
mbullington Jul 19, 2021
82e957c
Merge remote-tracking branch 'origin/main' into jd-feat-tsMapObject
jdeniau Oct 15, 2021
43f6590
fix linting
jdeniau Oct 15, 2021
72f60e9
Remove weird comment
jdeniau Oct 15, 2021
b58b9d6
Fix website build
jdeniau Oct 15, 2021
b0faf97
add test for `.set`
jdeniau Oct 15, 2021
42170d6
ObjectLikeMap → MapFromObject
jdeniau Oct 21, 2021
790773a
Update CHANGELOG
jdeniau Oct 21, 2021
3e160f8
MapFromObject: add `remove` and `delete` + better type definition for…
jdeniau Oct 27, 2021
a26d4ec
be consistant with standard objects
jdeniau Oct 27, 2021
d112b0c
reset Map and OrderedMap return to `this` due to latest change on this
jdeniau Oct 27, 2021
f225394
ignore ConditionalType in website generation
jdeniau Oct 28, 2021
89b83a0
Merge branch 'main' into jd-feat-tsMapObject
jdeniau Oct 28, 2021
0d57333
Merge remote-tracking branch 'origin/main' into jd-feat-tsMapObject
jdeniau Nov 12, 2021
d627ad3
implement `.update` thanks to @mbullington
jdeniau Nov 12, 2021
0d5d6c8
fox get + add some tests
jdeniau Nov 21, 2021
ee1eca1
Make sure merge works properly with MapFromObject
mbullington Dec 1, 2021
47be7ee
fix update method
mbullington Dec 1, 2021
0e9da4d
Merge remote-tracking branch 'origin/main' into jd-feat-tsMapObject
jdeniau Sep 28, 2022
ad0356b
Merge remote-tracking branch 'origin/main' into jd-feat-tsMapObject
jdeniau Feb 10, 2023
beeac64
Rename `MapFromObject` to `MapOf` to match `RecordOf`
jdeniau Feb 10, 2023
def6f09
Merge remote-tracking branch 'origin/main' into jd-feat-tsMapObject
jdeniau Aug 23, 2023
c302576
implement toJS and toJSON due to recent fixes in main branch
jdeniau Aug 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
332 changes: 178 additions & 154 deletions CHANGELOG.md
View file
Open in desktop

Large diffs are not rendered by default.

6 changes: 5 additions & 1 deletion __tests__/Map.ts
View file
Open in desktop
Expand Up @@ -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');
Comment on lines +63 to 68
Copy link
Collaborator

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?

});

Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should not be a type error. Similarly, we should ensure that m.get(1) is well typed to return NSV (or be a type error if that arg is omitted)

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' });
});
Expand Down Expand Up @@ -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());
});
Expand Down
20 changes: 10 additions & 10 deletions __tests__/merge.ts
View file
Open in desktop
Expand Up @@ -18,13 +18,13 @@ describe('merge', () => {

it('can merge in an explicitly undefined value', () => {
const m1 = Map({ a: 1, b: 2 });
const m2 = Map({ a: undefined as any });
const m2 = Map({ a: undefined });
expect(m1.merge(m2)).toEqual(Map({ a: undefined, b: 2 }));
});

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 })
);
Expand All @@ -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)
Expand Down Expand Up @@ -274,8 +274,8 @@ describe('merge', () => {
expect(mergeDeep(aObject, bObject)).toEqual(bObject);
expect(mergeDeep(bObject, aObject)).toEqual(aObject);

const aMap = Map({ a: value1 }) as Map<unknown, unknown>;
const bMap = Map({ a: value2 }) as Map<unknown, unknown>;
const aMap = Map({ a: value1 });
const bMap = Map({ a: value2 });
expect(aMap.mergeDeep(bMap).equals(bMap)).toBe(true);
expect(bMap.mergeDeep(aMap).equals(aMap)).toBe(true);
});
Expand Down Expand Up @@ -312,15 +312,15 @@ describe('merge', () => {
const bObject = { a: value2 };
expect(mergeDeep(aObject, bObject)).toEqual({ a: result });

const aMap = Map({ a: value1 }) as Map<unknown, unknown>;
const aMap = Map({ a: value1 });
const bMap = Map({ a: value2 });
expect(aMap.mergeDeep(bMap)).toEqual(Map({ a: result }));
});
}

it('Map#mergeDeep replaces nested List with Map and Map with List', () => {
const a = Map({ a: List([Map({ x: 1 })]) }) as Map<unknown, unknown>;
const b = Map({ a: Map([[0, Map({ y: 2 })]]) }) as Map<unknown, unknown>;
const a = Map({ a: List([Map({ x: 1 })]) });
const b = Map({ a: Map([[0, Map({ y: 2 })]]) });
expect(a.mergeDeep(b).equals(b)).toBe(true);
expect(b.mergeDeep(a).equals(a)).toBe(true);
});
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Expand Up @@ -63,7 +63,7 @@
"lint:js": "eslint \"{__tests__,src,website/src}/**/*.js\"",
"lint:ts": "tslint --project type-definitions/tsconfig.json \"{__tests__,src,type-definitions,website/src}/**/*.{ts,tsx}\"",
"type-check": "run-s type-check:*",
"type-check:ts": "tsc --project type-definitions/tsconfig.json && tsc --project __tests__/tsconfig.json && dtslint type-definitions/ts-tests",
"type-check:ts": "tsc --project type-definitions/tsconfig.json && tsc --project __tests__/tsconfig.json && dtslint type-definitions/ts-tests --localTs node_modules/typescript/lib",
"type-check:flow": "flow check type-definitions/flow-tests --include-warnings",
"build": "run-s build:*",
"build:clean": "rimraf dist",
Expand Down
137 changes: 123 additions & 14 deletions type-definitions/immutable.d.ts
View file
Open in desktop
Expand Up @@ -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]> {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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: any, notSetValue: NSV): NSV;

// https://github.com/microsoft/TypeScript/pull/39094
getIn<P extends ReadonlyArray<string | number | symbol>>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

A nested structure may have keys that are not string | number | symbol

I think this needs to be ReadonlyArray<any> to account for all possible key types of a typical Map

Copy link
Member Author

Choose a reason for hiding this comment

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

I do not know why, but if I do that, calling getIn(['a', 'b', 'c', 'd']) is not handled and fallback to be a never type

searchKeyPath: [...P],
notSetValue?: unknown
): RetrievePath<R, P>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Part of what has made typing getIn correctly challenging is that we don't necessarily know the nested types within. The recursive type you've defined here gets way closer, but if it's correct it needs to handle all possible cases that getIn operates on in the nested layers. Then there's no reason it can't be used in all getIn methods


set<K extends keyof R>(key: K, value: R[K]): this;
Copy link
Collaborator

Choose a reason for hiding this comment

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

similar to get above, I think this needs another definition which covers where key is not in keyof R with an appropriate return type

Copy link

Choose a reason for hiding this comment

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

if key is not in keyof R immutable will return a undefined right?

Copy link
Member Author

Choose a reason for hiding this comment

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

@EduardoAraujoB if key is not in keyof R, then technically a new Map is returned:

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>(
Copy link
Collaborator

Choose a reason for hiding this comment

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

similar feedback to set above, but perhaps you can include similar logic you have for get to provide the correct value type provided to the updater function

Copy link

Choose a reason for hiding this comment

The 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;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This implies that getIn will be used on something that is MapFromObject all the way down. What happens when it includes a typical Map or List?

Copy link

Choose a reason for hiding this comment

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

I think that it will just return the type Map or List, am I correct? @jdeniau

Copy link
Member Author

Choose a reason for hiding this comment

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

On something that is not a MapFromObject, it will return the type directly. I do not really know in the end what will happen though. Maybe @mbullington who coded this part ?

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 RetrievePath<R, ['m', 'some-string', number]> should return number


/** @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.
Expand Down Expand Up @@ -980,16 +1064,17 @@ declare namespace Immutable {
*/
merge<KC, VC>(
...collections: Array<Iterable<[KC, VC]>>
): Map<K | KC, V | VC>;
): Map<K | KC, Exclude<V, VC> | VC>;
merge<C>(
...collections: Array<{ [key: string]: C }>
): Map<K | string, V | C>;
): Map<K | string, Exclude<V, C> | C>;

concat<KC, VC>(
...collections: Array<Iterable<[KC, VC]>>
): Map<K | KC, V | VC>;
): Map<K | KC, Exclude<V, VC> | VC>;
concat<C>(
...collections: Array<{ [key: string]: C }>
): Map<K | string, V | C>;
): Map<K | string, Exclude<V, C> | C>;

/**
* Like `merge()`, `mergeWith()` returns a new Map resulting from merging
Expand All @@ -1009,10 +1094,14 @@ declare namespace Immutable {
*
* Note: `mergeWith` can be used in `withMutations`.
*/
mergeWith(
merger: (oldVal: V, newVal: V, key: K) => V,
...collections: Array<Iterable<[K, V]> | { [key: string]: V }>
): this;
mergeWith<KC, VC, VCC>(
merger: (oldVal: V, newVal: VC, key: K) => VCC,
...collections: Array<Iterable<[KC, VC]>>
): Map<K | KC, V | VC | VCC>;
mergeWith<C, CC>(
merger: (oldVal: V, newVal: C, key: string) => CC,
...collections: Array<{ [key: string]: C }>
): Map<K | string, V | C | CC>;

/**
* Like `merge()`, but when two compatible collections are encountered with
Expand Down Expand Up @@ -1043,9 +1132,12 @@ declare namespace Immutable {
*
* Note: `mergeDeep` can be used in `withMutations`.
*/
mergeDeep(
...collections: Array<Iterable<[K, V]> | { [key: string]: V }>
): this;
mergeDeep<KC, VC>(
...collections: Array<Iterable<[KC, VC]>>
): Map<K | KC, V | VC>;
mergeDeep<C>(
...collections: Array<{ [key: string]: C }>
): Map<K | string, V | C>;

/**
* Like `mergeDeep()`, but when two non-collections or incompatible
Expand Down Expand Up @@ -1500,15 +1592,32 @@ declare namespace Immutable {
*/
merge<KC, VC>(
...collections: Array<Iterable<[KC, VC]>>
): OrderedMap<K | KC, V | VC>;
): OrderedMap<K | KC, Exclude<V, VC> | VC>;
merge<C>(
...collections: Array<{ [key: string]: C }>
): OrderedMap<K | string, V | C>;
): OrderedMap<K | string, Exclude<V, C> | C>;

concat<KC, VC>(
...collections: Array<Iterable<[KC, VC]>>
): OrderedMap<K | KC, V | VC>;
): OrderedMap<K | KC, Exclude<V, VC> | VC>;
concat<C>(
...collections: Array<{ [key: string]: C }>
): OrderedMap<K | string, Exclude<V, C> | C>;

mergeWith<KC, VC, VCC>(
merger: (oldVal: V, newVal: VC, key: K) => VCC,
...collections: Array<Iterable<[KC, VC]>>
): OrderedMap<K | KC, V | VC | VCC>;
mergeWith<C, CC>(
merger: (oldVal: V, newVal: C, key: string) => CC,
...collections: Array<{ [key: string]: C }>
): OrderedMap<K | string, V | C | CC>;

mergeDeep<KC, VC>(
...collections: Array<Iterable<[KC, VC]>>
): OrderedMap<K | KC, V | VC>;
mergeDeep<C>(
...collections: Array<{ [key: string]: C }>
): OrderedMap<K | string, V | C>;

// Sequence algorithms
Expand Down
2 changes: 1 addition & 1 deletion type-definitions/ts-tests/covariance.ts
View file
Open in desktop
Expand Up @@ -34,7 +34,7 @@ let listOfC: List<C> = listOfB;
declare var mapOfB: Map<string, B>;
let mapOfA: Map<string, A> = mapOfB;

// $ExpectType Map<string, B>
// $ExpectType MapFromObject<{ b: B; }>
mapOfA = Map({ b: new B() });

// $ExpectError
Expand Down