From a66a1561a8c16c4abf2f406af74c3aa505d4e0a7 Mon Sep 17 00:00:00 2001 From: Lee Byron Date: Wed, 4 Oct 2017 16:05:33 -0700 Subject: [PATCH] BREAKING: KeyedCollection.toArray() returns array of tuples. This fixes a long standing API quirk where `x.toArray()` had different behavior from `[...x]` or `Array.from(x)`. This is breaking since previously Map, and other keyed collections would provide an array of only values from `.toArray()`, to fix code that relies on this behavior, convert `x.toArray()` to `x.valueSeq().toArray()`. Likewise, for code that was working around this behavior before, `x.entrySeq().toArray()` can typically be replaced with `x.toArray()`. Fixes #333 --- __tests__/ObjectSeq.ts | 6 ++--- __tests__/OrderedMap.ts | 12 +++++----- __tests__/concat.ts | 7 ++++-- __tests__/groupBy.ts | 2 +- src/CollectionImpl.js | 7 ++++-- src/Operations.js | 1 + type-definitions/Immutable.d.ts | 37 ++++++++++++++++++++++++++++-- type-definitions/immutable.js.flow | 5 +++- 8 files changed, 60 insertions(+), 17 deletions(-) diff --git a/__tests__/ObjectSeq.ts b/__tests__/ObjectSeq.ts index ac214dd0bf..8e5e8d5e61 100644 --- a/__tests__/ObjectSeq.ts +++ b/__tests__/ObjectSeq.ts @@ -32,13 +32,13 @@ describe('ObjectSequence', () => { it('is reversable', () => { let i = Seq({a: 'A', b: 'B', c: 'C'}); let k = i.reverse().toArray(); - expect(k).toEqual(['C', 'B', 'A']); + expect(k).toEqual([['c', 'C'], ['b', 'B'], ['a', 'A']]); }); - it('can double reversable', () => { + it('is double reversable', () => { let i = Seq({a: 'A', b: 'B', c: 'C'}); let k = i.reverse().reverse().toArray(); - expect(k).toEqual(['A', 'B', 'C']); + expect(k).toEqual([['a', 'A'], ['b', 'B'], ['c', 'C']]); }); it('can be iterated', () => { diff --git a/__tests__/OrderedMap.ts b/__tests__/OrderedMap.ts index fabc393369..4c3f78f315 100644 --- a/__tests__/OrderedMap.ts +++ b/__tests__/OrderedMap.ts @@ -16,7 +16,7 @@ describe('OrderedMap', () => { expect(m.get('a')).toBe('A'); expect(m.get('b')).toBe('B'); expect(m.get('c')).toBe('C'); - expect(m.toArray()).toEqual(['C', 'B', 'A']); + expect(m.toArray()).toEqual([['c', 'C'], ['b', 'B'], ['a', 'A']]); }); it('constructor provides initial values', () => { @@ -25,7 +25,7 @@ describe('OrderedMap', () => { expect(m.get('b')).toBe('B'); expect(m.get('c')).toBe('C'); expect(m.size).toBe(3); - expect(m.toArray()).toEqual(['A', 'B', 'C']); + expect(m.toArray()).toEqual([['a', 'A'], ['b', 'B'], ['c', 'C']]); }); it('provides initial values in a mixed order', () => { @@ -34,7 +34,7 @@ describe('OrderedMap', () => { expect(m.get('b')).toBe('B'); expect(m.get('c')).toBe('C'); expect(m.size).toBe(3); - expect(m.toArray()).toEqual(['C', 'B', 'A']); + expect(m.toArray()).toEqual([['c', 'C'], ['b', 'B'], ['a', 'A']]); }); it('constructor accepts sequences', () => { @@ -44,7 +44,7 @@ describe('OrderedMap', () => { expect(m.get('b')).toBe('B'); expect(m.get('c')).toBe('C'); expect(m.size).toBe(3); - expect(m.toArray()).toEqual(['C', 'B', 'A']); + expect(m.toArray()).toEqual([['c', 'C'], ['b', 'B'], ['a', 'A']]); }); it('maintains order when new keys are set', () => { @@ -53,7 +53,7 @@ describe('OrderedMap', () => { .set('Z', 'zebra') .set('A', 'antelope'); expect(m.size).toBe(2); - expect(m.toArray()).toEqual(['antelope', 'zebra']); + expect(m.toArray()).toEqual([['A', 'antelope'], ['Z', 'zebra']]); }); it('resets order when a keys is deleted', () => { @@ -63,7 +63,7 @@ describe('OrderedMap', () => { .remove('A') .set('A', 'antelope'); expect(m.size).toBe(2); - expect(m.toArray()).toEqual(['zebra', 'antelope']); + expect(m.toArray()).toEqual([['Z', 'zebra'], ['A', 'antelope']]); }); it('removes correctly', () => { diff --git a/__tests__/concat.ts b/__tests__/concat.ts index 4766b3cd32..68f35102c5 100644 --- a/__tests__/concat.ts +++ b/__tests__/concat.ts @@ -125,8 +125,11 @@ describe('concat', () => { it('iterates repeated keys', () => { let a = Seq({a: 1, b: 2, c: 3}); expect(a.concat(a, a).toObject()).toEqual({a: 1, b: 2, c: 3}); - expect(a.concat(a, a).toArray()).toEqual([1, 2, 3, 1, 2, 3, 1, 2, 3]); + expect(a.concat(a, a).valueSeq().toArray()).toEqual([1, 2, 3, 1, 2, 3, 1, 2, 3]); expect(a.concat(a, a).keySeq().toArray()).toEqual(['a', 'b', 'c', 'a', 'b', 'c', 'a', 'b', 'c']); + expect(a.concat(a, a).toArray()).toEqual( + [['a', 1], ['b', 2], ['c', 3], ['a', 1], ['b', 2], ['c', 3], ['a', 1], ['b', 2], ['c', 3]], + ); }); it('lazily reverses un-indexed sequences', () => { @@ -145,7 +148,7 @@ describe('concat', () => { let a = Seq([1, 2, 3]).filter(x => true); expect(a.size).toBe(undefined); // Note: lazy filter does not know what size in O(1). expect(a.concat(a, a).toKeyedSeq().reverse().size).toBe(undefined); - expect(a.concat(a, a).toKeyedSeq().reverse().entrySeq().toArray()).toEqual( + expect(a.concat(a, a).toKeyedSeq().reverse().toArray()).toEqual( [[8, 3], [7, 2], [6, 1], [5, 3], [4, 2], [3, 1], [2, 3], [1, 2], [0, 1]], ); }); diff --git a/__tests__/groupBy.ts b/__tests__/groupBy.ts index bf1ece605b..0939c05b8a 100644 --- a/__tests__/groupBy.ts +++ b/__tests__/groupBy.ts @@ -17,7 +17,7 @@ describe('groupBy', () => { // Each group should be a keyed sequence, not an indexed sequence const firstGroup = grouped.get(1); - expect(firstGroup && firstGroup.toArray()).toEqual([1, 3]); + expect(firstGroup && firstGroup.toArray()).toEqual([['a', 1], ['c', 3]]); }); it('groups indexed sequence', () => { diff --git a/src/CollectionImpl.js b/src/CollectionImpl.js index f24de062f0..e7f3eefa1d 100644 --- a/src/CollectionImpl.js +++ b/src/CollectionImpl.js @@ -104,8 +104,11 @@ mixin(Collection, { toArray() { assertNotInfinite(this.size); const array = new Array(this.size || 0); - this.valueSeq().__iterate((v, i) => { - array[i] = v; + const useTuples = isKeyed(this); + let i = 0; + this.__iterate((v, k) => { + // Keyed collections produce an array of tuples. + array[i++] = useTuples ? [k, v] : v; }); return array; }, diff --git a/src/Operations.js b/src/Operations.js index 9de70c397c..3c34bbce58 100644 --- a/src/Operations.js +++ b/src/Operations.js @@ -734,6 +734,7 @@ export function sortFactory(collection, comparator, mapper) { const entries = collection .toSeq() .map((v, k) => [k, v, index++, mapper ? mapper(v, k, collection) : v]) + .valueSeq() .toArray(); entries.sort((a, b) => comparator(a[3], b[3]) || a[2] - b[2]).forEach( isKeyedCollection diff --git a/type-definitions/Immutable.d.ts b/type-definitions/Immutable.d.ts index 76f13f5db9..79175c66c7 100644 --- a/type-definitions/Immutable.d.ts +++ b/type-definitions/Immutable.d.ts @@ -2661,6 +2661,11 @@ declare module Immutable { */ toJSON(): { [key: string]: V }; + /** + * Shallowly converts this collection to an Array. + */ + toArray(): Array<[K, V]>; + /** * Returns itself */ @@ -2772,6 +2777,11 @@ declare module Immutable { */ toJSON(): Array; + /** + * Shallowly converts this collection to an Array. + */ + toArray(): Array; + /** * Returns itself */ @@ -2916,6 +2926,11 @@ declare module Immutable { */ toJSON(): Array; + /** + * Shallowly converts this collection to an Array. + */ + toArray(): Array; + /** * Returns itself */ @@ -3182,6 +3197,11 @@ declare module Immutable { */ toJSON(): { [key: string]: V }; + /** + * Shallowly converts this collection to an Array. + */ + toArray(): Array<[K, V]>; + /** * Returns Seq.Keyed. * @override @@ -3330,6 +3350,11 @@ declare module Immutable { */ toJSON(): Array; + /** + * Shallowly converts this collection to an Array. + */ + toArray(): Array; + // Reading values /** @@ -3605,6 +3630,11 @@ declare module Immutable { */ toJSON(): Array; + /** + * Shallowly converts this collection to an Array. + */ + toArray(): Array; + /** * Returns Seq.Set. * @override @@ -3822,9 +3852,12 @@ declare module Immutable { toJSON(): Array | { [key: string]: V }; /** - * Shallowly converts this collection to an Array, discarding keys. + * Shallowly converts this collection to an Array. + * + * `Collection.Indexed`, and `Collection.Set` produce an Array of values. + * `Collection.Keyed` produce an Array of [key, value] tuples. */ - toArray(): Array; + toArray(): Array | Array<[K, V]>; /** * Shallowly converts this Collection to an Object. diff --git a/type-definitions/immutable.js.flow b/type-definitions/immutable.js.flow index f306d3be41..637a38aac8 100644 --- a/type-definitions/immutable.js.flow +++ b/type-definitions/immutable.js.flow @@ -50,7 +50,7 @@ declare class _Collection /*implements ValueObject*/ { toJS(): Array | { [key: string]: mixed }; toJSON(): Array | { [key: string]: V }; - toArray(): Array; + toArray(): Array | Array<[K,V]>; toObject(): { [key: string]: V }; toMap(): Map; toOrderedMap(): OrderedMap; @@ -209,6 +209,7 @@ declare class KeyedCollection extends Collection { toJS(): { [key: string]: mixed }; toJSON(): { [key: string]: V }; + toArray(): Array<[K, V]>; @@iterator(): Iterator<[K, V]>; toSeq(): KeyedSeq; flip(): KeyedCollection; @@ -247,6 +248,7 @@ declare class IndexedCollection<+T> extends Collection { toJS(): Array; toJSON(): Array; + toArray(): Array; @@iterator(): Iterator; toSeq(): IndexedSeq; fromEntrySeq(): KeyedSeq; @@ -388,6 +390,7 @@ declare class SetCollection<+T> extends Collection { toJS(): Array; toJSON(): Array; + toArray(): Array; @@iterator(): Iterator; toSeq(): SetSeq;