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.

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

Merged
merged 32 commits into from Aug 24, 2023
Merged
Show file tree
Hide file tree
Changes from all 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
70 changes: 70 additions & 0 deletions CHANGELOG.md
Expand Up @@ -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)
Expand Down
6 changes: 5 additions & 1 deletion __tests__/Map.ts
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 @@ -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());
});
Expand Down
20 changes: 10 additions & 10 deletions __tests__/merge.ts
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
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
145 changes: 131 additions & 14 deletions type-definitions/immutable.d.ts
Expand Up @@ -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
{
Expand Down Expand Up @@ -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>>(
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

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 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.
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion type-definitions/ts-tests/covariance.ts
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 MapOf<{ b: B; }>
mapOfA = Map({ b: new B() });

// $ExpectError
Expand Down
2 changes: 1 addition & 1 deletion type-definitions/ts-tests/from-js.ts
Expand Up @@ -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>>
Expand Down