Repository: immutable-js/immutable-js (ts)

Issue:
Singleton `emptyList` is problematic
I just ran down a nasty bug related to this code below:

https://github.com/immutable-js/immutable-js/blob/493afba6ec17d9c999dc5a15ac80c71c6bdba1c3/src/List.js#L413

I did not realize that every time I was creating a new list with `List()`, I was getting the same _instance_ of `List`. This seems counterintuitive in a library based on the idea of immutability. 

Some code in the project was mutating an instance of an empty list which caused all subsequent newly minted lists to be non-empty. Obviously the code in my project was incorrect, but I came here to suggest _not_ using a singleton pattern here regardless.  

Pull request description:
Try to fix issue with empty list that is a singleton

Fixes #2003

The EMPTY_LIST singleton is a really old code. It may have been there to avoid calling "makeList", but I don't think that there is a really huge performance impact.

Patch:
diff --git a/src/List.js b/src/List.js
index e512bb90d..be698316d 100644
--- a/src/List.js
+++ b/src/List.js
@@ -408,9 +408,8 @@ function makeList(origin, capacity, level, root, tail, ownerID, hash) {
   return list;
 }
 
-let EMPTY_LIST;
 export function emptyList() {
-  return EMPTY_LIST || (EMPTY_LIST = makeList(0, 0, SHIFT));
+  return makeList(0, 0, SHIFT);
 }
 
 function updateList(list, index, value) {


Test patch:
diff --git a/__tests__/List.ts b/__tests__/List.ts
index 2cb28ee6a..1fa808b80 100644
--- a/__tests__/List.ts
+++ b/__tests__/List.ts
@@ -829,7 +829,7 @@ describe('List', () => {
   it('chained mutations does not result in new empty list instance', () => {
     const v1 = List(['x']);
     const v2 = v1.withMutations(v => v.push('y').pop().pop());
-    expect(v2).toBe(List());
+    expect(v2).toEqual(List());
   });
 
   it('calling `clear` and `setSize` should set all items to undefined', () => {
@@ -927,6 +927,37 @@ describe('List', () => {
     expect(isNaNValue(list.get(0))).toBe(true);
   });
 
+  it('return a new emptyList if the emptyList has been mutated #2003', () => {
+    const emptyList = List();
+
+    const nonEmptyList = emptyList.withMutations(l => {
+      l.setSize(1);
+      l.set(0, 'a');
+    });
+
+    expect(nonEmptyList.size).toBe(1);
+    expect(nonEmptyList).toEqual(List.of('a'));
+    expect(emptyList.size).toBe(0);
+
+    const mutableList = emptyList.asMutable();
+
+    mutableList.setSize(1);
+    mutableList.set(0, 'b');
+
+    expect(mutableList.size).toBe(1);
+    expect(mutableList).toEqual(List.of('b'));
+
+    expect(emptyList.size).toBe(0);
+    expect(List().size).toBe(0);
+  });
+
+  it('Mutating empty list with a JS API should not mutate new instances', () => {
+    Object.assign(List(), List([1, 2]));
+
+    expect(List().size).toBe(0);
+    expect(List().toArray()).toEqual([]);
+  });
+
   // Note: NaN is the only value not equal to itself. The isNaN() built-in
   // function returns true for any non-numeric value, not just the NaN value.
   function isNaNValue(value) {