Skip to content

Adds support for symbols as keys in Maps. #1392

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

Closed
wants to merge 11 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
76 changes: 76 additions & 0 deletions __tests__/Map.ts
Original file line number Diff line number Diff line change
Expand Up @@ -473,4 +473,80 @@ describe('Map', () => {
Map([[a, Map([[b, Map([[c, 10], [d, 2], [e, 20], [f, 30], [g, 40]])]])]])
);
});

it('supports Symbol keys from object literal', () => {
const a = Symbol('A');
const b = Symbol('B');
const c = Symbol('C');
const m = Map.fromOwnEntries({ [a]: 'A', [b]: 'B', [c]: 'C' });
expect(m.size).toBe(3);
expect(m.get(a)).toBe('A');
expect(m.get(b)).toBe('B');
expect(m.get(c)).toBe('C');
});

it('Symbol keys from object literal are unique', () => {
const a = Symbol('FooBar');
const b = Symbol('FooBar');
const m = Map.fromOwnEntries({ [a]: 'FizBuz', [b]: 'FooBar' });
expect(m.size).toBe(2);
expect(m.get(a)).toBe('FizBuz');
expect(m.get(b)).toBe('FooBar');
});

it('creates nested Map from object with nested Symbol keys', () => {
const a = Symbol('a');
const b = Symbol('b');
const c = Symbol('c');
const e = Symbol('e');
const f = Symbol('f');
const g = Symbol('g');
const map = Map.fromOwnEntries({
[a]: { [b]: { [c]: 10, [e]: 20 }, [f]: 30 },
[g]: 40,
});

expect(
map
.get(a)
.get(b)
.get(c)
).toEqual(10);
expect(
map
.get(a)
.get(b)
.get(e)
).toEqual(20);
expect(map.get(a).get(f)).toEqual(30);
expect(map.get(g)).toEqual(40);
});

it('mergeDeep Map from object literal', () => {
const a = Symbol('a');
const b = Symbol('b');
const c = Symbol('c');
const d = Symbol('d');
const e = Symbol('e');
const f = Symbol('f');
const g = Symbol('g');
const m1 = Map.fromOwnEntries({ [a]: { [b]: { [c]: 1, [d]: 2 } } });
const m2 = Map.fromOwnEntries({
[a]: { [b]: { [c]: 10, [e]: 20 }, [f]: 30 },
[g]: 40,
});
const expected = Map.fromOwnEntries({
[a]: { [b]: { [c]: 10, [d]: 2, [e]: 20 }, [f]: 30 },
[g]: 40,
});
const actual = m1.mergeDeep(m2);
expect(actual).toEqual(expected);

const actualJs = actual.toJS();
expect(actualJs[a][b][c]).toEqual(10);
expect(actualJs[a][b][d]).toEqual(2);
expect(actualJs[a][b][e]).toEqual(20);
expect(actualJs[a][f]).toEqual(30);
expect(actualJs[g]).toEqual(40);
});
});
11 changes: 11 additions & 0 deletions src/Map.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,17 @@ export class Map extends KeyedCollection {
});
}

static fromOwnEntries(source) {
return Map(
Reflect.ownKeys(source).map(key => [
key,
typeof source[key] === 'object'
? Map.fromOwnEntries(source[key])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I don't think this recursion is something we want to encourage - that would result in a deep exploration of a data structure which could be very slow, that might be surprising to someone who thought this worked like the other constructors which are all shallow - if an inner value is intended to also be a map, then explicitly calling this in user code will both make that use case clear, avoid unnecessary recursion and make ts/flow more accurate.

If I understand correctly, this function should just become:

return Map(Reflect.ownKeys(source).map(key => [key, source[key]]))

Given the simplicity of this function, how it is likely to be used less often than the standard constructor, how tree-shaking often does not yet remove static methods, and your pointing out that your use case may desire recursion to be more ergonomic, do you think it should live in the core library directly? I'm thinking that this could probably be either a single function in someone's own codebase or a micro library on npm (immutable-map-from-own-entries)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i completely agree. i'm ok with closing this now, unless you think there is something else to discuss. thanks for going on this adventure with me : )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks again. i'm closing this request now.

: source[key],
])
);
}

toString() {
return this.__toString('Map {', '}');
}
Expand Down
42 changes: 42 additions & 0 deletions type-definitions/Immutable.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -717,6 +717,48 @@ declare module Immutable {
* @deprecated Use Map([ [ 'k', 'v' ] ]) or Map({ k: 'v' })
*/
function of(...keyValues: Array<any>): Map<any, any>;

/**
* Creates a new Map from an object.
*
* <!-- runkit:activate -->
* ```js
* const { Map } = require('immutable')
* Map.fromOwnEntries({
* foo: 'bar',
* fiz: 'buz'
* })
* // Map { "foo": "bar", "fiz": "buz" }
* ```
*
* Nested objects are recursively converted into instances of Map.
*
* <!-- runkit:activate -->
* ```js
* const { Map } = require('immutable')
* Map.fromOwnEntries({
* foo: 'bar',
* fiz: {
* buz: 'qux'
* }
* })
* // Map { "foo": "bar", "fiz": Map { "buz": "qux" } }
* ```
*
* Symbol keys are supported.
*
* <!-- runkit:activate -->
* ```js
* const { Map } = require('immutable')
* const fiz = Symbol('fiz');
* Map.fromOwnEntries({
* foo: 'bar',
* [fiz]: 'buz'
* })
* // Map { "foo": "bar", "Symbol(fiz)": "buz" }
* ```
*/
function fromOwnEntries(obj: any): Map<any, any>;
}

/**
Expand Down
1 change: 1 addition & 0 deletions type-definitions/immutable.js.flow
Original file line number Diff line number Diff line change
Expand Up @@ -868,6 +868,7 @@ declare class Map<K, +V> extends KeyedCollection<K, V> mixins UpdatableInCollect

size: number;

fromOwnEntries(obj: mixed): Map<K, V>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This would be static, and ideally the K, V are mapped to something on the input, see static <K, V>(...) above, which is the standard constructor.

set<K_, V_>(key: K_, value: V_): Map<K | K_, V | V_>;
delete(key: K): this;
remove(key: K): this;
Expand Down