Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEATURE ember-runtime-enumerable-includes] Implements Array.includes and deprecates Array.contains #13553

Merged
merged 1 commit into from Jun 4, 2016

Conversation

@bmeurant
Copy link
Contributor

@bmeurant bmeurant commented May 24, 2016

Implementation of emberjs/rfcs#136

TODO :

  • Implement Enumerable.includes
  • Implement Array.includes
  • Deprecate contains under feature flag
  • Replace usages of contains by includes
  • Update docs, guides (emberjs/guides#1439) & API. To be merged once feature enabled.
  • Write deprecation guide (emberjs/website#2600) & add url in deprecate call
  • Rebase / squash
@alexspeller
Copy link
Contributor

@alexspeller alexspeller commented May 24, 2016

Oh awesome, thanks for getting this implementation done!

@bmeurant
Copy link
Contributor Author

@bmeurant bmeurant commented May 24, 2016

Here is a first proposal for emberjs/rfcs#136. Work is in progress and feedbacks expected.

I made the asumption that we have to keep Enumerable & Array APIs consistent and then have to provide a consistent includes method in Enumerable (with no index). Let me know if this is not expected.

Aliasing contains with the new includes method can break some existing apps because, according to the spec (https://tc39.github.io/ecma262/2016/#sec-array.prototype.includes), includes relies on the SameValueZero algorithm and actual contains relies on strict equality leading to differences in NaN detections (I will document it). Can we still make this ?

@mixonic
Copy link
Member

@mixonic mixonic commented May 24, 2016

@bmeurant Thanks :-)

contains should not be aliased to includes. Contains should keep its current implementation, and we can deprecate it though we will likely do that in a separate PR. In this PR it should simply retain its current behavior.

@bmeurant bmeurant force-pushed the bmeurant:master branch May 24, 2016
@bmeurant
Copy link
Contributor Author

@bmeurant bmeurant commented May 24, 2016

@mixonic thx for your feedback.

I updated this PR to keep current implementation for Enumerable.contains & Array.contains.

we can deprecate it though we will likely do that in a separate PR

Does it means that I should also remove @deprecated and deprecation warning for now ?

@rwjblue
Copy link
Member

@rwjblue rwjblue commented May 24, 2016

I believe that the deprecation should be included here when the feature flag is enabled.

@mixonic
Copy link
Member

@mixonic mixonic commented May 24, 2016

@rwjblue I am easily convinced, was just erring on the conservative side. @bmeurant please include the deprecation behind the feature flag! :-)

@bmeurant bmeurant force-pushed the bmeurant:master branch May 25, 2016
@bmeurant
Copy link
Contributor Author

@bmeurant bmeurant commented May 25, 2016

@mixonic I added feature flag and move deprecation behind it. Let me know if I forgotten something.

I also removed @deprecated from API because contains is not actually deprecated if feature is not enabled, right ?

But I was wondering: doing that, users will loose the information that contains will be deprecated soon and that they should use includes instead. Is it exists a way to provide this kind of information in APIs ?

@bmeurant bmeurant force-pushed the bmeurant:master branch May 25, 2016
@bmeurant bmeurant force-pushed the bmeurant:master branch May 25, 2016
@bmeurant bmeurant changed the title [WIP][FEATURE contains-to-includes] Implements Array.includes and deprecates Array.contains [FEATURE contains-to-includes] Implements Array.includes and deprecates Array.contains May 25, 2016
@bmeurant bmeurant force-pushed the bmeurant:master branch May 25, 2016
@mixonic
Copy link
Member

@mixonic mixonic commented May 27, 2016

@bmeurant Yeah the documentation may require some followup tweaks. Feature flagging things like the @deprecated flag is tricky :-/. I think you should feel free to create a followup issue of things to do once the feature is "go'd" for beta.

@mixonic
mixonic reviewed May 27, 2016
View changes
FEATURES.md Outdated

* `ember-runtime-enumerable-contains`

Deprecates `Enumerable.contains` and `Array.contains` in favor of `Enumerable.includes` and `Array.includes`

This comment has been minimized.

@mixonic

mixonic May 27, 2016
Member

Again here I lean toward Enumerable#contains, per emberjs/website#2600 (comment). It is a Rubyish approach, curious if there are other thoughts.

This comment has been minimized.

@alexspeller

alexspeller May 27, 2016
Contributor

I think Enumerable#contains is the convention in Ember even though it was originally a rubyism - at least it has the advantage that it's not valid syntax for something that is completely different

(See for example the changelog where it's used extensively)

@mixonic
mixonic reviewed May 27, 2016
View changes
packages/ember-runtime/lib/mixins/enumerable.js Outdated
@return {Boolean} `true` if object is found in the enumerable.
@public
*/
includes(obj) {

This comment has been minimized.

@mixonic

mixonic May 27, 2016
Member

Can you add an assertion that there is no second argument? Something like:

  includes(obj) {
    assert('Enumerable#includes cannot accept a second argument "startAt" as enumerable items are unordered.', arguments.length === 1);
@mixonic
mixonic reviewed May 27, 2016
View changes
packages/ember-runtime/lib/mixins/mutable_array.js Outdated
@@ -388,7 +388,7 @@ export default Mixin.create(EmberArray, MutableEnumerable, {
@public
*/
addObject(obj) {
if (!this.contains(obj)) {
if (!this.includes(obj)) {

This comment has been minimized.

@mixonic

mixonic May 27, 2016
Member

This is a small breaking change. I've left a note that it be called out in the deprecation guide emberjs/website#2600 (comment). Here and without.

This comment has been minimized.

@bmeurant

bmeurant May 27, 2016
Author Contributor

Sure. Moreover, I think I have to use includes instead of contains only behind feature flag, right?

This comment has been minimized.

@mixonic

mixonic May 27, 2016
Member

👍 yup


var suite = SuiteModuleBuilder.create();

suite.module('includes');

This comment has been minimized.

@mixonic

mixonic May 27, 2016
Member

Should include a test/tests without a starting position

This comment has been minimized.

@bmeurant

bmeurant May 27, 2016
Author Contributor

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

This comment has been minimized.

@mixonic

mixonic May 27, 2016
Member

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?

@mixonic
Copy link
Member

@mixonic mixonic commented May 27, 2016

@bmeurant heh, there are no tests for Array#contains?! Can you add one asserting basic behavior and the deprecation please?

This is looking pretty good after some tweaks!

@mixonic
mixonic reviewed May 27, 2016
View changes
packages/ember-runtime/lib/mixins/array.js Outdated
@return {Boolean} `true` if object is found in the array.
@public
*/
includes(obj, startAt) {

This comment has been minimized.

@mixonic

mixonic May 27, 2016
Member

This function (and the enumerable one) must be added only when the feature flag is enabled. For example:

let ArrayMixin = Mixin.create(Enumerable, {
 /* snip */
});
if (isEnabled('ember-runtime-enumerable-includes')) {
  ArrayMixin.reopen({
    includes(obj, startAt) {
      /* ... */
    }
  });
}
export default ArrayMixin;
@mixonic
mixonic reviewed May 27, 2016
View changes
packages/ember-runtime/tests/suites/enumerable/includes.js Outdated

suite.module('includes');

suite.test('includes returns true if items is in enumerable', function() {

This comment has been minimized.

@mixonic

mixonic May 27, 2016
Member

Once the method itself is behind a feature flag, you will also need to wrap these tests inside an isEnabled check.

@mixonic
mixonic reviewed May 27, 2016
View changes
features.json Outdated
@@ -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-contains": null

This comment has been minimized.

@mixonic

mixonic May 27, 2016
Member

Lastly the feature flag here seems poorly named. Perhaps ember-runtime-enumerable-includes

@mixonic
mixonic reviewed May 27, 2016
View changes
packages/ember-runtime/lib/mixins/enumerable.js Outdated
return item === obj || (item !== item && obj !== obj);
});

return found !== undefined;

This comment has been minimized.

@mixonic

mixonic May 27, 2016
Member

Wouldn't this break for includes(undefined)? Can you add a test?

Likely should be refactored to do something more similar to the array version, or to the internals of find here.

This comment has been minimized.

@bmeurant

bmeurant May 27, 2016
Author Contributor

Right ! I missed it, sorry :-(

@bmeurant
Copy link
Contributor Author

@bmeurant bmeurant commented May 27, 2016

Thx for feedbacks, I'll work on it ASAP

@bmeurant
Copy link
Contributor Author

@bmeurant bmeurant commented May 27, 2016

It seemed to me that Array#contains tests are also imported and run from Enumerable#contains tests. Did I miss something ?

@mixonic
Copy link
Member

@mixonic mixonic commented May 27, 2016

Ah I see @bmeurant :-) right there is no contains on arrays, only on enumerable 👍

@bmeurant
Copy link
Contributor Author

@bmeurant bmeurant commented May 27, 2016

I wanted to add a test to verify assertion calling Enumerable#includes with a second argument but, because tests defined on Enumerable are also run on Array suites, I did not find a good way to do it. Any advice ?

@bmeurant bmeurant force-pushed the bmeurant:master branch May 27, 2016
@bmeurant
bmeurant reviewed May 27, 2016
View changes
packages/ember-runtime/lib/mixins/enumerable.js Outdated
@@ -811,7 +820,9 @@ var Enumerable = Mixin.create({
@public
*/
without(value) {

This comment has been minimized.

@bmeurant

bmeurant May 27, 2016
Author Contributor

I think we have a problem line 832 during k !== value. If the feature is enabled, without becomes inconsistent because NaN !== NaN is true

@bmeurant bmeurant force-pushed the bmeurant:master branch May 27, 2016
return found;
},

without(value) {

This comment has been minimized.

@bmeurant

bmeurant May 27, 2016
Author Contributor

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

This comment has been minimized.

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' }

This comment has been minimized.

@bmeurant

bmeurant May 27, 2016
Author Contributor

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

This comment has been minimized.

@mixonic

mixonic May 30, 2016
Member

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

},

without(value) {
if (!this.includes(value)) {

This comment has been minimized.

@mixonic

mixonic May 30, 2016
Member

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.

This comment has been minimized.

@bmeurant

bmeurant May 30, 2016
Author Contributor

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.

This comment has been minimized.

@mixonic

mixonic May 30, 2016
Member

👍 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.

@mixonic
mixonic reviewed May 30, 2016
View changes
packages/ember/tests/component_registration_test.js Outdated
keys(helpers).
forEach((name) => {
if (!originalHelpers.contains(name)) {
included = isEnabled('ember-runtime-enumerable-includes') ? originalHelpers.includes(name) : originalHelpers.contains(name);

This comment has been minimized.

@mixonic

mixonic May 30, 2016
Member

Unfortunately the ternary syntax is not supported by feature flag stripping. Please rewrite this to use an if statement.

@mixonic
mixonic reviewed May 30, 2016
View changes
packages/ember-runtime/lib/mixins/mutable_array.js Outdated
@@ -388,7 +389,9 @@ export default Mixin.create(EmberArray, MutableEnumerable, {
@public
*/
addObject(obj) {
if (!this.contains(obj)) {
var included = isEnabled('ember-runtime-enumerable-includes') ? this.includes(obj) : this.contains(obj);

This comment has been minimized.

@mixonic

mixonic May 30, 2016
Member

Unfortunately the ternary syntax is not supported by feature flag stripping. Please rewrite this to use an if statement.

@mixonic
Copy link
Member

@mixonic mixonic commented May 30, 2016

@bmeurant it seems ok to add a test for contains without an offset in packages/ember-runtime/tests/mixins/enumerable_test.js, since it only impacts enumerables.

@mixonic
Copy link
Member

@mixonic mixonic commented May 30, 2016

Thanks for your continued work on this 😄 it looks really close, and quite nice!

@bmeurant bmeurant force-pushed the bmeurant:master branch May 30, 2016
@mixonic
Copy link
Member

@mixonic mixonic commented May 30, 2016

Looking nice. A rebase/squash to some number is semantically meaningful commits (one or more) would be great, each prefixed with [FEATURE ember-runtime-enumerable-includes] would be great.

@mixonic mixonic changed the title [FEATURE contains-to-includes] Implements Array.includes and deprecates Array.contains [FEATURE ember-runtime-enumerable-includes] Implements Array.includes and deprecates Array.contains May 30, 2016
@bmeurant bmeurant force-pushed the bmeurant:master branch May 30, 2016
@bmeurant
Copy link
Contributor Author

@bmeurant bmeurant commented May 30, 2016

Done 👌

@homu
Copy link
Contributor

@homu homu commented Jun 1, 2016

The latest upstream changes (presumably #13328) made this pull request unmergeable. Please resolve the merge conflicts.

@bmeurant bmeurant force-pushed the bmeurant:master branch Jun 2, 2016
@mixonic
Copy link
Member

@mixonic mixonic commented Jun 3, 2016

Looks pretty great to me @bmeurant! Thank you! I'll get someone else on core to review and we can :shipit: with their 👍

@rwjblue
rwjblue reviewed Jun 3, 2016
View changes
packages/ember-runtime/lib/mixins/array.js Outdated
var len = get(this, 'length');
var idx, currentObj;

if (Ember.isNone(startAt)) {

This comment has been minimized.

@rwjblue

rwjblue Jun 3, 2016
Member

Should not use the global here (Ember.isNone), if we need isNone it should be imported. Can we just do the null / undefined check ourselves?

This comment has been minimized.

@bmeurant

bmeurant Jun 4, 2016
Author Contributor

@rwjblue using the global here was surely a mistake, sorry. isNone is already imported and used in slice. I'll update. But why do you prefer to not use imported isNone and doing null / undefined check ourselves in this case?

This comment has been minimized.

@bmeurant

bmeurant Jun 4, 2016
Author Contributor

Anyway, explicit null check is unnecessary as we only iterating on idx = startAt. I removed it.

@rwjblue
Copy link
Member

@rwjblue rwjblue commented Jun 3, 2016

This looks good, only one small tweak and this is good to go...

…udes

and deprecate Enumerable#contains
@bmeurant bmeurant force-pushed the bmeurant:master branch to 732e907 Jun 4, 2016
@mixonic mixonic merged commit b0332ef into emberjs:master Jun 4, 2016
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@mixonic
Copy link
Member

@mixonic mixonic commented Jun 4, 2016

This is great! Thank you @bmeurant!

@homu homu mentioned this pull request Jun 4, 2016
2 of 2 tasks complete
@rwjblue
Copy link
Member

@rwjblue rwjblue commented Aug 25, 2016

FYI - I created https://github.com/rwjblue/ember-runtime-enumerable-includes-polyfill to make this a bit easier for addons to avoid deprecations, use the newer syntax, and continue to support older versions of Ember.

homu added a commit to emberjs/guides that referenced this pull request Oct 18, 2016
Update documentation to move from contains to includes

Related to emberjs/ember.js#13553.

To be merged when feature has been released.
ecksun pushed a commit to Manetos/ember-searchable-select that referenced this pull request Sep 4, 2018
This is required as .contains seems to be deprecated in ember-3.

See emberjs/ember.js#13553 for more information
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.