903755 - Additional builtin objects' methods break SugarJS

Status: RESOLVED FIXED

(Web Compatibility :: Site Reports, defect)

Description

[Till Schneidereit [:till]]

SugarJS adds serveral functions to Array and other builtins' prototypes. Some of them conflict with ones added in ES6.

At least, the following functions are affected:
- Array#find
- Array#findIndex
- Array#repeat
- String#repeat

Not only might there be more already, it's also somewhat likely that additions in later editions of the standard will conflict.

IMO, we should try to convince the SugarJS maintainers to abandon the approach of adding functions with possibly-conflicting names to any builtins' prototypes.

They could either focus on polyfilling harmonious builtin additions per spec, or, potentially in addition, add functions with names that are extremely unlikely to collide. Something like `sugarEach` or at least `sEach` would work.

Comment 1

[Kohei Yoshino]

(In reply to Till Schneidereit [:till] from comment #0)
> At least, the following functions are affected:
> - Array#find
> - Array#findIndex
> - Array#repeat
> - String#repeat

Correction:

- Array#find
- Array#findIndex
- Array#from
- String#repeat

Comment 2

[Kohei Yoshino]

Tweeted: https://twitter.com/koyoskeh_pub/status/366226507485544448

Comment 3

[Kohei Yoshino]

V8 has also recently implemented the same ES6 methods, so this library might be broken on Chromium too.

http://code.google.com/p/v8/issues/detail?id=2776 Array.find
http://code.google.com/p/v8/issues/detail?id=2777 Array.findIndex
http://code.google.com/p/v8/issues/detail?id=2799 String.repeat

Comment 4

[Kohei Yoshino]

http://sugarjs.com/api/String/repeat won't be a problem, as it works the same as the native function.

Comment 5

[Andrew Plummer]

SugarJS creator here. First some clarifications:

1. Array.from does not exist on the prototype in the ES6 spec unlike the others, so it's not an issue here. 

2. Of the remaining 3, Sugar's implementation is for the most part the same but with some notable exceptions:

  - Array#find and Array#findIndex do not accept a context object (instead, their second and third arguments are "startIndex" and "loop" arguments, respectively).
  - String#repeat has some edge cases that aren't covered (passing a negative number, passing +Infinity, etc.)

3. String#startsWith and String#endsWith also collide but these were retrofitted to match the es6 spec (and polyfill when not available) a while back.

4. String#normalize also collides but it is not part of the main Sugar download package and I don't think very many people are using it.


Now to the main part. I have already taken some flak for this on Twitter, and I think it's with very, very good reason. First, I want to make it clear that these collisions aren't taken lightly, and they aren't some kind of attempt to go against or second guess the spec. Sugar made a decision a long time ago not only to abide the spec but also facilitate it (in other words to the greatest extent possible try to promote the use of Javascript native methods and paradigms, not try to circumvent it with similarly named methods that do totally different things). So with that in mind a couple points:

1. Sugar needs to change. There is no other way. In fact, part of the alignment to upcoming spec was already in progress already. It will be up to the community to decide *how* it changes, but the fact remains that it has to.

2. Regarding this bug it is my opinion that it is invalid and should be closed. It isn't the responsibility of those implementing spec to be concerned with environments that go against it, no matter how small, no matter intentional or not. Dealing with the repercussions of how new spec will affect Sugar users should be the responsibility of the Sugar community alone. There has always been a kind of implied pact between those who choose to use Sugar that they accept a certain amount of risk. For one, it has always been intended for end-user use and not, for example, plugin developers who might introduce unexpected consequences down the chain to *their* end users. Change of spec is another risk, and there are others too. As the state of JS dev has improved many of these risks have been mitigated, and it is Sugar's continued stance that with a certain level of caution and common sense applied, the benefits of extending prototypes can far outweigh the risks. However, they still exist, and Sugar has attempted to make this clear from the start. This ticket is in the end one such instance, and really should be closed.

Comment 6

[David Bruant]

(In reply to plummer.andrew from comment #5)
> SugarJS creator here. 
> Now to the main part. I have already taken some flak for this on Twitter,
> and I think it's with very, very good reason.
I have given a good share of that and mean to say that there is nothing personal. I've been tweeting mostly to educate other devs on what to avoid doing.
A bunch of people (like myself) keep repeating over and over "don't override built-ins, that threatens the future evolution of the platform". But that's all theoretical, so few really understand and even fewer care. An example of what happens when one doesn't follow the advice is an occasion to re-explain the advice in a more concrete way.
Anyway, no bad feelings on my side and importantly nothing personal.


> First, I want to make it clear that these collisions aren't taken lightly

> 1. Sugar needs to change. There is no other way. In fact, part of the
> alignment to upcoming spec was already in progress already. It will be up to
> the community to decide *how* it changes, but the fact remains that it has
> to.
> 
> 2. Regarding this bug it is my opinion that it is invalid and should be
> closed. It isn't the responsibility of those implementing spec to be
> concerned with environments that go against it, no matter how small, no
> matter intentional or not. Dealing with the repercussions of how new spec
> will affect Sugar users should be the responsibility of the Sugar community
> alone.
I wish I could agree, and I am thankful of your sense of responsibility (it's really not all library maintainers who have this sense of responsibility towards the web ecosystem), but that's not how the web works unfortunately.

To make it short (I'll probably write a longer more detailed explanation as a blog post soon), web browsers have an incentive to never break a website. If one browser breaks a website, users will just change of browser and that means decrease in the "guilty" web browser market share.
No browser wants to take the risk of decreasing its user base/market share.

So, if some websites uses sugarJS and introducing native Array.prototype.find breaks them, web browsers will try to see if the websites can be fixed (this is what tech evangelism bugs are about at Mozilla at least). If there are too many broken websites or if some websites are unmaintained, web browsers will just choose to not introduce Array.prototype.find, regardless of how much library users feel responsible for their mistakes.

Browsers (and consequently standards which follow browsers and not the other way around) make decisions based on number and importance of broken websites. That's just how it is regardless of whether we like it or not (I've been aware of that for quite some time and still hate it, but accept it :-) )


Despite your sense of responsibility, this bug will have to remain open until everyone has an understanding of how many websites are at stake and the introduction of new built-ins would break. I guess this bug will be closed if either there is sufficient proof that few enough websites are affected or too many websites are affected.

Fixing SugarJS, the library, is a good thing for the long term and future websites, but doesn't help with (worldwide!) deployed copies.

One way you can help to improve the short term situation:
1) write patches for every single released version of SugarJS that people might be using in the wild (I'm aware it can be a lot of work and I suggest this idea only out of necessity). The idea is to create drop-in replacements for what people are using and that don't conflict with standard methods. Idea: https://bugzilla.mozilla.org/show_bug.cgi?id=897784#c2
2) (Assuming you have a communication channel to find all (or at least most) people using SugarJS for their websites) : tell people to upgrade to the patched version.
3) Keep people posted, at least through this bug.

Thanks!

Comment 7

[Andrew Plummer]

> To make it short (I'll probably write a longer more detailed explanation as
> a blog post soon), web browsers have an incentive to never break a website.
> If one browser breaks a website, users will just change of browser and that
> means decrease in the "guilty" web browser market share.
> No browser wants to take the risk of decreasing its user base/market share.

Right, I see your point. I guess that's just the way things work. On the bright side, I think that the impact for this particular ticket should be low. For one, Sugar's traction has been good, but not outstanding, probably for obvious reasons. Additionally, the number and nature of affected methods here is quite small. In fact, even in applications using them, provided calls to find and findIndex have only one argument, the behavior should be identical (don't have the data but I doubt that the second two arguments are commonly used -- also even if passing "startIndex", which would result in it being turned into the function context in a native implementation, still would not likely cause any hard-errors. In most scenarios it would simply result in looping over a few extra elements). String#repeat should be nearly identical.


> One way you can help to improve the short term situation...write patches...upgrading..

Ok going to have a look into this right away, and will try to think about other ways to reach people as well.

Comment 8

[Till Schneidereit [:till]]

Andrew, thanks a lot for the stance you're taking here - very much appreciated!

We'll probably have to be a bit patient about this, and wait for other browsers to also implement these methods to share the pressure, there's two things you can do that should help quite a bit:

- quickly release a new version of SugarJS that changes the problematic methods
- give your users clear instructions for updating and urge them to do so ASAP

While it's likely that not all of your users are going to receive the message right away, you're in a much better position for reaching them then we are.

Comment 9

[Andrew Plummer]

New version is being worked on right now and will land very soon. It introduces many changes though so will also do a quick patch for older versions.

One they're gtg I'll try to get the word out in whatever way I can!

Comment 10

[Andrew Plummer]

Ok the new version is out! The latest release:

1. Completely aligns with the ES6 spec. To be clear, that doesn't mean it polyfills all ES6 methods, but the ones that were in conflict with Sugar are now following the spec so they are effectively serving as polyfills (again, they were *mostly* following the spec before, but not 100%).

2. Effectively clobbers any methods it defines that are *not* flagged as polyfills. A tough decision to be sure (updated the main site to explain the rationale behind it), but this will effectively prevent any issues arising from changes in spec going forward, and should prevent any more tickets like these. To be 100% clear though, the goal for Sugar is staying well ahead of actual implementations of spec changes in any case.

Additionally I released a patch available here http://bit.ly/19FrJKx for currently existing legacy versions for which this ticket might be a problem. This patch essentially does the same thing as above, but only for the specific colliding methods and only if they already exist.

Have already announced these changes through my main channels (blog, Twitter, site), but will also have a look at reaching out directly to larger sites for which this might be an issue.

Comment 11

[Till Schneidereit [:till]]

Andrew, thanks again for actively working on fixing this in SugarJS!

Let's hope people quickly adopt the new version, then. :)