Merge pull request #16179 from emberjs/array-proxy-fixes

[BUGFIX beta] Fix a few bugs in the caching ArrayProxy implementation

packages/ember-runtime/lib/system/array_proxy.js

@@ -5,8 +5,8 @@
 import {
   get,
   computed,
-  observer,
-  alias
+  alias,
+  PROPERTY_DID_CHANGE
 } from 'ember-metal';
 import {
   isArray
@@ -20,6 +20,11 @@ import {
 } from '../mixins/array';
 import { assert } from 'ember-debug';
 
+const ARRAY_OBSERVER_MAPPING = {
+  willChange: '_arrangedContentArrayWillChange',
+  didChange: '_arrangedContentArrayDidChange'
+};
+
 /**
   An ArrayProxy wraps any other object that implements `Array` and/or
   `MutableArray,` forwarding all requests. This makes it very useful for
@@ -67,8 +72,23 @@ import { assert } from 'ember-debug';
 export default EmberObject.extend(MutableArray, {
   init() {
     this._super(...arguments);
-    this._cache = null;
-    this._dirtyStart = 0;
+
+    /*
+      `this._objectsDirtyIndex` determines which indexes in the `this._objects`
+      cache are dirty.
+
+      If `this._objectsDirtyIndex === -1` then no indexes are dirty.
+      Otherwise, an index `i` is dirty if `i >= this._objectsDirtyIndex`.
+
+      Calling `objectAt` with a dirty index will cause the `this._objects`
+      cache to be recomputed.
+    */
+    this._objectsDirtyIndex = 0;
+    this._objects = null;
+
+    this._lengthDirty = true;
+    this._length = 0;
+
     this._arrangedContent = null;
     this._addArrangedContentArrayObsever();
   },
@@ -139,51 +159,71 @@ export default EmberObject.extend(MutableArray, {
 
   // Overriding objectAt is not supported.
   objectAt(idx) {
-    this._sync();
-    return this._cache[idx];
+    if (this._objects === null) {
+      this._objects = [];
+    }
+
+    if (this._objectsDirtyIndex !== -1 && idx >= this._objectsDirtyIndex) {
+      let arrangedContent = get(this, 'arrangedContent');
+      if (arrangedContent) {
+        let length = this._objects.length = get(arrangedContent, 'length');
+
+        for (let i = this._objectsDirtyIndex; i < length; i++) {
+          this._objects[i] = this.objectAtContent(i);
+        }
+      } else {
+        this._objects.length = 0;
+      }
+      this._objectsDirtyIndex = -1;
+    }
+
+    return this._objects[idx];
   },
 
   // Overriding length is not supported.
   length: computed(function() {
-    this._sync();
-    return this._cache.length;
-  }),
+    if (this._lengthDirty) {
+      let arrangedContent = get(this, 'arrangedContent');
+      this._length = arrangedContent ? get(arrangedContent, 'length') : 0;
+      this._lengthDirty = false;
+    }
 
-  _arrangedContentDidChange: observer('arrangedContent', function() {
-    let oldLength = this._cache === null ? 0 : this._cache.length;
-    let arrangedContent = get(this, 'arrangedContent');
-    let newLength = arrangedContent ? get(arrangedContent, 'length') : 0;
+    return this._length;
+  }).volatile(),
 
-    this._removeArrangedContentArrayObsever();
-    this.arrayContentWillChange(0, oldLength, newLength);
-    this._dirtyStart = 0;
-    this.arrayContentDidChange(0, oldLength, newLength);
-    this._addArrangedContentArrayObsever();
-  }),
+  [PROPERTY_DID_CHANGE](key) {
+    if (key === 'arrangedContent') {
+      let oldLength = this._objects === null ? 0 : this._objects.length;
+      let arrangedContent = get(this, 'arrangedContent');
+      let newLength = arrangedContent ? get(arrangedContent, 'length') : 0;
+
+      this._removeArrangedContentArrayObsever();
+      this.arrayContentWillChange(0, oldLength, newLength);
+
+      this._objectsDirtyIndex = 0;
+      this._lengthDirty = true;
+
+      this.arrayContentDidChange(0, oldLength, newLength);
+      this._addArrangedContentArrayObsever();
+    }
+  },
 
   _addArrangedContentArrayObsever() {
     let arrangedContent = get(this, 'arrangedContent');
-
     if (arrangedContent) {
       assert('Can\'t set ArrayProxy\'s content to itself', arrangedContent !== this);
       assert(`ArrayProxy expects an Array or ArrayProxy, but you passed ${typeof arrangedContent}`,
         isArray(arrangedContent) || arrangedContent.isDestroyed);
 
-      addArrayObserver(arrangedContent, this, {
-        willChange: '_arrangedContentArrayWillChange',
-        didChange: '_arrangedContentArrayDidChange'
-      });
+      addArrayObserver(arrangedContent, this, ARRAY_OBSERVER_MAPPING);
 
       this._arrangedContent = arrangedContent;
     }
   },
 
   _removeArrangedContentArrayObsever() {
     if (this._arrangedContent) {
-      removeArrayObserver(this._arrangedContent, this, {
-        willChange: '_arrangedContentArrayWillChange',
-        didChange: '_arrangedContentArrayDidChange'
-      });
+      removeArrayObserver(this._arrangedContent, this, ARRAY_OBSERVER_MAPPING);
     }
   },
 
@@ -192,38 +232,22 @@ export default EmberObject.extend(MutableArray, {
   _arrangedContentArrayDidChange(proxy, idx, removedCnt, addedCnt) {
     this.arrayContentWillChange(idx, removedCnt, addedCnt);
 
-    if (this._dirtyStart === undefined) {
-      this._dirtyStart = idx;
-    } else {
-      if (this._dirtyStart > idx) {
-        this._dirtyStart = idx;
-      }
+    let dirtyIndex = idx;
+    if (dirtyIndex < 0) {
+      let length = get(this._arrangedContent, 'length');
+      dirtyIndex += length + removedCnt - addedCnt;
     }
 
-    this.arrayContentDidChange(idx, removedCnt, addedCnt);
-  },
-
-  _sync() {
-    if (this._cache === null) {
-      this._cache = [];
+    if (this._objectsDirtyIndex === -1) {
+      this._objectsDirtyIndex = dirtyIndex;
+    } else {
+      if (this._objectsDirtyIndex > dirtyIndex) {
+        this._objectsDirtyIndex = dirtyIndex;
+      }
     }
 
-    if (this._dirtyStart !== undefined) {
-      let arrangedContent = get(this, 'arrangedContent');
-
-      if (arrangedContent) {
-        let length = get(arrangedContent, 'length');
-
-        this._cache.length = length;
+    this._lengthDirty = true;
 
-        for (let i = this._dirtyStart; i < length; i++) {
-          this._cache[i] = this.objectAtContent(i);
-        }
-      } else {
-        this._cache.length = 0;
-      }
-
-      this._dirtyStart = undefined;
-    }
+    this.arrayContentDidChange(idx, removedCnt, addedCnt);
   }
 });

packages/ember-runtime/tests/system/array_proxy/array_observer_test.js

@@ -1,4 +1,4 @@
-import { get, set } from 'ember-metal';
+import { set } from 'ember-metal';
 import ArrayProxy from '../../../system/array_proxy';
 import { A } from '../../../system/native_array';
 
@@ -21,7 +21,7 @@ QUnit.test('mutating content', assert => {
     }
   });
 
-  get(proxy, 'length');
+  proxy.toArray();
   content.replace(1, 1, ['a', 'b', 'c']);
 });
 
@@ -42,6 +42,6 @@ QUnit.test('assigning content', assert => {
     }
   });
 
-  get(proxy, 'length');
+  proxy.toArray();
   set(proxy, 'content', A(['a', 'b', 'c', 'd', 'e']));
 });

packages/ember-runtime/tests/system/array_proxy/content_change_test.js

@@ -1,4 +1,4 @@
-import { set, run } from 'ember-metal';
+import { get, set, run, changeProperties } from 'ember-metal';
 import { not } from '../../../computed/computed_macros';
 import ArrayProxy from '../../../system/array_proxy';
 import { A as emberA } from '../../../system/native_array';
@@ -45,3 +45,93 @@ QUnit.test('The ArrayProxy doesn\'t explode when assigned a destroyed object', f
 
   assert.ok(true, 'No exception was raised');
 });
+
+QUnit.test('should update if content changes while change events are deferred', function(assert) {
+  let proxy = ArrayProxy.create();
+
+  assert.deepEqual(proxy.toArray(), []);
+
+  changeProperties(() => {
+    proxy.set('content', emberA([1, 2, 3]));
+    assert.deepEqual(proxy.toArray(), [1, 2, 3]);
+  });
+});
+
+QUnit.test('objectAt recomputes the object cache correctly', function(assert) {
+  let indexes = [];
+
+  let proxy = ArrayProxy.extend({
+    objectAtContent(index) {
+      indexes.push(index);
+      return this.content[index];
+    }
+  }).create({
+    content: emberA([1, 2, 3, 4, 5])
+  });
+
+  assert.deepEqual(indexes, []);
+  assert.deepEqual(proxy.objectAt(0), 1);
+  assert.deepEqual(indexes, [0, 1, 2, 3, 4]);
+
+  indexes.length = 0;
+  proxy.set('content', emberA([1, 2, 3]));
+  assert.deepEqual(proxy.objectAt(0), 1);
+  assert.deepEqual(indexes, [0, 1, 2]);
+
+  indexes.length = 0;
+  proxy.content.replace(2, 0, [4, 5]);
+  assert.deepEqual(proxy.objectAt(0), 1);
+  assert.deepEqual(proxy.objectAt(1), 2);
+  assert.deepEqual(indexes, []);
+  assert.deepEqual(proxy.objectAt(2), 4);
+  assert.deepEqual(indexes, [2, 3, 4]);
+});
+
+QUnit.test('getting length does not recompute the object cache', function(assert) {
+  let indexes = [];
+
+  let proxy = ArrayProxy.extend({
+    objectAtContent(index) {
+      indexes.push(index);
+      return this.content[index];
+    }
+  }).create({
+    content: emberA([1, 2, 3, 4, 5])
+  });
+
+  assert.equal(get(proxy, 'length'), 5);
+  assert.deepEqual(indexes, []);
+
+  indexes.length = 0;
+  proxy.set('content', emberA([6, 7, 8]));
+  assert.equal(get(proxy, 'length'), 3);
+  assert.deepEqual(indexes, []);
+
+  indexes.length = 0;
+  proxy.content.replace(1, 0, [1, 2, 3]);
+  assert.equal(get(proxy, 'length'), 6);
+  assert.deepEqual(indexes, []);
+});
+
+QUnit.test('negative indexes are handled correctly', function(assert) {
+  let indexes = [];
+
+  let proxy = ArrayProxy.extend({
+    objectAtContent(index) {
+      indexes.push(index);
+      return this.content[index];
+    }
+  }).create({
+    content: emberA([1, 2, 3, 4, 5])
+  });
+
+  assert.deepEqual(proxy.toArray(), [1, 2, 3, 4, 5]);
+
+  indexes.length = 0;
+
+  proxy.content.replace(-1, 0, [7]);
+  proxy.content.replace(-2, 0, [6]);
+
+  assert.deepEqual(proxy.toArray(), [1, 2, 3, 4, 6, 7, 5]);
+  assert.deepEqual(indexes, [4, 5, 6]);
+});