Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions FEATURES.md
Original file line number Diff line number Diff line change
Expand Up @@ -57,3 +57,8 @@ for a detailed explanation.
- `interaction.<event-name>` for events handled by a component.
- `interaction.ember-action` for closure actions.
- `interaction.link-to` for link-to execution.

* `ember-runtime-enumerable-includes`

Deprecates `Enumerable#contains` and `Array#contains` in favor of `Enumerable#includes` and `Array#includes`
to stay in line with ES standards (see [RFC](https://github.com/emberjs/rfcs/blob/master/text/0136-contains-to-includes.md)).
3 changes: 2 additions & 1 deletion features.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
"ember-route-serializers": null,
"ember-glimmer": null,
"ember-runtime-computed-uniq-by": null,
"ember-improved-instrumentation": null
"ember-improved-instrumentation": null,
"ember-runtime-enumerable-includes": null
}
}
64 changes: 63 additions & 1 deletion packages/ember-runtime/lib/mixins/array.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ import {
import { meta as metaFor } from 'ember-metal/meta';
import { markObjectAsDirty } from 'ember-metal/tags';
import EachProxy from 'ember-runtime/system/each_proxy';
import { deprecate } from 'ember-metal/debug';
import isEnabled from 'ember-metal/features';

function arrayObserversHelper(obj, target, opts, operation, notify) {
var willChange = (opts && opts.willChange) || 'arrayWillChange';
Expand Down Expand Up @@ -112,7 +114,7 @@ export function isEmberArray(obj) {
@since Ember 0.9.0
@public
*/
export default Mixin.create(Enumerable, {
var ArrayMixin = Mixin.create(Enumerable, {

[EMBER_ARRAY]: true,

Expand Down Expand Up @@ -214,6 +216,14 @@ export default Mixin.create(Enumerable, {

// optimized version from Enumerable
contains(obj) {
if (isEnabled('ember-runtime-enumerable-includes')) {
deprecate(
'`Enumerable#contains` is deprecated, use `Enumerable#includes` instead.',
false,
{ id: 'ember-runtime.enumerable-contains', until: '3.0.0', url: 'http://emberjs.com/deprecations/v2.x#toc_enumerable-contains' }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not change deprecation id because feature is about includes but deprecation is about contains.

But I don't know if it is accepted. Let me know

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the deprecation id and feature id are totally distinct. Seems fine to use different names 👍

);
}

return this.indexOf(obj) >= 0;
},

Expand Down Expand Up @@ -573,3 +583,55 @@ export default Mixin.create(Enumerable, {
return this.__each;
}).volatile()
});

if (isEnabled('ember-runtime-enumerable-includes')) {
ArrayMixin.reopen({
/**
Returns `true` if the passed object can be found in the array.
This method is a Polyfill for ES 2016 Array.includes.
If no `startAt` argument is given, the starting location to
search is 0. If it's negative, searches from the index of
`this.length + startAt` by asc.
```javascript
[1, 2, 3].includes(2); // true
[1, 2, 3].includes(4); // false
[1, 2, 3].includes(3, 2); // true
[1, 2, 3].includes(3, 3); // false
[1, 2, 3].includes(3, -1); // true
[1, 2, 3].includes(1, -1); // false
[1, 2, 3].includes(1, -4); // true
[1, 2, NaN].includes(NaN); // true
```
@method includes
@param {Object} obj The object to search for.
@param {Number} startAt optional starting location to search, default 0
@return {Boolean} `true` if object is found in the array.
@public
*/
includes(obj, startAt) {
var len = get(this, 'length');
var idx, currentObj;

if (startAt === undefined) {
startAt = 0;
}

if (startAt < 0) {
startAt += len;
}

for (idx = startAt; idx < len; idx++) {
currentObj = objectAt(this, idx);

// SameValueZero comparison (NaN !== NaN)
if (obj === currentObj || (obj !== obj && currentObj !== currentObj)) {
return true;
}
}

return false;
}
});
}

export default ArrayMixin;
72 changes: 70 additions & 2 deletions packages/ember-runtime/lib/mixins/enumerable.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
} from 'ember-metal/events';
import compare from 'ember-runtime/compare';
import require from 'require';
import { assert, deprecate } from 'ember-metal/debug';

let _emberA;

Expand Down Expand Up @@ -231,6 +232,14 @@ var Enumerable = Mixin.create({
@public
*/
contains(obj) {
if (isEnabled('ember-runtime-enumerable-includes')) {
deprecate(
'`Enumerable#contains` is deprecated, use `Enumerable#includes` instead.',
false,
{ id: 'ember-runtime.enumerable-contains', until: '3.0.0', url: 'http://emberjs.com/deprecations/v2.x#toc_enumerable-contains' }
);
}

var found = this.find(function(item) {
return item === obj;
});
Expand Down Expand Up @@ -797,8 +806,8 @@ var Enumerable = Mixin.create({

/**
Returns a new enumerable that excludes the passed value. The default
implementation returns an array regardless of the receiver type unless
the receiver does not contain the value.
implementation returns an array regardless of the receiver type.
If the receiver does not contain the value it returns the original enumerable.

```javascript
var arr = ['a', 'b', 'a', 'c'];
Expand Down Expand Up @@ -1118,4 +1127,63 @@ if (isEnabled('ember-runtime-computed-uniq-by')) {
});
}

if (isEnabled('ember-runtime-enumerable-includes')) {
Enumerable.reopen({
/**
Returns `true` if the passed object can be found in the enumerable.
```javascript
[1, 2, 3].includes(2); // true
[1, 2, 3].includes(4); // false
[1, 2, undefined].includes(undefined); // true
[1, 2, null].includes(null); // true
[1, 2, NaN].includes(NaN); // true
```
@method includes
@param {Object} obj The object to search for.
@return {Boolean} `true` if object is found in the enumerable.
@public
*/
includes(obj) {
assert('Enumerable#includes cannot accept a second argument "startAt" as enumerable items are unordered.', arguments.length === 1);

var len = get(this, 'length');
var idx, next;
var last = null;
var found = false;

var context = popCtx();

for (idx = 0; idx < len && !found; idx++) {
next = this.nextObject(idx, last, context);

found = obj === next || (obj !== obj && next !== next);

last = next;
}

next = last = null;
context = pushCtx(context);

return found;
},

without(value) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefered replacing without behind feature flag to not add multiple isEnabled calls in original method

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

if (!this.includes(value)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Calling includes before the forEach means we walk the contents of the array twice. Additionally it seems a bit unexpected for without() to sometimes return a new array, and sometimes return the array passed in.

I'd prefer to drop the includes check and just run the array builder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd also prefer but it seems that returning the same instance if not included (or not contained), is part of the API since it is tested here : https://github.com/emberjs/ember.js/blob/master/packages/ember-runtime/tests/suites/enumerable/without.js#L19

Does this PR should also change this behaviour?

I could make it, of course, and update the deprecation guide. But in any case, I propose to update without API doc to explicitely mention it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 to updating the docs and keeping the current behavior. Great catch.

The without method on enumerables and on mixins just should just 🔥 🔥 🔥. On enumerables we can just use reject which seems more sane, and on mixins the functionality is only used for native array prototype application. I attempted killing it once and ran out of steam.

return this; // nothing to do
}

var ret = emberA();

this.forEach(function(k) {
// SameValueZero comparison (NaN !== NaN)
if (!(k === value || k !== k && value !== value)) {
ret[ret.length] = k;
}
});

return ret;
}
});
}

export default Enumerable;
11 changes: 10 additions & 1 deletion packages/ember-runtime/lib/mixins/mutable_array.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import { Mixin } from 'ember-metal/mixin';
import EmberArray, { objectAt } from 'ember-runtime/mixins/array';
import MutableEnumerable from 'ember-runtime/mixins/mutable_enumerable';
import Enumerable from 'ember-runtime/mixins/enumerable';
import isEnabled from 'ember-metal/features';

/**
This mixin defines the API for modifying array-like objects. These methods
Expand Down Expand Up @@ -388,7 +389,15 @@ export default Mixin.create(EmberArray, MutableEnumerable, {
@public
*/
addObject(obj) {
if (!this.contains(obj)) {
var included;

if (isEnabled('ember-runtime-enumerable-includes')) {
included = this.includes(obj);
} else {
included = this.contains(obj);
}

if (!included) {
this.pushObject(obj);
}

Expand Down
26 changes: 24 additions & 2 deletions packages/ember-runtime/tests/mixins/enumerable_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { A as emberA } from 'ember-runtime/system/native_array';
import { get } from 'ember-metal/property_get';
import { computed } from 'ember-metal/computed';
import { observer as emberObserver } from 'ember-metal/mixin';
import isEnabled from 'ember-metal/features';

function K() { return this; }

Expand Down Expand Up @@ -93,11 +94,21 @@ QUnit.test('should apply Ember.Array to return value of toArray', function() {
});

QUnit.test('should apply Ember.Array to return value of without', function() {
var x = EmberObject.extend(Enumerable, {
var X = EmberObject.extend(Enumerable, {
contains() {
return true;
}
}).create();
});

if (isEnabled('ember-runtime-enumerable-includes')) {
X.reopen({
includes() {
return true;
}
});
}

var x = X.create();
var y = x.without(K);
equal(EmberArray.detect(y), true, 'should have mixin applied');
});
Expand Down Expand Up @@ -164,6 +175,17 @@ QUnit.test('every', function() {
equal(allWhite, true);
});

if (isEnabled('ember-runtime-enumerable-includes')) {
QUnit.test('should throw an error passing a second argument to includes', function() {
var x = EmberObject.extend(Enumerable).create();

equal(x.includes('any'), false);
expectAssertion(() => {
x.includes('any', 1);
}, /Enumerable#includes cannot accept a second argument "startAt" as enumerable items are unordered./);
});
}

// ..........................................................
// CONTENT DID CHANGE
//
Expand Down
6 changes: 6 additions & 0 deletions packages/ember-runtime/tests/suites/array.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@ import {
import indexOfTests from 'ember-runtime/tests/suites/array/indexOf';
import lastIndexOfTests from 'ember-runtime/tests/suites/array/lastIndexOf';
import objectAtTests from 'ember-runtime/tests/suites/array/objectAt';
import includesTests from 'ember-runtime/tests/suites/array/includes';
import {
addArrayObserver,
removeArrayObserver
} from 'ember-runtime/mixins/array';
import isEnabled from 'ember-metal/features';

var ObserverClass = EnumerableTestsObserverClass.extend({

Expand Down Expand Up @@ -46,4 +48,8 @@ ArrayTests.importModuleTests(indexOfTests);
ArrayTests.importModuleTests(lastIndexOfTests);
ArrayTests.importModuleTests(objectAtTests);

if (isEnabled('ember-runtime-enumerable-includes')) {
ArrayTests.importModuleTests(includesTests);
}

export {ArrayTests, ObserverClass};
39 changes: 39 additions & 0 deletions packages/ember-runtime/tests/suites/array/includes.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
import {SuiteModuleBuilder} from 'ember-runtime/tests/suites/suite';

var suite = SuiteModuleBuilder.create();

suite.module('includes');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should include a test/tests without a starting position

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tests without starting position are imported from Enumerable test suite, no ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure what you mean, maybe I am missing something. This code tests the implementation of Array#includes. How are the tests for Enumerable#includes applied to an array?


suite.test('includes returns correct value if startAt is positive', function() {
var data = this.newFixture(3);
var obj = this.newObject(data);

equal(obj.includes(data[1], 1), true, 'should return true if included');
equal(obj.includes(data[0], 1), false, 'should return false if not included');
});

suite.test('includes returns correct value if startAt is negative', function() {
var data = this.newFixture(3);
var obj = this.newObject(data);

equal(obj.includes(data[1], -2), true, 'should return true if included');
equal(obj.includes(data[0], -2), false, 'should return false if not included');
});

suite.test('includes returns true if startAt + length is still negative', function() {
var data = this.newFixture(1);
var obj = this.newObject(data);

equal(obj.includes(data[0], -2), true, 'should return true if included');
equal(obj.includes(this.newFixture(1), -2), false, 'should return false if not included');
});

suite.test('includes returns false if startAt out of bounds', function() {
var data = this.newFixture(1);
var obj = this.newObject(data);

equal(obj.includes(data[0], 2), false, 'should return false if startAt >= length');
equal(obj.includes(this.newFixture(1), 2), false, 'should return false if startAt >= length');
});

export default suite;
5 changes: 5 additions & 0 deletions packages/ember-runtime/tests/suites/enumerable.js
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,7 @@ import anyTests from 'ember-runtime/tests/suites/enumerable/any';
import isAnyTests from 'ember-runtime/tests/suites/enumerable/is_any';
import compactTests from 'ember-runtime/tests/suites/enumerable/compact';
import containsTests from 'ember-runtime/tests/suites/enumerable/contains';
import includesTests from 'ember-runtime/tests/suites/enumerable/includes';
import everyTests from 'ember-runtime/tests/suites/enumerable/every';
import filterTests from 'ember-runtime/tests/suites/enumerable/filter';
import findTests from 'ember-runtime/tests/suites/enumerable/find';
Expand Down Expand Up @@ -325,6 +326,10 @@ if (isEnabled('ember-runtime-computed-uniq-by')) {
EnumerableTests.importModuleTests(uniqByTests);
}

if (isEnabled('ember-runtime-enumerable-includes')) {
EnumerableTests.importModuleTests(includesTests);
}

EnumerableTests.importModuleTests(withoutTests);

export default EnumerableTests;
Expand Down
11 changes: 10 additions & 1 deletion packages/ember-runtime/tests/suites/enumerable/contains.js
Original file line number Diff line number Diff line change
@@ -1,18 +1,27 @@
import {SuiteModuleBuilder} from 'ember-runtime/tests/suites/suite';
import isEnabled from 'ember-metal/features';

var suite = SuiteModuleBuilder.create();

suite.module('contains');

suite.test('contains returns true if items is in enumerable', function() {
suite.test('contains returns true if item is in enumerable', function() {
var data = this.newFixture(3);
var obj = this.newObject(data);

if (isEnabled('ember-runtime-enumerable-includes')) {
expectDeprecation('`Enumerable#contains` is deprecated, use `Enumerable#includes` instead.');
}
equal(obj.contains(data[1]), true, 'should return true if contained');
});

suite.test('contains returns false if item is not in enumerable', function() {
var data = this.newFixture(1);
var obj = this.newObject(this.newFixture(3));

if (isEnabled('ember-runtime-enumerable-includes')) {
expectDeprecation('`Enumerable#contains` is deprecated, use `Enumerable#includes` instead.');
}
equal(obj.contains(data[0]), false, 'should return false if not contained');
});

Expand Down
Loading