From f34dbabccfaa97c5d8989ce3ce4ec196e2450bbf Mon Sep 17 00:00:00 2001 From: Bj Date: Thu, 19 May 2016 21:58:15 -0400 Subject: [PATCH 1/3] Don't use prototype extensions (arrayContent{Did,Will}Change) --- packages/ember-runtime/lib/mixins/array.js | 183 +++++++++--------- .../ember-runtime/lib/system/array_proxy.js | 6 +- .../ember-runtime/lib/system/native_array.js | 9 +- .../ember-runtime/tests/mixins/array_test.js | 58 +++--- .../tests/mixins/mutable_array_test.js | 8 +- 5 files changed, 141 insertions(+), 123 deletions(-) diff --git a/packages/ember-runtime/lib/mixins/array.js b/packages/ember-runtime/lib/mixins/array.js index 1b539910f62..a1077279775 100644 --- a/packages/ember-runtime/lib/mixins/array.js +++ b/packages/ember-runtime/lib/mixins/array.js @@ -68,6 +68,99 @@ export function objectAt(content, idx) { return content[idx]; } +export function arrayContentWillChange(array, startIdx, removeAmt, addAmt) { + let removing, lim; + + // if no args are passed assume everything changes + if (startIdx === undefined) { + startIdx = 0; + removeAmt = addAmt = -1; + } else { + if (removeAmt === undefined) { + removeAmt = -1; + } + + if (addAmt === undefined) { + addAmt = -1; + } + } + + if (array.__each) { + array.__each.arrayWillChange(array, startIdx, removeAmt, addAmt); + } + + sendEvent(array, '@array:before', [array, startIdx, removeAmt, addAmt]); + + if (startIdx >= 0 && removeAmt >= 0 && get(array, 'hasEnumerableObservers')) { + removing = []; + lim = startIdx + removeAmt; + + for (let idx = startIdx; idx < lim; idx++) { + removing.push(objectAt(array, idx)); + } + } else { + removing = removeAmt; + } + + array.enumerableContentWillChange(removing, addAmt); + + return array; +} + +export function arrayContentDidChange(array, startIdx, removeAmt, addAmt) { + markObjectAsDirty(metaFor(array)); + + // if no args are passed assume everything changes + if (startIdx === undefined) { + startIdx = 0; + removeAmt = addAmt = -1; + } else { + if (removeAmt === undefined) { + removeAmt = -1; + } + + if (addAmt === undefined) { + addAmt = -1; + } + } + + let adding; + if (startIdx >= 0 && addAmt >= 0 && get(array, 'hasEnumerableObservers')) { + adding = []; + let lim = startIdx + addAmt; + + for (let idx = startIdx; idx < lim; idx++) { + adding.push(objectAt(array, idx)); + } + } else { + adding = addAmt; + } + + array.enumerableContentDidChange(removeAmt, adding); + + if (array.__each) { + array.__each.arrayDidChange(array, startIdx, removeAmt, addAmt); + } + + sendEvent(array, '@array:change', [array, startIdx, removeAmt, addAmt]); + + let length = get(array, 'length'); + let cachedFirst = cacheFor(array, 'firstObject'); + let cachedLast = cacheFor(array, 'lastObject'); + + if (objectAt(array, 0) !== cachedFirst) { + propertyWillChange(array, 'firstObject'); + propertyDidChange(array, 'firstObject'); + } + + if (objectAt(array, length - 1) !== cachedLast) { + propertyWillChange(array, 'lastObject'); + propertyDidChange(array, 'lastObject'); + } + + return array; +} + const EMBER_ARRAY = symbol('EMBER_ARRAY'); export function isEmberArray(obj) { @@ -437,43 +530,7 @@ const ArrayMixin = Mixin.create(Enumerable, { @public */ arrayContentWillChange(startIdx, removeAmt, addAmt) { - let removing, lim; - - // if no args are passed assume everything changes - if (startIdx === undefined) { - startIdx = 0; - removeAmt = addAmt = -1; - } else { - if (removeAmt === undefined) { - removeAmt = -1; - } - - if (addAmt === undefined) { - addAmt = -1; - } - } - - if (this.__each) { - this.__each.arrayWillChange(this, startIdx, removeAmt, addAmt); - } - - sendEvent(this, '@array:before', [this, startIdx, removeAmt, addAmt]); - - - if (startIdx >= 0 && removeAmt >= 0 && get(this, 'hasEnumerableObservers')) { - removing = []; - lim = startIdx + removeAmt; - - for (let idx = startIdx; idx < lim; idx++) { - removing.push(objectAt(this, idx)); - } - } else { - removing = removeAmt; - } - - this.enumerableContentWillChange(removing, addAmt); - - return this; + return arrayContentWillChange(this, startIdx, removeAmt, addAmt); }, /** @@ -492,57 +549,7 @@ const ArrayMixin = Mixin.create(Enumerable, { @public */ arrayContentDidChange(startIdx, removeAmt, addAmt) { - markObjectAsDirty(metaFor(this)); - - // if no args are passed assume everything changes - if (startIdx === undefined) { - startIdx = 0; - removeAmt = addAmt = -1; - } else { - if (removeAmt === undefined) { - removeAmt = -1; - } - - if (addAmt === undefined) { - addAmt = -1; - } - } - - let adding; - if (startIdx >= 0 && addAmt >= 0 && get(this, 'hasEnumerableObservers')) { - adding = []; - let lim = startIdx + addAmt; - - for (let idx = startIdx; idx < lim; idx++) { - adding.push(objectAt(this, idx)); - } - } else { - adding = addAmt; - } - - this.enumerableContentDidChange(removeAmt, adding); - - if (this.__each) { - this.__each.arrayDidChange(this, startIdx, removeAmt, addAmt); - } - - sendEvent(this, '@array:change', [this, startIdx, removeAmt, addAmt]); - - let length = get(this, 'length'); - let cachedFirst = cacheFor(this, 'firstObject'); - let cachedLast = cacheFor(this, 'lastObject'); - - if (objectAt(this, 0) !== cachedFirst) { - propertyWillChange(this, 'firstObject'); - propertyDidChange(this, 'firstObject'); - } - - if (objectAt(this, length - 1) !== cachedLast) { - propertyWillChange(this, 'lastObject'); - propertyDidChange(this, 'lastObject'); - } - - return this; + return arrayContentDidChange(this, startIdx, removeAmt, addAmt); }, /** diff --git a/packages/ember-runtime/lib/system/array_proxy.js b/packages/ember-runtime/lib/system/array_proxy.js index f1b78d2ed96..83ae0bbbf56 100644 --- a/packages/ember-runtime/lib/system/array_proxy.js +++ b/packages/ember-runtime/lib/system/array_proxy.js @@ -20,6 +20,8 @@ import alias from 'ember-metal/alias'; import { addArrayObserver, removeArrayObserver, + arrayContentDidChange, + arrayContentWillChange, objectAt } from 'ember-runtime/mixins/array'; @@ -371,11 +373,11 @@ export default EmberObject.extend(MutableArray, { }, arrangedContentArrayWillChange(item, idx, removedCnt, addedCnt) { - this.arrayContentWillChange(idx, removedCnt, addedCnt); + arrayContentWillChange(this, idx, removedCnt, addedCnt); }, arrangedContentArrayDidChange(item, idx, removedCnt, addedCnt) { - this.arrayContentDidChange(idx, removedCnt, addedCnt); + arrayContentDidChange(this, idx, removedCnt, addedCnt); }, init() { diff --git a/packages/ember-runtime/lib/system/native_array.js b/packages/ember-runtime/lib/system/native_array.js index 79d03cdf3ac..3989fa35661 100644 --- a/packages/ember-runtime/lib/system/native_array.js +++ b/packages/ember-runtime/lib/system/native_array.js @@ -7,7 +7,10 @@ import { ENV } from 'ember-environment'; import { _replace as replace } from 'ember-metal/replace'; import { get } from 'ember-metal/property_get'; import { Mixin } from 'ember-metal/mixin'; -import EmberArray from 'ember-runtime/mixins/array'; +import EmberArray, { + arrayContentDidChange, + arrayContentWillChange +} from 'ember-runtime/mixins/array'; import MutableArray from 'ember-runtime/mixins/mutable_array'; import Observable from 'ember-runtime/mixins/observable'; import Copyable from 'ember-runtime/mixins/copyable'; @@ -58,7 +61,7 @@ let NativeArray = Mixin.create(MutableArray, Observable, Copyable, { // replaced range. Otherwise, pass the full remaining array length // since everything has shifted let len = objects ? get(objects, 'length') : 0; - this.arrayContentWillChange(idx, amt, len); + arrayContentWillChange(this, idx, amt, len); if (len === 0) { this.splice(idx, amt); @@ -66,7 +69,7 @@ let NativeArray = Mixin.create(MutableArray, Observable, Copyable, { replace(this, idx, amt, objects); } - this.arrayContentDidChange(idx, amt, len); + arrayContentDidChange(this, idx, amt, len); return this; }, diff --git a/packages/ember-runtime/tests/mixins/array_test.js b/packages/ember-runtime/tests/mixins/array_test.js index 394501dd071..88f438075cf 100644 --- a/packages/ember-runtime/tests/mixins/array_test.js +++ b/packages/ember-runtime/tests/mixins/array_test.js @@ -9,6 +9,8 @@ import EmberObject from 'ember-runtime/system/object'; import EmberArray, { addArrayObserver, removeArrayObserver, + arrayContentDidChange, + arrayContentWillChange, objectAt } from 'ember-runtime/mixins/array'; import { A as emberA } from 'ember-runtime/system/native_array'; @@ -29,15 +31,15 @@ const TestArray = EmberObject.extend(EmberArray, { // MutableArray is just a standard API for mutation but not required. addObject(obj) { let idx = this._content.length; - this.arrayContentWillChange(idx, 0, 1); + arrayContentWillChange(this, idx, 0, 1); this._content.push(obj); - this.arrayContentDidChange(idx, 0, 1); + arrayContentDidChange(this, idx, 0, 1); }, removeFirst(idx) { - this.arrayContentWillChange(0, 1, 0); + arrayContentWillChange(this, 0, 1, 0); this._content.shift(); - this.arrayContentDidChange(0, 1, 0); + arrayContentDidChange(this, 0, 1, 0); }, objectAt(idx) { @@ -124,8 +126,8 @@ QUnit.test('should notify observers of []', function() { equal(obj._count, 0, 'should not have invoked yet'); - obj.arrayContentWillChange(0, 1, 1); - obj.arrayContentDidChange(0, 1, 1); + arrayContentWillChange(obj, 0, 1, 1); + arrayContentDidChange(obj, 0, 1, 1); equal(obj._count, 1, 'should have invoked'); }); @@ -153,27 +155,27 @@ QUnit.module('notify observers of length', { }); QUnit.test('should notify observers when call with no params', function() { - obj.arrayContentWillChange(); + arrayContentWillChange(obj); equal(obj._after, 0); - obj.arrayContentDidChange(); + arrayContentDidChange(obj); equal(obj._after, 1); }); // API variation that included items only QUnit.test('should not notify when passed lengths are same', function() { - obj.arrayContentWillChange(0, 1, 1); + arrayContentWillChange(obj, 0, 1, 1); equal(obj._after, 0); - obj.arrayContentDidChange(0, 1, 1); + arrayContentDidChange(obj, 0, 1, 1); equal(obj._after, 0); }); QUnit.test('should notify when passed lengths are different', function() { - obj.arrayContentWillChange(0, 1, 2); + arrayContentWillChange(obj, 0, 1, 2); equal(obj._after, 0); - obj.arrayContentDidChange(0, 1, 2); + arrayContentDidChange(obj, 0, 1, 2); equal(obj._after, 1); }); @@ -210,36 +212,36 @@ QUnit.module('notify array observers', { }); QUnit.test('should notify enumerable observers when called with no params', function() { - obj.arrayContentWillChange(); + arrayContentWillChange(obj); deepEqual(observer._before, [obj, 0, -1, -1]); - obj.arrayContentDidChange(); + arrayContentDidChange(obj); deepEqual(observer._after, [obj, 0, -1, -1]); }); // API variation that included items only QUnit.test('should notify when called with same length items', function() { - obj.arrayContentWillChange(0, 1, 1); + arrayContentWillChange(obj, 0, 1, 1); deepEqual(observer._before, [obj, 0, 1, 1]); - obj.arrayContentDidChange(0, 1, 1); + arrayContentDidChange(obj, 0, 1, 1); deepEqual(observer._after, [obj, 0, 1, 1]); }); QUnit.test('should notify when called with diff length items', function() { - obj.arrayContentWillChange(0, 2, 1); + arrayContentWillChange(obj, 0, 2, 1); deepEqual(observer._before, [obj, 0, 2, 1]); - obj.arrayContentDidChange(0, 2, 1); + arrayContentDidChange(obj, 0, 2, 1); deepEqual(observer._after, [obj, 0, 2, 1]); }); QUnit.test('removing enumerable observer should disable', function() { removeArrayObserver(obj, observer); - obj.arrayContentWillChange(); + arrayContentWillChange(obj); deepEqual(observer._before, null); - obj.arrayContentDidChange(); + arrayContentDidChange(obj); deepEqual(observer._after, null); }); @@ -275,36 +277,36 @@ QUnit.module('notify enumerable observers as well', { }); QUnit.test('should notify enumerable observers when called with no params', function() { - obj.arrayContentWillChange(); + arrayContentWillChange(obj); deepEqual(observer._before, [obj, null, null], 'before'); - obj.arrayContentDidChange(); + arrayContentDidChange(obj); deepEqual(observer._after, [obj, null, null], 'after'); }); // API variation that included items only QUnit.test('should notify when called with same length items', function() { - obj.arrayContentWillChange(0, 1, 1); + arrayContentWillChange(obj, 0, 1, 1); deepEqual(observer._before, [obj, ['ITEM-0'], 1], 'before'); - obj.arrayContentDidChange(0, 1, 1); + arrayContentDidChange(obj, 0, 1, 1); deepEqual(observer._after, [obj, 1, ['ITEM-0']], 'after'); }); QUnit.test('should notify when called with diff length items', function() { - obj.arrayContentWillChange(0, 2, 1); + arrayContentWillChange(obj, 0, 2, 1); deepEqual(observer._before, [obj, ['ITEM-0', 'ITEM-1'], 1], 'before'); - obj.arrayContentDidChange(0, 2, 1); + arrayContentDidChange(obj, 0, 2, 1); deepEqual(observer._after, [obj, 2, ['ITEM-0']], 'after'); }); QUnit.test('removing enumerable observer should disable', function() { obj.removeEnumerableObserver(observer); - obj.arrayContentWillChange(); + arrayContentWillChange(obj); deepEqual(observer._before, null, 'before'); - obj.arrayContentDidChange(); + arrayContentDidChange(obj); deepEqual(observer._after, null, 'after'); }); diff --git a/packages/ember-runtime/tests/mixins/mutable_array_test.js b/packages/ember-runtime/tests/mixins/mutable_array_test.js index 2cf64ed1ef7..5a0cafd777c 100644 --- a/packages/ember-runtime/tests/mixins/mutable_array_test.js +++ b/packages/ember-runtime/tests/mixins/mutable_array_test.js @@ -3,6 +3,10 @@ import MutableArrayTests from 'ember-runtime/tests/suites/mutable_array'; import MutableArray from 'ember-runtime/mixins/mutable_array'; import EmberObject from 'ember-runtime/system/object'; import { A as emberA } from 'ember-runtime/system/native_array'; +import { + arrayContentDidChange, + arrayContentWillChange +} from 'ember-runtime/mixins/array'; /* Implement a basic fake mutable array. This validates that any non-native @@ -21,12 +25,12 @@ const TestMutableArray = EmberObject.extend(MutableArray, { let removeAmt = amt; let addAmt = args.length; - this.arrayContentWillChange(idx, removeAmt, addAmt); + arrayContentWillChange(this, idx, removeAmt, addAmt); args.unshift(amt); args.unshift(idx); this._content.splice.apply(this._content, args); - this.arrayContentDidChange(idx, removeAmt, addAmt); + arrayContentDidChange(this, idx, removeAmt, addAmt); return this; }, From ad806e26c8dedfd3be6b0f30b22c8349b689664b Mon Sep 17 00:00:00 2001 From: Bj Date: Fri, 20 May 2016 00:08:41 -0400 Subject: [PATCH 2/3] Don't use prototype extensions for `removeAt` --- .../helpers/element-action-test.js | 3 +- .../tests/integration/syntax/each-test.js | 13 ++-- .../tests/integration/syntax/with-test.js | 3 +- .../tests/utils/shared-conditional-tests.js | 5 +- .../ember-runtime/lib/mixins/mutable_array.js | 38 ++++++----- .../ember-runtime/lib/system/array_proxy.js | 64 ++++++++++--------- .../computed/reduce_computed_macros_test.js | 9 +-- .../tests/suites/mutable_array/removeAt.js | 26 ++++---- .../array_proxy/arranged_content_test.js | 4 +- 9 files changed, 90 insertions(+), 75 deletions(-) diff --git a/packages/ember-glimmer/tests/integration/helpers/element-action-test.js b/packages/ember-glimmer/tests/integration/helpers/element-action-test.js index d915b32896b..b0f5563298a 100644 --- a/packages/ember-glimmer/tests/integration/helpers/element-action-test.js +++ b/packages/ember-glimmer/tests/integration/helpers/element-action-test.js @@ -5,6 +5,7 @@ import { set } from 'ember-metal/property_set'; import EmberObject from 'ember-runtime/system/object'; import { A as emberA } from 'ember-runtime/system/native_array'; +import { removeAt } from 'ember-runtime/mixins/mutable_array'; import ActionManager from 'ember-views/system/action_manager'; import jQuery from 'ember-views/system/jquery'; @@ -761,7 +762,7 @@ moduleFor('Helpers test: element action', class extends RenderingTest { var actionId = this.$('a[data-ember-action]').attr('data-ember-action'); this.runTask(() => { - things.removeAt(0); + removeAt(things, 0); }); ok(!ActionManager.registeredActions[actionId], 'After the virtual view was destroyed, the action was unregistered'); diff --git a/packages/ember-glimmer/tests/integration/syntax/each-test.js b/packages/ember-glimmer/tests/integration/syntax/each-test.js index 937eb6d208e..59796c806d5 100644 --- a/packages/ember-glimmer/tests/integration/syntax/each-test.js +++ b/packages/ember-glimmer/tests/integration/syntax/each-test.js @@ -3,6 +3,7 @@ import { set } from 'ember-metal/property_set'; import { applyMixins, strip } from '../../utils/abstract-test-case'; import { moduleFor, RenderingTest } from '../../utils/test-case'; import { A as emberA } from 'ember-runtime/system/native_array'; +import { removeAt } from 'ember-runtime/mixins/mutable_array'; import { BasicConditionalsTest, @@ -78,7 +79,7 @@ moduleFor('Syntax test: {{#each as}}', class extends EachTest { this.runTask(() => { let list = get(this.context, 'list'); list.pushObject({ text: 'Earth' }); - list.removeAt(1); + removeAt(list, 1); list.insertAt(1, { text: 'Globe' }); }); @@ -87,11 +88,11 @@ moduleFor('Syntax test: {{#each as}}', class extends EachTest { this.runTask(() => { let list = get(this.context, 'list'); list.pushObject({ text: 'Planet' }); - list.removeAt(1); + removeAt(list, 1); list.insertAt(1, { text: ' ' }); list.pushObject({ text: ' ' }); list.pushObject({ text: 'Earth' }); - list.removeAt(3); + removeAt(list, 3); }); this.assertText('Hello WorldPlanet Earth'); @@ -99,11 +100,11 @@ moduleFor('Syntax test: {{#each as}}', class extends EachTest { this.runTask(() => { let list = get(this.context, 'list'); list.pushObject({ text: 'Globe' }); - list.removeAt(1); + removeAt(list, 1); list.insertAt(1, { text: ' ' }); list.pushObject({ text: ' ' }); list.pushObject({ text: 'World' }); - list.removeAt(2); + removeAt(list, 2); }); this.assertText('Hello Planet EarthGlobe World'); @@ -372,7 +373,7 @@ moduleFor('Syntax test: {{#each as}}', class extends EachTest { this.runTask(() => { let people = get(this.context, 'people'); set(people.objectAt(1), 'name', 'Stefan Penner'); - people.removeAt(0); + removeAt(people, 0); people.pushObject({ name: 'Tom Dale' }); people.insertAt(1, { name: 'Chad Hietala' }); set(this.context, 'title', 'Principal Engineer'); diff --git a/packages/ember-glimmer/tests/integration/syntax/with-test.js b/packages/ember-glimmer/tests/integration/syntax/with-test.js index dc34a9d5f7d..0feb602cc00 100644 --- a/packages/ember-glimmer/tests/integration/syntax/with-test.js +++ b/packages/ember-glimmer/tests/integration/syntax/with-test.js @@ -5,6 +5,7 @@ import { moduleFor, RenderingTest } from '../../utils/test-case'; import { TogglingSyntaxConditionalsTest } from '../../utils/shared-conditional-tests'; import { strip } from '../../utils/abstract-test-case'; import ObjectProxy from 'ember-runtime/system/object_proxy'; +import { removeAt } from 'ember-runtime/mixins/mutable_array'; moduleFor('Syntax test: {{#with}}', class extends TogglingSyntaxConditionalsTest { @@ -198,7 +199,7 @@ moduleFor('Syntax test: {{#with as}}', class extends TogglingSyntaxConditionalsT this.runTask(() => { let array = get(this.context, 'arrayThing'); array.replace(0, 1, 'Goodbye'); - array.removeAt(1); + removeAt(array, 1); array.insertAt(1, ', '); array.pushObject('!'); }); diff --git a/packages/ember-glimmer/tests/utils/shared-conditional-tests.js b/packages/ember-glimmer/tests/utils/shared-conditional-tests.js index fa381f30533..e11f1e13535 100644 --- a/packages/ember-glimmer/tests/utils/shared-conditional-tests.js +++ b/packages/ember-glimmer/tests/utils/shared-conditional-tests.js @@ -7,6 +7,7 @@ import EmberObject from 'ember-runtime/system/object'; import ObjectProxy from 'ember-runtime/system/object_proxy'; import { A as emberA } from 'ember-runtime/system/native_array'; import ArrayProxy from 'ember-runtime/system/array_proxy'; +import { removeAt } from 'ember-runtime/mixins/mutable_array'; import { Component } from './helpers'; class AbstractConditionalsTest extends RenderingTest { @@ -308,7 +309,7 @@ export const ArrayTestCases = { this.assertText('T1F2'); - this.runTask(() => get(this.context, 'cond1').removeAt(0)); + this.runTask(() => removeAt(get(this.context, 'cond1'), 0)); this.assertText('F1F2'); @@ -373,7 +374,7 @@ export const ArrayTestCases = { this.assertText('T1F2'); - this.runTask(() => get(this.context, 'cond1.content').removeAt(0)); + this.runTask(() => removeAt(get(this.context, 'cond1.content'), 0)); this.assertText('F1F2'); diff --git a/packages/ember-runtime/lib/mixins/mutable_array.js b/packages/ember-runtime/lib/mixins/mutable_array.js index efdba681fec..6de28cfbbe7 100644 --- a/packages/ember-runtime/lib/mixins/mutable_array.js +++ b/packages/ember-runtime/lib/mixins/mutable_array.js @@ -26,6 +26,23 @@ import MutableEnumerable from 'ember-runtime/mixins/mutable_enumerable'; import Enumerable from 'ember-runtime/mixins/enumerable'; import isEnabled from 'ember-metal/features'; +export function removeAt(array, start, len) { + if ('number' === typeof start) { + if ((start < 0) || (start >= get(array, 'length'))) { + throw new EmberError(OUT_OF_RANGE_EXCEPTION); + } + + // fast case + if (len === undefined) { + len = 1; + } + + array.replace(start, len, EMPTY); + } + + return array; +} + /** This mixin defines the API for modifying array-like objects. These methods can be applied only to a collection that keeps its items in an ordered set. @@ -129,9 +146,9 @@ export default Mixin.create(EmberArray, MutableEnumerable, { ```javascript let colors = ['red', 'green', 'blue', 'yellow', 'orange']; - colors.removeAt(0); // ['green', 'blue', 'yellow', 'orange'] - colors.removeAt(2, 2); // ['green', 'blue'] - colors.removeAt(4, 2); // Error: Index out of range + removeAt(colors, 0); // ['green', 'blue', 'yellow', 'orange'] + removeAt(colors, 2, 2); // ['green', 'blue'] + removeAt(colors, 4, 2); // Error: Index out of range ``` @method removeAt @@ -141,20 +158,7 @@ export default Mixin.create(EmberArray, MutableEnumerable, { @public */ removeAt(start, len) { - if ('number' === typeof start) { - if ((start < 0) || (start >= get(this, 'length'))) { - throw new EmberError(OUT_OF_RANGE_EXCEPTION); - } - - // fast case - if (len === undefined) { - len = 1; - } - - this.replace(start, len, EMPTY); - } - - return this; + return removeAt(this, start, len); }, /** diff --git a/packages/ember-runtime/lib/system/array_proxy.js b/packages/ember-runtime/lib/system/array_proxy.js index 83ae0bbbf56..58e60a44c2c 100644 --- a/packages/ember-runtime/lib/system/array_proxy.js +++ b/packages/ember-runtime/lib/system/array_proxy.js @@ -35,6 +35,39 @@ const EMPTY = []; function K() { return this; } +export function removeAt(array, start, len) { + if ('number' === typeof start) { + let content = get(array, 'content'); + let arrangedContent = get(array, 'arrangedContent'); + let indices = []; + + if ((start < 0) || (start >= get(array, 'length'))) { + throw new EmberError(OUT_OF_RANGE_EXCEPTION); + } + + if (len === undefined) { + len = 1; + } + + // Get a list of indices in original content to remove + for (let i = start; i < start + len; i++) { + // Use arrangedContent here so we avoid confusion with objects transformed by objectAtContent + indices.push(content.indexOf(objectAt(arrangedContent, i))); + } + + // Replace in reverse order since indices will change + indices.sort((a, b) => b - a); + + beginPropertyChanges(); + for (let i = 0; i < indices.length; i++) { + array._replace(indices[i], 1, EMPTY); + } + endPropertyChanges(); + } + + return array; +} + /** An ArrayProxy wraps any other object that implements `Ember.Array` and/or `Ember.MutableArray,` forwarding all requests. This makes it very useful for @@ -302,36 +335,7 @@ export default EmberObject.extend(MutableArray, { }, removeAt(start, len) { - if ('number' === typeof start) { - let content = get(this, 'content'); - let arrangedContent = get(this, 'arrangedContent'); - let indices = []; - - if ((start < 0) || (start >= get(this, 'length'))) { - throw new EmberError(OUT_OF_RANGE_EXCEPTION); - } - - if (len === undefined) { - len = 1; - } - - // Get a list of indices in original content to remove - for (let i = start; i < start + len; i++) { - // Use arrangedContent here so we avoid confusion with objects transformed by objectAtContent - indices.push(content.indexOf(objectAt(arrangedContent, i))); - } - - // Replace in reverse order since indices will change - indices.sort((a, b) => b - a); - - beginPropertyChanges(); - for (let i = 0; i < indices.length; i++) { - this._replace(indices[i], 1, EMPTY); - } - endPropertyChanges(); - } - - return this; + return removeAt(this, start, len); }, pushObject(obj) { diff --git a/packages/ember-runtime/tests/computed/reduce_computed_macros_test.js b/packages/ember-runtime/tests/computed/reduce_computed_macros_test.js index 14af521fdb6..ca27901345d 100644 --- a/packages/ember-runtime/tests/computed/reduce_computed_macros_test.js +++ b/packages/ember-runtime/tests/computed/reduce_computed_macros_test.js @@ -28,6 +28,7 @@ import { import { isArray } from 'ember-runtime/utils'; import { testBoth } from 'ember-metal/tests/props_helper'; import { A as emberA } from 'ember-runtime/system/native_array'; +import { removeAt } from 'ember-runtime/mixins/mutable_array'; let obj; QUnit.module('map', { @@ -68,7 +69,7 @@ QUnit.test('it maps simple properties', function() { deepEqual(obj.get('mapped'), [1, 3, 2, 1, 5]); - obj.get('array').removeAt(3); + removeAt(obj.get('array'), 3); deepEqual(obj.get('mapped'), [1, 3, 2, 5]); }); @@ -135,7 +136,7 @@ QUnit.test('it maps objects', function() { { name: 'Eddard' } ]); - obj.get('arrayObjects').removeAt(1); + removeAt(obj.get('arrayObjects'), 1); deepEqual(obj.get('mappedObjects'), [ { name: 'Robert' }, @@ -201,7 +202,7 @@ QUnit.test('it maps properties', function() { deepEqual(obj.get('mapped'), [1, 3, 2, 1, 5]); - obj.get('array').removeAt(3); + removeAt(obj.get('array'), 3); deepEqual(obj.get('mapped'), [1, 3, 2, 5]); }); @@ -474,7 +475,7 @@ QUnit.test('properties values can be replaced', function() { deepEqual(obj.get('union').sort((x, y) => x - y), [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11], name + ' adds new items'); - array2.removeAt(6); // remove 7 + removeAt(array2, 6); // remove 7 deepEqual(obj.get('union').sort((x, y) => x - y), [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11], name + ' does not remove items that are still in the dependent array'); diff --git a/packages/ember-runtime/tests/suites/mutable_array/removeAt.js b/packages/ember-runtime/tests/suites/mutable_array/removeAt.js index 2bf30b82783..5b26baaf961 100644 --- a/packages/ember-runtime/tests/suites/mutable_array/removeAt.js +++ b/packages/ember-runtime/tests/suites/mutable_array/removeAt.js @@ -1,11 +1,12 @@ import {SuiteModuleBuilder} from 'ember-runtime/tests/suites/suite'; import {get} from 'ember-metal/property_get'; +import { removeAt } from 'ember-runtime/mixins/mutable_array'; const suite = SuiteModuleBuilder.create(); suite.module('removeAt'); -suite.test('[X].removeAt(0) => [] + notify', function() { +suite.test('removeAt([X], 0) => [] + notify', function() { let before = this.newFixture(1); let after = []; let obj = this.newObject(before); @@ -13,7 +14,7 @@ suite.test('[X].removeAt(0) => [] + notify', function() { obj.getProperties('firstObject', 'lastObject'); /* Prime the cache */ - equal(obj.removeAt(0), obj, 'return self'); + equal(removeAt(obj, 0), obj, 'return self'); deepEqual(this.toArray(obj), after, 'post item results'); equal(get(obj, 'length'), after.length, 'length'); @@ -25,12 +26,13 @@ suite.test('[X].removeAt(0) => [] + notify', function() { equal(observer.timesCalled('lastObject'), 1, 'should have notified lastObject once'); }); -suite.test('[].removeAt(200) => OUT_OF_RANGE_EXCEPTION exception', function() { +<<<<<<< f34dbabccfaa97c5d8989ce3ce4ec196e2450bbf +suite.test('removeAt([], 200) => OUT_OF_RANGE_EXCEPTION exception', function() { let obj = this.newObject([]); - throws(() => obj.removeAt(200), Error); + throws(() => removeAt(obj, 200), Error); }); -suite.test('[A,B].removeAt(0) => [B] + notify', function() { +suite.test('removeAt([A,B], 0) => [B] + notify', function() { let before = this.newFixture(2); let after = [before[1]]; let obj = this.newObject(before); @@ -38,7 +40,7 @@ suite.test('[A,B].removeAt(0) => [B] + notify', function() { obj.getProperties('firstObject', 'lastObject'); /* Prime the cache */ - equal(obj.removeAt(0), obj, 'return self'); + equal(removeAt(obj, 0), obj, 'return self'); deepEqual(this.toArray(obj), after, 'post item results'); equal(get(obj, 'length'), after.length, 'length'); @@ -51,7 +53,7 @@ suite.test('[A,B].removeAt(0) => [B] + notify', function() { equal(observer.validate('lastObject'), false, 'should NOT have notified lastObject'); }); -suite.test('[A,B].removeAt(1) => [A] + notify', function() { +suite.test('removeAt([A,B], 1) => [A] + notify', function() { let before = this.newFixture(2); let after = [before[0]]; let obj = this.newObject(before); @@ -59,7 +61,7 @@ suite.test('[A,B].removeAt(1) => [A] + notify', function() { obj.getProperties('firstObject', 'lastObject'); /* Prime the cache */ - equal(obj.removeAt(1), obj, 'return self'); + equal(removeAt(obj, 1), obj, 'return self'); deepEqual(this.toArray(obj), after, 'post item results'); equal(get(obj, 'length'), after.length, 'length'); @@ -72,7 +74,7 @@ suite.test('[A,B].removeAt(1) => [A] + notify', function() { equal(observer.validate('firstObject'), false, 'should NOT have notified firstObject once'); }); -suite.test('[A,B,C].removeAt(1) => [A,C] + notify', function() { +suite.test('removeAt([A,B,C], 1) => [A,C] + notify', function() { let before = this.newFixture(3); let after = [before[0], before[2]]; let obj = this.newObject(before); @@ -80,7 +82,7 @@ suite.test('[A,B,C].removeAt(1) => [A,C] + notify', function() { obj.getProperties('firstObject', 'lastObject'); /* Prime the cache */ - equal(obj.removeAt(1), obj, 'return self'); + equal(removeAt(obj, 1), obj, 'return self'); deepEqual(this.toArray(obj), after, 'post item results'); equal(get(obj, 'length'), after.length, 'length'); @@ -93,7 +95,7 @@ suite.test('[A,B,C].removeAt(1) => [A,C] + notify', function() { equal(observer.validate('lastObject'), false, 'should NOT have notified lastObject once'); }); -suite.test('[A,B,C,D].removeAt(1,2) => [A,D] + notify', function() { +suite.test('removeAt([A,B,C,D], 1,2) => [A,D] + notify', function() { let before = this.newFixture(4); let after = [before[0], before[3]]; let obj = this.newObject(before); @@ -101,7 +103,7 @@ suite.test('[A,B,C,D].removeAt(1,2) => [A,D] + notify', function() { obj.getProperties('firstObject', 'lastObject'); /* Prime the cache */ - equal(obj.removeAt(1, 2), obj, 'return self'); + equal(removeAt(obj, 1, 2), obj, 'return self'); deepEqual(this.toArray(obj), after, 'post item results'); equal(get(obj, 'length'), after.length, 'length'); diff --git a/packages/ember-runtime/tests/system/array_proxy/arranged_content_test.js b/packages/ember-runtime/tests/system/array_proxy/arranged_content_test.js index 29d0698f152..672df239dd6 100644 --- a/packages/ember-runtime/tests/system/array_proxy/arranged_content_test.js +++ b/packages/ember-runtime/tests/system/array_proxy/arranged_content_test.js @@ -1,6 +1,6 @@ import run from 'ember-metal/run_loop'; import { computed } from 'ember-metal/computed'; -import ArrayProxy from 'ember-runtime/system/array_proxy'; +import ArrayProxy, { removeAt } from 'ember-runtime/system/array_proxy'; import { A as emberA } from 'ember-runtime/system/native_array'; import { objectAt } from 'ember-runtime/mixins/array'; @@ -101,7 +101,7 @@ QUnit.test('pushObjects - adds multiple to end of content even if it already exi }); QUnit.test('removeAt - removes from index in arrangedContent', function() { - run(() => array.removeAt(1, 2)); + run(() => removeAt(array, 1, 2)); deepEqual(array.get('content'), [1, 5]); }); From 5f5407c589fb5a6b00109e55900b681c3f251e96 Mon Sep 17 00:00:00 2001 From: Bj Date: Mon, 23 May 2016 19:18:35 -0400 Subject: [PATCH 3/3] Add test for `.removeAt` Add test for `.removeAt` using prototype extensions. Update comments. --- .../ember-runtime/lib/mixins/mutable_array.js | 6 +- .../ember-runtime/lib/system/array_proxy.js | 63 +++++++++---------- .../tests/suites/mutable_array/removeAt.js | 23 ++++++- .../array_proxy/arranged_content_test.js | 4 +- 4 files changed, 57 insertions(+), 39 deletions(-) diff --git a/packages/ember-runtime/lib/mixins/mutable_array.js b/packages/ember-runtime/lib/mixins/mutable_array.js index 6de28cfbbe7..94fa461c839 100644 --- a/packages/ember-runtime/lib/mixins/mutable_array.js +++ b/packages/ember-runtime/lib/mixins/mutable_array.js @@ -146,9 +146,9 @@ export default Mixin.create(EmberArray, MutableEnumerable, { ```javascript let colors = ['red', 'green', 'blue', 'yellow', 'orange']; - removeAt(colors, 0); // ['green', 'blue', 'yellow', 'orange'] - removeAt(colors, 2, 2); // ['green', 'blue'] - removeAt(colors, 4, 2); // Error: Index out of range + colors.removeAt(0); // ['green', 'blue', 'yellow', 'orange'] + colors.removeAt(2, 2); // ['green', 'blue'] + colors.removeAt(4, 2); // Error: Index out of range ``` @method removeAt diff --git a/packages/ember-runtime/lib/system/array_proxy.js b/packages/ember-runtime/lib/system/array_proxy.js index 58e60a44c2c..f34a81174a8 100644 --- a/packages/ember-runtime/lib/system/array_proxy.js +++ b/packages/ember-runtime/lib/system/array_proxy.js @@ -35,38 +35,6 @@ const EMPTY = []; function K() { return this; } -export function removeAt(array, start, len) { - if ('number' === typeof start) { - let content = get(array, 'content'); - let arrangedContent = get(array, 'arrangedContent'); - let indices = []; - - if ((start < 0) || (start >= get(array, 'length'))) { - throw new EmberError(OUT_OF_RANGE_EXCEPTION); - } - - if (len === undefined) { - len = 1; - } - - // Get a list of indices in original content to remove - for (let i = start; i < start + len; i++) { - // Use arrangedContent here so we avoid confusion with objects transformed by objectAtContent - indices.push(content.indexOf(objectAt(arrangedContent, i))); - } - - // Replace in reverse order since indices will change - indices.sort((a, b) => b - a); - - beginPropertyChanges(); - for (let i = 0; i < indices.length; i++) { - array._replace(indices[i], 1, EMPTY); - } - endPropertyChanges(); - } - - return array; -} /** An ArrayProxy wraps any other object that implements `Ember.Array` and/or @@ -335,7 +303,36 @@ export default EmberObject.extend(MutableArray, { }, removeAt(start, len) { - return removeAt(this, start, len); + if ('number' === typeof start) { + let content = get(this, 'content'); + let arrangedContent = get(this, 'arrangedContent'); + let indices = []; + + if ((start < 0) || (start >= get(this, 'length'))) { + throw new EmberError(OUT_OF_RANGE_EXCEPTION); + } + + if (len === undefined) { + len = 1; + } + + // Get a list of indices in original content to remove + for (let i = start; i < start + len; i++) { + // Use arrangedContent here so we avoid confusion with objects transformed by objectAtContent + indices.push(content.indexOf(objectAt(arrangedContent, i))); + } + + // Replace in reverse order since indices will change + indices.sort((a, b) => b - a); + + beginPropertyChanges(); + for (let i = 0; i < indices.length; i++) { + this._replace(indices[i], 1, EMPTY); + } + endPropertyChanges(); + } + + return this; }, pushObject(obj) { diff --git a/packages/ember-runtime/tests/suites/mutable_array/removeAt.js b/packages/ember-runtime/tests/suites/mutable_array/removeAt.js index 5b26baaf961..c7d494d9a09 100644 --- a/packages/ember-runtime/tests/suites/mutable_array/removeAt.js +++ b/packages/ember-runtime/tests/suites/mutable_array/removeAt.js @@ -26,7 +26,6 @@ suite.test('removeAt([X], 0) => [] + notify', function() { equal(observer.timesCalled('lastObject'), 1, 'should have notified lastObject once'); }); -<<<<<<< f34dbabccfaa97c5d8989ce3ce4ec196e2450bbf suite.test('removeAt([], 200) => OUT_OF_RANGE_EXCEPTION exception', function() { let obj = this.newObject([]); throws(() => removeAt(obj, 200), Error); @@ -116,4 +115,26 @@ suite.test('removeAt([A,B,C,D], 1,2) => [A,D] + notify', function() { equal(observer.validate('lastObject'), false, 'should NOT have notified lastObject once'); }); +suite.test('[A,B,C,D].removeAt(1,2) => [A,D] + notify', function() { + var obj, before, after, observer; + + before = this.newFixture(4); + after = [before[0], before[3]]; + obj = this.newObject(before); + observer = this.newObserver(obj, '[]', '@each', 'length', 'firstObject', 'lastObject'); + obj.getProperties('firstObject', 'lastObject'); /* Prime the cache */ + + equal(obj.removeAt(1, 2), obj, 'return self'); + + deepEqual(this.toArray(obj), after, 'post item results'); + equal(get(obj, 'length'), after.length, 'length'); + + equal(observer.timesCalled('[]'), 1, 'should have notified [] once'); + equal(observer.timesCalled('@each'), 0, 'should not have notified @each once'); + equal(observer.timesCalled('length'), 1, 'should have notified length once'); + + equal(observer.validate('firstObject'), false, 'should NOT have notified firstObject once'); + equal(observer.validate('lastObject'), false, 'should NOT have notified lastObject once'); +}); + export default suite; diff --git a/packages/ember-runtime/tests/system/array_proxy/arranged_content_test.js b/packages/ember-runtime/tests/system/array_proxy/arranged_content_test.js index 672df239dd6..29d0698f152 100644 --- a/packages/ember-runtime/tests/system/array_proxy/arranged_content_test.js +++ b/packages/ember-runtime/tests/system/array_proxy/arranged_content_test.js @@ -1,6 +1,6 @@ import run from 'ember-metal/run_loop'; import { computed } from 'ember-metal/computed'; -import ArrayProxy, { removeAt } from 'ember-runtime/system/array_proxy'; +import ArrayProxy from 'ember-runtime/system/array_proxy'; import { A as emberA } from 'ember-runtime/system/native_array'; import { objectAt } from 'ember-runtime/mixins/array'; @@ -101,7 +101,7 @@ QUnit.test('pushObjects - adds multiple to end of content even if it already exi }); QUnit.test('removeAt - removes from index in arrangedContent', function() { - run(() => removeAt(array, 1, 2)); + run(() => array.removeAt(1, 2)); deepEqual(array.get('content'), [1, 5]); });