Tony, are you planning to take on this bug?

(In reply to James Forrester from comment #1)

Tony, are you planning to take on this bug?

I plan to. But is there anything like bind() is only present in modern browsers ?
will replacing, say
this.$element.on( 'keypress', $.proxy( this.keyup, this ) )
with
this.$element.on( 'keypress', this.keyup.bind(this)) will do the trick ?

We don't use $.proxy() directly in the VE code base, instead we set ve.bind = $.proxy; and use ve.bind().

Yes, this.keyup.bind() exists in modern browsers. It doesn't exist in older browsers, but we don't really care because VE doesn't support browsers that old for lots of other reasons anyway.

There are two ways we could approach this. I'll ask Krinkle which one he thinks is best before we proceed.

#1 is to migrate uses of ve.bind( this.foo, this ); to this.foo.bind( this ); across the code base, then deprecate and later remove ve.bind(). This would be pretty laborious.

#2 is to change ve.bind() to a wrapper that uses native .bind(). This will change every single function binding in the entire codebase to use native bind instantaneously.

My guess it's probably best to do #2 first, then #1 later, but I'd like Krinkle to weigh in on this.

(In reply to James Forrester from comment #1)

Tony, are you planning to take on this bug?

I plan to. But is there anything like bind() is only present in modern
browsers ?

Indeed, Function.prototype.bind is new in ECMAScript 5, which VisualEditor requires. Or rather, VE requires ECMAScript 3 syntax engine and an environment that somehow provides certain ES5 features, which can potentially be polyfilled. We don't yet (and that is by design), adopt any ES5 syntax or ES5 features that can't be polyfilled.

This is already guarded against using the feature test in our mw integration init[1], so you can rely on this without having to worry about old browsers.

(In reply to Roan Kattouw from comment #3)

I'd recommend #2 for consistency (we do the same with other ES5 features we've adopted) and minor performance micro optimisation (extra global and property, extra callstack because it has to be a proxy calling bind, unlike Array.isArray is can't be assigned to ve.bind by reference because bind needs to be bound itself) and because we're already using '.bind' in a few places.

But keeping ve.bind with a small wrapper to bind using bind seems fine for backwards compatibility with ve plugins in the wild. But I'd recommend we don't use this in core.

[1] https://github.com/wikimedia/mediawiki-extensions-VisualEditor/blob/7e52a1ab2ab514/modules/ve-mw/init/targets/ve.init.mw.ViewPageTarget.init.js#L74-L93

Ping on this bug again – do you need any support/advice/help?

Sorry, I got bit busy with my other project.

We now have the polyfill for Function.prototype.bind provided by MediaWiki so this should be easier to do now. Simply replace all uses of $.proxy(fn..) and ve.bind(fn) in the modules/ directory with calls to fn.bind(..).

Change 144840 had a related patch set uploaded by Alex Monk:
Replace ve.bind( fn, ... ) calls with fn.bind( ... )

https://gerrit.wikimedia.org/r/144840

If we're going to replace ve.bind it should be done in ve core first so we'll need a polyfill there. In ve-mw we can omit that polyfill as mw loads it's own.

Why? How is this different from the other ES5 methods we've been using already? VisualEditor nor OOjs UI support ES3 engines afaik.

MediaWiki itself does, and OOjs core does (for other consumers of OOjs). But we don't (we user .super which breaks IE's ES3 parser, Array.isArray, Object.keys etc.)

mw-ve, ve-core, oojs-ui, and oojs all share a requirement for ES5 environments beyond just the polyfillable methods.

(In reply to Krinkle from comment #10)

Why? How is this different from the other ES5 methods we've been using
already? VisualEditor nor OOjs UI support ES3 engines afaik.

MediaWiki itself does, and OOjs core does (for other consumers of OOjs). But
we don't (we user .super which breaks IE's ES3 parser, Array.isArray,
Object.keys etc.)

mw-ve, ve-core, oojs-ui, and oojs all share a requirement for ES5
environments beyond just the polyfillable methods.

oojs-core supports ES3 engines but requires an ES5 shim. The same would apply to OOjs UI and/or VisualEditor core if and when we decide to support older browsers.

Change 144840 merged by jenkins-bot:
Replace ve.bind( fn, ... ) calls with fn.bind( ... )

https://gerrit.wikimedia.org/r/144840

(In reply to Krinkle from comment #7)

We now have the polyfill for Function.prototype.bind provided by MediaWiki
so this should be easier to do now.

Heh.

Anyway, is there anything else to do here?

(In reply to Alex Monk from comment #13)

(In reply to Krinkle from comment #7)

We now have the polyfill for Function.prototype.bind provided by MediaWiki
so this should be easier to do now.

Heh.

Anyway, is there anything else to do here?

VisualEditor core, and OOjs UI.

(In reply to Krinkle from comment #14)

VisualEditor core, and OOjs UI.

Me and Krinkle spoke about this and think it would be a better idea to defer this for now. PhantomJS is not implementing Function#bind (like we would expect) and that's necessary for the tests to succeed. In other repos we get around this by loading es5-shim but in these ones we should be able to rely on ES5 properly.

(In reply to Alex Monk from comment #15)

(In reply to Krinkle from comment #14)

VisualEditor core, and OOjs UI.

Me and Krinkle spoke about this and think it would be a better idea to defer
this for now. PhantomJS is not implementing Function#bind (like we would
expect) and that's necessary for the tests to succeed. In other repos we get
around this by loading es5-shim but in these ones we should be able to rely
on ES5 properly.

With I64a0bd96b in VE-core and If2da01a25 in OOUI this is no longer a blocker.

Change 166155 had a related patch set uploaded by Alex Monk:
Replace calls to OO.ui.bind( fn, ... ) with fn.bind( ... )

https://gerrit.wikimedia.org/r/166155

Change 166155 merged by jenkins-bot:
Replace calls to OO.ui.bind( fn, ... ) with fn.bind( ... )

https://gerrit.wikimedia.org/r/166155

Change 167055 had a related patch set uploaded by Alex Monk:
Replace calls to ve.bind( fn, ... ) with fn.bind( ... )

https://gerrit.wikimedia.org/r/167055

Change 167055 merged by jenkins-bot:
Replace calls to ve.bind( fn, ... ) with fn.bind( ... )

https://gerrit.wikimedia.org/r/167055

Removal of this function is punted to bug 72156.