Skip to content

Add contains to includes RFC#136

Merged
mixonic merged 1 commit intoemberjs:masterfrom
alexspeller:contains-to-includes
Apr 30, 2016
Merged

Add contains to includes RFC#136
mixonic merged 1 commit intoemberjs:masterfrom
alexspeller:contains-to-includes

Conversation

@alexspeller
Copy link
Contributor

@alexspeller alexspeller commented Apr 16, 2016

contains is implemented on Ember.Array, but contains was renamed to includes in 2014 - should contains be deprecated in favour of an includes method in ember?

Rendered

@mixonic
Copy link
Member

mixonic commented Apr 17, 2016

@alexspeller includes would be an extension of the native array prototype, and thus should be 100% API compatible with the native implementation that will replace it when present. For example includes (MDN docs here) takes an optional fromIndex.

Basically, I suggest that includes be a polyfill for the spec'd behavior. Then contains can be deprecated. However just renaming contains -> includes is not sufficient.

@alexspeller alexspeller force-pushed the contains-to-includes branch from fb72b45 to d0d6fa0 Compare April 17, 2016 23:45
@alexspeller
Copy link
Contributor Author

@mixonic updated RFC to reflect that


# How We Teach This

Update any references in docs and guides to `includes`
Copy link
Contributor

Choose a reason for hiding this comment

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

A deprecation guide also needs to get written, and since includes and contains seem to have slightly different semantics, the guide should address that with examples of how to migrate existing code.

Copy link
Contributor

Choose a reason for hiding this comment

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

We might also want to indicate in the API documentation that this is a polyfill.

@alexspeller alexspeller force-pushed the contains-to-includes branch from d0d6fa0 to b81ccfb Compare April 30, 2016 18:09
@alexspeller
Copy link
Contributor Author

@locks updated based on your comments

@rwjblue
Copy link
Member

rwjblue commented Apr 30, 2016

We discussed in the core team meeting yesterday, we are 👍 on this.

@mixonic mixonic merged commit 8e9f588 into emberjs:master Apr 30, 2016
@mixonic
Copy link
Member

mixonic commented Apr 30, 2016

:-D

@bmeurant
Copy link
Contributor

bmeurant commented May 24, 2016

I think I can try to handle this if still available

One thing to confirm, it has to be implemented also in Enumerable (without fromIndex), right ? Not only in Array ?

@rwjblue
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants