Skip to content
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

MooTool's String.prototype.contains conflicts with ES6 #2402

Closed
benjaminp opened this issue Aug 10, 2012 · 11 comments · Fixed by #2440
Closed

MooTool's String.prototype.contains conflicts with ES6 #2402

benjaminp opened this issue Aug 10, 2012 · 11 comments · Fixed by #2440
Labels
Milestone

Comments

@benjaminp
Copy link

ES6 adds the String.prototype.contains method (specified here; look at the PDF). As you can see, it does not have the same behavior as MooTools' String.prototype.contains. Namely, the second parameter in ES6 is a position to start searching from not a separator. Firefox 17 implements the ES6 version, which is breaking sites that use MooTools.

@arian
Copy link
Member

arian commented Aug 10, 2012

Only MooTools 1.2 breaks. MooTools >= 1.3 overwrites .contains because it's not in this list: https://github.com/mootools/mootools-core/blob/c7a161d85/Source/Core/Core.js#L266-268

That said, we probably should prepare for those new methods and make our API compatible in 1.4.6 and 1.5.

@gutworth what's the current status? As far as I know it's only a proposal, but now it's implemented it probably won't change a lot, will it?
Also, what other new methods can we expect, I saw the string extras, Array.from/Array.of, and some number methods right?

@benjaminp
Copy link
Author

There are more methods on the Number constructor: Number.isFinite(), Number.isInteger(), Number.isNaN(), Number.toInteger().

Technically, everything is subject to change until the spec is finalized. However, the new methods in Firefox are based on the harmony proposals page, which is supposed to mean the committee has reached consensus even if it's not formally speced yet.

@sebmarkbage
Copy link
Member

There is some chance there might be Object.extend and more as well.

@BenoitZugmeyer
Copy link

Note for completeness: Mozilla care about this issue and apparently will do some evangelism in order to prevent websites using MooTools 1.2 to break when Firefox 17 will be released.

See comment https://bugzilla.mozilla.org/show_bug.cgi?id=784280#c10

@arian
Copy link
Member

arian commented Sep 3, 2012

That's great to hear. I do think we should change String.prototype.contains to match the signature with ES6 though in 1.5.

@DavidBruant
Copy link

I've added a section on MDN to describe this issue. Please tell me if I've made a mistake.

I agree it would make sense for MooTools to match ES6 signature. However, the feature isn't exactly the same, namely on the second parameter.
So, one thing that would ease the transition from MooTools 1.2 to 1.5 would be to provide an alias for the current String.prototype.contains (maybe to String.prototype.legacyContains or String.prototype.oldContains). This way, people using the old version will just have to rename their calls and not the logic around it.

@sebmarkbage
Copy link
Member

We did end up changing method signature for bind and had a tutorial for upgrading: https://github.com/mootools/mootools-core/wiki/Update-from-1.2-to-1.3
We already had a suitable alternative.

There was some projects that couldn't update to 1.3 for a long time, for various reasons (most notably Joomla). So there was also a minor revision update 1.2.5. For whatever reason that version only fixed bind specifically. Not the real issue. It wasn't until 1.3 all native methods were overridden. Maybe there needs to be a 1.2.6 release to allow for a quick fix.

@arian
Copy link
Member

arian commented Dec 10, 2012

Needs a fix for MooTools 1.2.6, as it probably breaks some selectors ($$) too: https://groups.google.com/forum/#!topic/mootools-users/W7MHwTFHYQ4

@Enyby
Copy link

Enyby commented Jan 8, 2013

Quick fix: You need add "String.prototype.contains = undefined;" before init mootools core. Or add this row as first row in your core file.
Note: You need check other js code for using String.prototype.contains in default way. But this function implement only in firefox, than its usage unlikely.

arian added a commit to arian/mootools-core that referenced this issue Jan 15, 2013
… the MooTools version.

Firefox 18 implements .contains, and ES6 might do that as well. That breaks
some stuff, so we have to make sure it's overwritten with the 'correct'
MooTools version.
arian added a commit to arian/mootools-core that referenced this issue Jan 15, 2013
…ols version.

Firefox 18 implements .contains, and ES6 might do that as well. That breaks
some stuff, so we have to make sure it's overwritten with the 'correct'
MooTools version.
@hnuecke
Copy link

hnuecke commented Feb 3, 2013

Danke! Die Zeile in mootools core einzufügen war auch für mich (2.9.5) die Lösung

@arian
Copy link
Member

arian commented Feb 19, 2013

Okay, this problem is solved for 1.2 now too, and released, so we can close this! http://mootools.net/blog/2013/02/19/mootools-1-2-6-released/

@arian arian closed this as completed Feb 19, 2013
ibolmo added a commit to ibolmo/mootools-core that referenced this issue Feb 16, 2014
…cording to the new ES6 standard

- Moved the old String.prototype.contains implementation in 1.4compat
- Using good old indexOf at other places
pull bot pushed a commit to nsdevaraj/mediacore-community that referenced this issue Dec 11, 2018
…dev, git revision a3be7a0ef5205eb655c4ee11e8cbedcb3f2ffa95).

The update makes the MediaCore admin functional again in Firefox 18. Before we had broken squeezeboxes (e.g. "Add & Manage Files") as well as several layout issues.

MooTools caused the bug because they modify String.prototype in a way that clashes with EcmaScript 6 and Firefox 18 was obviously the first browser to implement that part of ES6.

More information can be found in the MooTools issue 2402 ("MooTool's String.prototype.contains conflicts with ES6"):
  mootools/mootools-core#2402
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants