r104671 MediaWiki - Code Review archive

jquery.delayedBind explicitly says that the third parameter (data) is optional, so the previous ought to work surely...? What's the reason why the patch is supposed to fix it?

Now after a closer look I understand what's going on: Once (jQuery 1.4.2, [1]) $.bind checked for jQuery.isFunction( data ) to see if the arguments needed to be shifted, and all worked as expected.

But with jQuery 1.6.2 ([2]) this check is now arguments.length === 2 which is false, since there are always 3 arguments, even if the last argument is undefined.

A probably better solution than my original patch would of course be to fix the delayedBind-plugin. I neither can test javascript code nor can I created patches right now, but I think something like

if ( callback === undefined ) {
 $(this).bind( '_delayedBind-' + encEvent + '-' + timeout, data );
} else {
 $(this).bind( '_delayedBind-' + encEvent + '-' + timeout, data, callback );
}

instead of line 42 should work.

Or just write in the documentation that data isn't optional any more ...

Aha, that makes sense!

Adding keyword needs-js-test -- we should confirm that jquery.delayedBind works with that option in the qunit tests.

Ok r105007 undoes this change, fixes jquery.delayedBind's optional data parameter handling, and adds a QUnit test case for delayedBind with and without the data parameter to confirm.

Should get merged to 1.18 since the updated jQuery is present there.