Improve Typescript definition for Map (#1841)

Co-authored-by: Michael Bullington <michael.bullington@mapbox.com>
immutable-js · Aug 24, 2023 · c25fac2 · c25fac2
1 parent 2306527
commit c25fac2
Show file tree

Hide file tree

Showing 12 changed files with 392 additions and 62 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -8,6 +8,76 @@ Dates are formatted as YYYY-MM-DD.
 
 ## Unreleased
 
+### Changed
+
+### Improve TypeScript definition for `Map`
+
+Imagine the following code
+
+```ts
+const m = Map({ length: 3, 1: 'one' });
+```
+
+This was previously typed as `Map<string, string | number>`
+
+and return type of `m.get('length')` or `m.get('inexistant')` was typed as `string | number | undefined`.
+
+This made `Map` really unusable with TypeScript.
+
+Now the Map is typed like this:
+
+```ts
+MapOf<{
+    length: number;
+    1: string;
+}>
+```
+
+and the return type of `m.get('length')` is typed as `number`.
+
+The return of `m.get('inexistant')` throw the TypeScript error:
+
+> Argument of type '"inexistant"' is not assignable to parameter of type '1 | "length"
+
+#### If you want to keep the old definition
+
+**This is a minor BC for TS users**, so if you want to keep the old definition, you can declare you Map like this:
+
+```ts
+const m = Map<string, string | number>({ length: 3, 1: 'one' });
+```
+
+#### If you need to type the Map with a larger definition
+
+You might want to declare a wider definition, you can type your Map like this:
+
+```ts
+type MyMapType = {
+  length: number;
+  1: string | null;
+  optionalProperty?: string;
+};
+const m = Map<MyMapType>({ length: 3, 1: 'one' });
+```
+
+Keep in mind that the `MapOf` will try to be consistant with the simple TypeScript object, so you can not do this:
+
+```ts
+Map({ a: 'a' }).set('b', 'b');
+Map({ a: 'a' }).delete('a');
+```
+
+Like a simple object, it will only work if the type is forced:
+
+```ts
+Map<{ a: string; b?: string }>({ a: 'a' }).set('b', 'b'); // b is forced in type and optional
+Map<{ a?: string }>({ a: 'a' }).delete('a'); // you can only delete an optional key
+```
+
+#### Are all `Map` methods implemented ?
+
+For now, only `get`, `getIn`, `set`, `update`, `delete` and `remove` methods are implemented. All other methods will fallback to the basic `Map` definition. Other method definition will be added later, but as some might be really complex, we prefer the progressive enhancement on the most used functions.
+
 ## [4.3.3] - 2023-08-23
 
 - [typescript] manage to handle toJS circular reference. [#1932](https://github.com/immutable-js/immutable-js/pull/1932) by [@jdeniau](https://github.com/jdeniau)

diff --git a/__tests__/Map.ts b/__tests__/Map.ts
@@ -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
     expect(m.get('1')).toBe('one');
     expect(m.toJS()).toEqual({ length: 3, 1: 'one' });
   });
@@ -424,7 +428,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());
   });

diff --git a/__tests__/merge.ts b/__tests__/merge.ts
@@ -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 })
     );
@@ -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)
@@ -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);
     });
@@ -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);
   });

diff --git a/package.json b/package.json
@@ -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",

diff --git a/type-definitions/immutable.d.ts b/type-definitions/immutable.d.ts
@@ -102,6 +102,11 @@ declare namespace Immutable {
       {
         [key in keyof R]: DeepCopy<R[key]>;
       }
+    : T extends MapOf<infer R>
+    ? // convert MapOf to DeepCopy plain JS object
+      {
+        [key in keyof R]: DeepCopy<R[key]>;
+      }
     : T extends Collection.Keyed<infer KeyedKey, infer V>
     ? // convert KeyedCollection to DeepCopy plain JS object
       {
@@ -829,9 +834,96 @@ 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
+  ): MapOf<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 MapOf<R extends { [key in string | number | symbol]: unknown }>
+    extends Map<keyof R, R[keyof R]> {
+    /**
+     * 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>>(
+      searchKeyPath: [...P],
+      notSetValue?: unknown
+    ): RetrievePath<R, P>;
+
+    set<K extends keyof R>(key: K, value: R[K]): this;
+
+    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 extends R[K]>(
+      key: K,
+      notSetValue: NSV,
+      updater: (value: R[K]) => R[K]
+    ): this;
+
+    // Possible best type is MapOf<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 MapOf
+    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;
+
+    toJS(): { [K in keyof R]: DeepCopy<R[K]> };
+
+    toJSON(): { [K in keyof R]: R[K] };
+  }
+
+  // Loosely based off of this work.
+  // https://github.com/immutable-js/immutable-js/issues/1462#issuecomment-584123268
+
+  /** @ignore */
+  type GetMapType<S> = S extends MapOf<infer T> ? T : S;
+
+  /** @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.
@@ -1049,16 +1141,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
@@ -1078,10 +1171,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
@@ -1112,9 +1209,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
@@ -1582,15 +1682,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

diff --git a/type-definitions/ts-tests/covariance.ts b/type-definitions/ts-tests/covariance.ts
@@ -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 MapOf<{ b: B; }>
 mapOfA = Map({ b: new B() });
 
 // $ExpectError

diff --git a/type-definitions/ts-tests/from-js.ts b/type-definitions/ts-tests/from-js.ts
@@ -18,7 +18,7 @@ import { fromJS, List, Map } from 'immutable';
   // $ExpectType Map<"b" | "a" | "c", number>
   fromJS({a: 0, b: 1, c: 2});
 
-  // $ExpectType Map<string, number>
+  // $ExpectType MapOf<{ a: number; b: number; c: number; }>
   fromJS(Map({a: 0, b: 1, c: 2}));
 
   // $ExpectType List<Map<"a", number>>