Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

rename flatten to smoosh #56

Closed
wants to merge 1 commit into from
Closed

rename flatten to smoosh #56

wants to merge 1 commit into from

Conversation

michaelficarra
Copy link
Member

As reported in Bugzilla bug 1443630, 8+year old versions of MooTools conditionally define an incompatible version of Array.prototype.flatten. See mootools/mootools-core/Source/Core/Core.js for the responsible code. In an attempt to turn a negative situation into a positive one, I am taking this opportunity to rename flatten to smoosh.

bunnies

@jdalton
Copy link
Member

jdalton commented Mar 7, 2018

What percentage of sites does this affect?

@TrySound
Copy link

TrySound commented Mar 7, 2018

What is this word?

@jdalton
Copy link
Member

jdalton commented Mar 7, 2018

@TrySound
Copy link

TrySound commented Mar 7, 2018

How about squash?

@js-choi
Copy link

js-choi commented Mar 7, 2018

See also IRC #tc39 from today.

@domenic
Copy link
Member

domenic commented Mar 7, 2018

I think we should rename flatMap to smooshMap in this case. (Not kidding.)

@jdalton
Copy link
Member

jdalton commented Mar 7, 2018

@mrm8488
Copy link

mrm8488 commented Mar 7, 2018

We should rename object to dictionary too...

@sindresorhus
Copy link

Swift recently renamed flatMap to compactMap, for clarity. How about Array#compact and Array#compactMap?

@tc39 tc39 deleted a comment from gkatsanos Mar 7, 2018
@jdalton
Copy link
Member

jdalton commented Mar 7, 2018

I know compact as array.filter(Boolean)

@zloirock
Copy link

zloirock commented Mar 7, 2018

@zloirock
Copy link

zloirock commented Mar 7, 2018

Array#flatten already added to core-js -> babel-polyfill, I hadn't any issues about it, so that means at least that it does not break any sites where they used together with MooTools.

@zloirock
Copy link

zloirock commented Mar 7, 2018

(Sure, MooTools used mainly in ancient sites, but I had many issues about compatibility with also ancient PrototypeJS.)

@brunolemos
Copy link

brunolemos commented Mar 7, 2018

Please let's not add lame non-optimal method names to a language specification (which is basically permanent) to avoid some libs and old websites from having to upgrade themselves.

We can use a gitbub bot to search and create an issue on all affected repositories, for example.

Flatten is the intuitive name for the feature? So that's what should be used.

@realityking
Copy link

What’s the incompatibility between the two implementations? Is it just the default value for the depth parameter? If so, how about aligning with Mootools?

@callmevlad
Copy link

I've opened up an issue with Mootools: mootools/mootools-core#2797 - however, they have not had a release in more than 2 years (which should hint at it's usage/adoption level), so I'm not too optimistic that they might change their implementation.

@Haroenv
Copy link

Haroenv commented Mar 7, 2018

What about importing native prototypes? As in import Array.prototype.flatten from “web”

@nilssolanki
Copy link

nilssolanki commented Mar 7, 2018

I'm also in favor of squash, as smoosh isn't intuitive for many non-native English speakers.

@ljharb
Copy link
Member

ljharb commented Mar 7, 2018

@callmevlad unless you could get all sites using older mootools to upgrade, it wouldn't make a difference.

@Haroenv that would conflict with https://npmjs.com/web

@Haroenv
Copy link

Haroenv commented Mar 7, 2018

@ljharb, I meant the idea, syntax could be different to avoid conflict. For example add prototype { flatten } to Array, which can only be the native one

@nafg
Copy link

nafg commented Mar 7, 2018

Other name for flatMap are chain, join, bind

@zloirock
Copy link

zloirock commented Mar 7, 2018

@Haroenv for this case, we already have bind syntax:

import flatten from 'Array';

array::flatten();

but it's not very convenient. Real prototype method without any imports / extensions much better.

@Haroenv
Copy link

Haroenv commented Mar 7, 2018

I get your point, but it would avoid the conflict. I would assume that build tooling can be set up to add the binding of prototypes by default to the native ones, which would avoid the hassle.

Seeing as how many times we see conflicts, having a way to definitely avoid them, rather than trying to find the least breaking name would be interesting IMO

@gkatsanos
Copy link

gkatsanos commented Mar 7, 2018

// edited from author.
core of message : Mootools should not influence the new spec https://libscore.com/#MooTools

@ljharb
Copy link
Member

ljharb commented Mar 7, 2018

@gkatsanos this is the second comment you've made on this thread that's inappropriate. I'll point you to our code of conduct - if you continue to be hostile, further moderation steps will be taken.

To answer your question, browsers care about Mootools (and users of sites that use Mootools), and thus so too must everyone.

@jsg2021
Copy link

jsg2021 commented Mar 7, 2018

Seriously? prototype polution has been frowned on for years with warnings that if you do, your site may break... Are we really backing down from that warning and letting these old sites dictate our future?

What’s the point of warning and cautioning developers if don’t honor our warnings?

@gkatsanos
Copy link

gkatsanos commented Mar 7, 2018

@ljharb My wording was inappropriate but judging from other comments, and on twitter, the core of my message is the popular opinion.

I urge anyone who is against not using "flatten" in the official spec in order to accomodate an anti-pattern of "Mootools" to upvote this comment.

Was this thread opened in order to see what the popular opinion is? In that case the native JavaScript language should not change it's upcoming new specs in order to accomodate obsolete libraries. I don't even understand why we're discussing it.

Aren't there IE6,7,8,9 quirks that the new JS spec doesn't support and the opposite? Definitely so. So why all this talk about a library not in use since years? Is there interest because one of its' creators is involved in TC39 or something?

@zonzujiro
Copy link

zonzujiro commented Mar 7, 2018

@aluanhaddad sorry, but I don't understand what you mean :) I don't want compile to another version of JS. Even if website will crash - it's ok. Fine. It's uses old library. If authors of library extending prototype of standard object without checking existing value - why this should be a problem?

In general, problem is in conflicts of names between old library and new standard. There is a lot of libraries. With a lot of names.

@miketaylr
Copy link

@gkatsanos if you wanted to do all JS developers a solid, you are more than welcome to try to contact every website that this proposal breaks, and get them to update.

I've been in the business of attempting that working for 2 browsers over the past 8 years. Don't hold your breath. :/

@jcimoch
Copy link

jcimoch commented Mar 7, 2018

Please decline this PR :(

@rimunroe
Copy link

rimunroe commented Mar 7, 2018

(Apologies if I should take this elsewhere)

@zonzujiro Website made on 8 year old technologies will stop working? Ok. Fine. Goodbye.

This is concerning and incompatible with what I feel is the fundamental idea of the web, which is to provide a network of information. You don't expect to go to a library and be unable to read a book because it was published ten years ago. Why should we punish users just to make our APIs a bit more pleasant?

And, surprise, anyway you can use old browser for this site.

Telling a user to use an older browser to view the site requires the following to be true, each of which is more difficult than the last:

  1. You can tell them about the problem.
  2. You can explain which old browser to use. (I'd have a hard time explaining this to some of my friends.)
  3. They can find the old browser. (Assumes the user already has a knack for tech, and assumes that the browser is actually available to them.)
  4. They can install & run the old browser. (At this point we're assuming they can also run a compatible OS on compatible hardware. It assumes that the users of websites have really deep computer knowledge.)

Your user was just trying to do a report on the 2008 coup in Guinea, and now they have to know how to get IE 6 to run on their 2032 hardware. This isn't an acceptable burden to push on anyone.

@Jerczu
Copy link

Jerczu commented Mar 7, 2018

@rimunroe as a developer it's your job to stay on top of changes you can't expect language to accommodate everyone. Otherwise we'll be stuck and not moving forward it's a terrible idea. As a professional you should take responsibility in code you ship if breaking change occurs it's up to you to fix it not language to accommodate your poor choices.

@Jerczu
Copy link

Jerczu commented Mar 7, 2018

You should be logging issue with all those library github accounts not here.

@miketaylr
Copy link

miketaylr commented Mar 7, 2018

@rimunroe as a developer it's your job to stay on top of changes you can't expect language to accommodate everyone. Otherwise we'll be stuck and not moving forward it's a terrible idea. As a professional you should take responsibility in code you ship if breaking change occurs it's up to you to fix it not language to accommodate your poor choices.

This is a pretty simplistic view on business realities. Do you have access to sites you maintained 5 years ago, 2 jobs back? Will your manager even let you take the time to work on a site where there's no client paying for your hours? There are so many more constraints than "take responsibility in code you ship."

@rimunroe
Copy link

rimunroe commented Mar 7, 2018

@Jerczu @rimunroe as a developer it's your job to stay on top of changes you can't expect language to accommodate everyone.

Not everyone is a developer. Sometimes you're just a person who wanted to write your diary, which just happened to be hosted online. Why should we force those people to maintain their work forever?

Alternatively, I feel like the more obvious response might be, "People aren't immortal." Sometimes humans die after writing something.

@lukejagodzinski
Copy link

lukejagodzinski commented Mar 7, 2018

I wonder which version of MooTools are we talking about. I've made a simple test where I'm trying to override includes and I didn't have any problem with that https://jsbin.com/sevubugowi/edit?html,js,console,output. So it shouldn't be any problem with the flatten too. If I'm wrong, then please explain to me why it would use native version instead of overridden one?

@JonAbrams
Copy link

A couple ideas:

  1. Have Array.prototype.flatten return falsey. (I think document.all does the same?) Provide an alternative Array.flattenSupported boolean for feature detecting.
  2. Would it be too much for browsers to detect MooTools and disable flatten if needed?

@zonzujiro
Copy link

zonzujiro commented Mar 7, 2018

@rimunroe when you take such decision, you must operate with concrete information, not with idea of ideal web itself. As I said - there is a lot of libraries, with a lot of used names.

Basically, you asking to prevent making changes in alphabet, because < 1% of readers will not understand your book. This is false point of view. Saying, that web is about information is one thing. Creating artificial obstacles - is another. Publisher must update his own book, not stopping to change language itself.

Also, I want to agree with @Jerczu - it's our, developers, job, to avoid such problems. This is what we are payed for.

@zonzujiro
Copy link

zonzujiro commented Mar 7, 2018

Also, about concrete information - I see not the first time in this discussion, that Array#flatten not brakes MooTools implementation.

#56 (comment)
#56 (comment)
#56 (comment)

And over, and over, and over...

@miketaylr
Copy link

@zonzujiro These commenters don't understand the bug. If you implement a native method and MooTools defers to it, but the site expected a different method signature or different semantics... 💣 💥.

@anba
Copy link

anba commented Mar 7, 2018

@michaelficarra @littledan @ljharb
For whatever reason, the site depends on "flatten" being enumerable. (It does override the native Array.prototype.flatten, but that happens through a normal property assignment, which obviously won't change the [[Enumerable]] flag.)

I've verified this by changing this line to use JSPROP_ENUMERATE and then rebuild Firefox.

@ljharb
Copy link
Member

ljharb commented Mar 7, 2018

@anba to clarify; are you saying that if flatten is enumerable, sites using all versions of Mootools will unconditionally override Array.prototype.flatten?

@anba
Copy link

anba commented Mar 7, 2018

@ljharb No. At least for this specific case at wetteronline.de, Array.prototype.flatten is always overridden with Mootools' version of flatten, but some part of this site (or of Mootools, I haven't investigated further), relies on Array.prototype.flatten being enumerable.

@littledan
Copy link
Member

We seem to be hearing a bit of skepticism here about whether web compatibility should be a goal in the evolution of JavaScript. Overall, maintaining compatibility is very important for TC39, and I don't think we'll be able to overcome that as a design constraint in this thread.

@tc39 tc39 locked as too heated and limited conversation to collaborators Mar 7, 2018
@bterlson
Copy link
Member

bterlson commented Mar 7, 2018

I think a lot of people are assuming that this process is done. It's not. We've yet to fully understand the scope of the breakage and exactly what patterns are broken and how we might fix them. I hope we'll learn more over the coming days.

Also what @littledan said is important: every change is, pedantically, a breaking change. Flatten is concerning because current signals and past experience with exactly this issue (Array#contains) suggests the problem will run too deep to fix easily. As we learn more this may change.

Everyone can rest easy knowing that TC39, being filled with developers, wants this great new functionality. We worked really hard on this proposal for many months! But developers are not our only "customers" and we owe it to all internet users to evolve their platform responsibly.

@bakkot
Copy link
Contributor

bakkot commented Mar 7, 2018

Thanks, @anba.

The original webcompat thread points to a very similar issue with contains and MooTools, which strongly implies that this is not about the difference in behavior of the method itself (though I imagine the difference in behavior would also cause breakage).

The specific problem with contains, and I strongly suspect here, is as follows:

  • MooTools is unconditionally setting up Array.prototype.{flatten, contains, etc} by regular property assignment, which creates an enumerable property if one does not exist but does not cause non-enumerable properties to become enumerable.
  • It latter copies all enumerable methods on Array.prototype to Elements.prototype. (which is a MooTools specific extension used as e.g. the return type of their global $$ helper).
  • As such, if we spec a non-enumerable method on Array.prototype which shares a name with something MooTools added, then MooTools will fail to copy that over to Elements.prototype. Sites depending on it being present will break.

That's unfortunate.

I don't think there's a fix short of renaming a la contains -> includes or weird hacks like a non-enumerable setter which, on invocation, overrides itself with an enumerable data property.

@rwaldron
Copy link

rwaldron commented Mar 7, 2018

@michaelficarra

I am taking this opportunity to rename flatten to smoosh.

@domenic

I think we should rename flatMap to smooshMap in this case. (Not kidding.)

+1 to both.

I'm choosing to look at this as a "fun" way to resolve an unfortunate problem.

@michaelficarra
Copy link
Member Author

I can live with smooshMap if we think the internal consistency with smoosh outweighs the external consistency with the many other languages that call this operation flatMap.

why not both?

@tabatkins
Copy link

Strongly against smooshMap when the other popular term for this - chain - is just sitting there.

@jridgewell
Copy link
Member

jridgewell commented Mar 8, 2018

Playing around with it a bit, we can actually "fix" this be defining an overridable setter:

Object.defineProperty(Array.prototype, 'flatten', {
  configurable: true,
  get() {
    return flatten;
  },
  set(v) {
    // A overridable setter, this acts like a normal property after first set.
    Object.defineProperty(Array.prototype, 'flatten', {
      configurable: true,
      enumerable: true,
      writable: true,
      value: v,
    });
  }
});

/cc @domenic, who mentioned something similar in the last committee meeting with Array.prototype.last.


Edit: Agh, @bakkot already mentioned this.

@chancancode
Copy link

If we are taking suggestions for alternative names to consider, I would chip in with unwrap

@bterlson
Copy link
Member

bterlson commented Mar 8, 2018

Since this thread is linked from a few external places and getting thousands of views, I want to make a process note:

This proposal to add flatten and flatMap to ECMAScript is at stage 3 of the TC39 Process. This is when implementers begin gathering real world implementation experience and uncovering these sorts of issues. This proposal will not be part of ES2018 and will be released in the next edition after it reaches stage 4. This issue will have to be resolved before the proposal can advance.

The name smoosh in particular is one of many options. If flatten needs to be renamed, a new name will be discussed here and during at least one plenary meeting. TC39's 40+ members will then need to reach consensus on a new name. Keep in mind that TC39's active GitHub users are not always representative of the entire committee.

@bakkot
Copy link
Contributor

bakkot commented Mar 9, 2018

As a followup to my comment above, I said

Sites depending on it being present will break.

It turns out that non-idiosyncratic usage can easily so depend. For example,

$("content-navigation").getElement("ul").getChildren("li").getChildren("ul").flatten()

is using .flatten on Elements.prototype to accomplish the equivalent of

document.querySelectorAll('#content-navigation > ul:first-of-type > li > ul').

This is precisely what the website this change broke was doing. So I expect this to break many more sites, as spec'd.

@michaelficarra
Copy link
Member Author

Resolved by 093eacc.

@ljharb ljharb deleted the smoosh branch May 22, 2018 19:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet