mootools / mootools-core

MooTools Core Repository
https://mootools.net
2.65k stars 510 forks source link

Use Array.convert instead of Array.from, add compat layer for Array.from #2758

Closed SergioCrisostomo closed 8 years ago

SergioCrisostomo commented 8 years ago

ES6 implemented Array.from, a method name that MooTools has since at least 2010. This pull request changes the MooTools implementation of Array.from to use the new native implementation. For older browsers that need a polyfill I added MDN's polyfill to make it available to older browsers also.

For compatibility and consistency added Array.convert which is the old Array.from implementation, thus making it easier to update pre-change code. The old Array.from implementation is not changed in the compat layer, still overriding the native implementation.

Since the old implementation was overwriting the native I think we don't need to change older versions.

Tested everything in Travis and got green.

Fixes #2757

anutron commented 8 years ago

I still think we should alias all the other .from static methods on the other natives to use .convert and make .from deprecated (but still present) for everything but Array.

/my 2 cents

Code LGTM.

arian commented 8 years ago

Should we add the Array.from? I think just changing Array.from to Array.convert should do the trick. Historically $splat or Array.from is used internally, because it's useful for other things, but also to be used outside. Inside MooTools there isn't really a use case for the new Array.from.

Additionally I suspect people that want to use the new Array.from either assume a modern environment, or use babel/polyfill or similar.

arian commented 8 years ago

Maybe you should do a search-replace through the entire docs, as Array.from is mentioned here too: http://mootools.net/core/docs/1.5.2/Core/Core#Deprecated-Functions:splat

arian commented 8 years ago

If we decide to add the polyfill, we also need to test it ;)

anutron commented 8 years ago

@arian I think we should maintain backwards compat for users who need it whenever possible. I.e. if the user downloads the compat version Array.from is there. I'm not sure if that's what you're suggesting when you ask if we should add it. Looking at my own work, there are 11 instances in all my Behavior stuff. I can search and replace it to upgrade, that's fine, but my point is that it's not only used in Core.

SergioCrisostomo commented 8 years ago

I agree with renaming, deprecating it and keep it only in the compat layer. I think that is what @arian also meant.

If so I can update it, and take into account the things @arian pointed out in review.

arian commented 8 years ago

@anutron of course we should maintain backwards compat (with the compat layer stuff), so Array.from = Array.convert.

What I'm not sure of is whether we should add the new ES6 version as polyfill.

anutron commented 8 years ago

Ah. I gotcha.

How complete is our ES6 polyfill currently? Is Array.from like, the only thing missing? Do we even profess to offer an ES6 shim? Those are easily found in stand alone projects, right? I think I'd vote no, unless this is the only thing we don't fill in.

arian commented 8 years ago

Looking at this pull request, @SergioCrisostomo just copied the one from MDN, which is likely pretty complete. However I don't think it's a good idea to just include it, as there are many other polyfills (or modern browsers) that developers can easily add themselves if needed.

Of course, as I said earlier, if MooTools itself would make heavy use of this functionality, such as other ES5 methods like bind/forEach/map, then it would make more sense.

SergioCrisostomo commented 8 years ago

Updated the pull request as sugested in discussion and fixing the things arian pointed out.

@arian @anutron I agree that polyfill is very easy to find if needed, so I removed the polyfill from the PR.

SergioCrisostomo commented 8 years ago

bump... :) merge anyone?

anutron commented 8 years ago

LGTM. Ship it.

anutron commented 8 years ago

Sergio, why don't you go ahead and ship this to master? I don't think you need someone else to do it after the review is done...

arian commented 8 years ago

One major concern, however, is the API consistency with String.from / Number.from / Function.from.

I suppose all of those need to change too, otherwise it would be inconsistent and weird.

anutron commented 8 years ago

I made that comment earlier. Don't disagree. I would only remove Array.from though; you could leave the old ones there and alias it to both .from and .convert, then just update the docs to refer to .convert.

arian commented 8 years ago

+1 to convert to .convert for those too, like you said.

SergioCrisostomo commented 8 years ago

@arian, @anutron: I am ok with removing/renaming them also.

Can I do that in a separate PR?

arian commented 8 years ago

:+1: