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

Update Array.prototype.flatten to match TC39 proposal #2797

Open
callmevlad opened this Issue Mar 7, 2018 · 13 comments

Comments

Projects
None yet
@callmevlad

callmevlad commented Mar 7, 2018

Array.prototype.flatten and Array.prototype.flatMap are already on Stage 3 (Candidate) of the TC39 standards process and are starting to land in browsers.

There's currently a discussion happening here around avoiding the flatten name due to the conflict with Mootools core, which was a response to this Bugzilla report.

Since flatten is the commonly accepted name for this type of operation, it seems unreasonable for the name to change for the entire spec to something more esoteric just because of the Mootools conflict.

Can this library be modified to match the upcoming TC39 standard for Array.prototype.flatten? 🙏

@cpojer

This comment has been minimized.

Member

cpojer commented Mar 7, 2018

It could be modified but MooTools isn't installed through npm (it didn't exist when MooTools was popular) and websites that still use it today aren't likely to update. Updating the code in this repo will not resolve your issue with Array.prototype.flatten unless you also find a way to update every (unmaintained) legacy website that still uses MooTools.

@gkatsanos

This comment has been minimized.

gkatsanos commented Mar 7, 2018

It's a start?

@cpojer

This comment has been minimized.

Member

cpojer commented Mar 7, 2018

Sure, feel free to send a PR with a fix. However, please also consider this tweet for more perspective: https://twitter.com/tomdale/status/971288054966247430

I am going to venture a guess that most of the sites that will break are not using MooTools 1.3 to 1.6 but rather 1.1 or 1.2. These sites haven't been updated in more than half a decade and it's unlikely that they will even if you make a change to the code in this repository.

@timwienk

This comment has been minimized.

Member

timwienk commented Mar 7, 2018

That being said, MooTools' aim has always been to help improve the JavaScript landscape, and I'd like to think we really did help do that.

Apart from the prototype extension wars, I believe the fact that MooTools libraries are not (and should not) be used for new projects anymore (and therefore only need maintenance - of which it doesn't need a lot) is because the libraries are not needed anymore: most of what MooTools wanted to bring to JavaScript has been implemented natively.

In my opinion, the worst thing to do is to settle for a bad name for a JavaScript API, because a JavaScript library decided to use it first.

API should be decided on because of how intuitive it is, which means it should at least be logical, easy to understand and if possible in line with established names in other languages.

Anyway -

I'm sure we can look into updating this API on the MooTools end, but like @cpojer said, it won't really change what happens to old projects.

It should work in such a way - but I'll have to double check if that goes for all versions (do you know off the top of your head @cpojer ?) - that old versions of MooTools Core just overwrite a native method if it exists. For a new version, it should then only place the method (with new API) if it doesn't exist.

This way old projects continue to work fine (since their code will expect MooTools Core's flatten and will get it), new projects (with an updated MooTools Core) will have the new method.

I believe the only difference with MooTools Core's flatten is the depth argument. MooTools Core flattens recursively, while the TC39 proposal only goes up to depth, which defaults to 1. Is this API final?

@cpojer

This comment has been minimized.

Member

cpojer commented Mar 7, 2018

@timwienk it's not final yet, it could still be changed. depth=1 is a sensible default, but in light of this TC39 may want to reconsider setting it to Infinite.

I think the best fix would be to delete Array.prototype.flatten at the top of Array.js in MooTools.

@timwienk

This comment has been minimized.

Member

timwienk commented Mar 7, 2018

I think the best fix would be to delete Array.prototype.flatten at the top of Array.js in MooTools.

I don't think we even have to. Old versions should overwrite (still have to check). For new versions we'd want to use the final API, I guess. (Like we did with all the ES5 methods, years ago.)

@appsforartists

This comment has been minimized.

appsforartists commented Mar 7, 2018

Any gauge of how big the window is (if any) where MooTools would check for flatten and use the built-in if it exists?

@bakkot

This comment has been minimized.

bakkot commented Mar 7, 2018

@timwienk

old versions of MooTools Core just overwrite a native method if it exists. For a new version, it should then only place the method (with new API) if it doesn't exist.

That's actually backwards: new versions overwrite unconditionally, whereas old versions check if the method exists first.

But this also appears not to be the issue. Rather, the issue is probably in how MooTools is setting up methods on Elements.prototype, including up to the present. See here. As a consequence, if MooTools adds any method on Array.prototype, then it will break any MooTools users if TC39 adds a method of the same name even if it behaves identically to how MooTools did it (since we will not add an enumerable method to Array.prototype). The fact that .flatten does not behave identically here was actually a red herring, though that difference would probably also break some sites.

That really sucks. There's probably nothing to be done about it - MooTools is too widely deployed, now - but I think it would still be good to change how MooTools sets up at least Elements.prototype so that this doesn't happen.

@kentaromiura

This comment has been minimized.

Member

kentaromiura commented Mar 12, 2018

@timwienk that is exactly how @arian solved String#contains back in 1.2.6:
7057039
@bakkot it's a known issue that IIRC goes way back before enumerability was a thing in JS; core has a force method to work around that.
If no one else wants to do it I can send a PR for 1.2.7; a compat+1.6.x branch can be done once the behavior is finalized.

@mathiasbynens

This comment has been minimized.

mathiasbynens commented Mar 19, 2018

IIUC, the problem is not the difference in implementation; MooTools overrides Array.prototype.flatten if it’s natively available.

The problem is that, when a non-enumerable Array.prototype.flatten is available (e.g. because a browser ships it natively), then MooTools assigning to it (i.e. Array.prototype.flatten = mooToolsImplementation) doesn’t magically make it enumerable, meaning MooTools won’t copy it over to Element.prototype.flatten. Code relying on Element.prototype.flatten now breaks.

Update: Ah, @bakkot already said this.

@ljharb

This comment has been minimized.

ljharb commented Mar 19, 2018

The better solution for MooTools (if this is still the approach in the current version) might be to hardcode its list of methods (on Array and Elements .prototype) rather than to infer the list from the objects at runtime?

@svieira

This comment has been minimized.

svieira commented Mar 19, 2018

As a slightly-different-form-of-horrible what if setting these methods without a descriptor made them enumerable by default? (E. g. monkey-patching of built-in prototypes as a signal to the JS-engine to revert to ES3 semantics). We probably don't want to do this for all objects, just the built-in ones.

Especially since Mootools isn't checking & using, but assigning to a "known-good" implementation always (horray for doing the sensible thing), that would solve these cases (flatten, contains, etc.).

@Kovensky

This comment has been minimized.

Kovensky commented Mar 20, 2018

That would also mean that polyfills would have to either replace the prototype with a Proxy, or would have to check and replace every single entry in the prototype with a getter/setter pair. 😱

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