Revert "feat(iterable_differ): support immutable lists"

tbosch · tbosch · commit d24df799d302 · 2016-04-13T17:02:48.000Z
In Dart, ImmutableLists are just a projection of an underlying list. I.e. if the underlying list changes, the ImmutableList also changes. So we can't make optimizations based on checking whether a collection is an ImmutableList. This reverts commit a10c02c. Closes angular#8023
diff --git a/modules/angular2/src/core/change_detection/differs/default_iterable_differ.ts b/modules/angular2/src/core/change_detection/differs/default_iterable_differ.ts
@@ -109,6 +109,7 @@ export class DefaultIterableDiffer implements IterableDiffer {
 
   onDestroy() {}
 
+  // todo(vicb): optim for UnmodifiableListView (frozen arrays)
   check(collection: any): boolean {
     this._reset();
 
@@ -118,27 +119,24 @@ export class DefaultIterableDiffer implements IterableDiffer {
     var item;
     var itemTrackBy;
     if (isArray(collection)) {
-      if (collection !== this._collection || !ListWrapper.isImmutable(collection)) {
-        var list = collection;
-        this._length = collection.length;
-
-        for (index = 0; index < this._length; index++) {
-          item = list[index];
-          itemTrackBy = this._trackByFn(index, item);
-          if (record === null || !looseIdentical(record.trackById, itemTrackBy)) {
-            record = this._mismatch(record, item, itemTrackBy, index);
-            mayBeDirty = true;
-          } else {
-            if (mayBeDirty) {
-              // TODO(misko): can we limit this to duplicates only?
-              record = this._verifyReinsertion(record, item, itemTrackBy, index);
-            }
-            if (!looseIdentical(record.item, item)) this._addIdentityChange(record, item);
-          }
+      var list = collection;
+      this._length = collection.length;
 
-          record = record._next;
+      for (index = 0; index < this._length; index++) {
+        item = list[index];
+        itemTrackBy = this._trackByFn(index, item);
+        if (record === null || !looseIdentical(record.trackById, itemTrackBy)) {
+          record = this._mismatch(record, item, itemTrackBy, index);
+          mayBeDirty = true;
+        } else {
+          if (mayBeDirty) {
+            // TODO(misko): can we limit this to duplicates only?
+            record = this._verifyReinsertion(record, item, itemTrackBy, index);
+          }
+          if (!looseIdentical(record.item, item)) this._addIdentityChange(record, item);
         }
-        this._truncate(record);
+
+        record = record._next;
       }
     } else {
       index = 0;
@@ -158,9 +156,9 @@ export class DefaultIterableDiffer implements IterableDiffer {
         index++;
       });
       this._length = index;
-      this._truncate(record);
     }
 
+    this._truncate(record);
     this._collection = collection;
     return this.isDirty;
   }
diff --git a/modules/angular2/src/facade/collection.dart b/modules/angular2/src/facade/collection.dart
@@ -1,6 +1,6 @@
 library facade.collection;
 
-import 'dart:collection' show IterableBase, UnmodifiableListView;
+import 'dart:collection' show IterableBase;
 import 'dart:convert' show JsonEncoder;
 export 'dart:core' show Iterator, Map, List, Set;
 import 'dart:math' show max, min;
@@ -110,9 +110,6 @@ class ListWrapper {
   static List/*<T>*/ createFixedSize/*<T>*/(int size) => new List(size);
   static List/*<T>*/ createGrowableSize/*<T>*/(int size) =>
       new List.generate(size, (_) => null, growable: true);
-  static UnmodifiableListView createImmutable(List input) {
-    return new UnmodifiableListView(input);
-  }
 
   static bool contains(List m, k) => m.contains(k);
   static int indexOf(List list, value, [int startIndex = 0]) =>
@@ -229,10 +226,6 @@ class ListWrapper {
     return solution;
   }
 
-  static bool isImmutable(List l) {
-    return l is UnmodifiableListView;
-  }
-
   static List flatten(List l) {
     final res = [];
     l.forEach((item) {
diff --git a/modules/angular2/src/facade/collection.ts b/modules/angular2/src/facade/collection.ts
@@ -184,11 +184,6 @@ export class ListWrapper {
   static createFixedSize(size: number): any[] { return new Array(size); }
   static createGrowableSize(size: number): any[] { return new Array(size); }
   static clone<T>(array: T[]): T[] { return array.slice(0); }
-  static createImmutable<T>(array: T[]): T[] {
-    var result = ListWrapper.clone(array);
-    Object.seal(result);
-    return result;
-  }
   static forEachWithIndex<T>(array: T[], fn: (t: T, n: number) => void) {
     for (var i = 0; i < array.length; i++) {
       fn(array[i], i);
@@ -277,7 +272,6 @@ export class ListWrapper {
     return solution;
   }
 
-  static isImmutable(list: any[]): boolean { return Object.isSealed(list); }
   static flatten<T>(array: T[][]): T[] {
     let res = [];
     array.forEach((a) => res = res.concat(a));
diff --git a/modules/angular2/test/core/change_detection/differs/default_iterable_differ_spec.ts b/modules/angular2/test/core/change_detection/differs/default_iterable_differ_spec.ts
@@ -31,6 +31,7 @@ class ComplexItem {
   toString() { return `{id: ${this.id}, color: ${this.color}}` }
 }
 
+// todo(vicb): UnmodifiableListView / frozen object when implemented
 export function main() {
   describe('iterable differ', function() {
     describe('DefaultIterableDiffer', function() {
@@ -313,36 +314,6 @@ export function main() {
             }));
       });
 
-      it('should not diff immutable collections if they are the same', () => {
-        // Note: Use trackBy to know if diffing happened
-        var trackByCount = 0;
-        var trackBy = (index: number, item: any): any => {
-          trackByCount++;
-          return item;
-        };
-        var differ = new DefaultIterableDiffer(trackBy);
-        var l1 = ListWrapper.createImmutable([1]);
-
-        differ.check(l1);
-        expect(trackByCount).toBe(1);
-        expect(differ.toString())
-            .toEqual(
-                iterableChangesAsString({collection: ['1[null->0]'], additions: ['1[null->0]']}));
-
-
-        trackByCount = 0;
-        differ.check(l1);
-        expect(trackByCount).toBe(0);
-        expect(differ.toString())
-            .toEqual(iterableChangesAsString({collection: ['1'], previous: ['1']}));
-
-        trackByCount = 0;
-        differ.check(l1);
-        expect(trackByCount).toBe(0);
-        expect(differ.toString())
-            .toEqual(iterableChangesAsString({collection: ['1'], previous: ['1']}));
-      });
-
       describe('diff', () => {
         it('should return self when there is a change',
            () => { expect(differ.diff(['a', 'b'])).toBe(differ); });
