Page MenuHomePhabricator

Fix "JQMIGRATE: Use of jQuery.fn.data('events') is deprecated" in TimedMediaHandler
Closed, ResolvedPublic

Description

Can't quite understand what it's triggered by – stack trace suggests it's coming from runPlayerSwap() on load of e.g. http://en.wikipedia.beta.wmflabs.org/wiki/User:Jdforrester_%28WMF%29/Sandbox


Version: master
Severity: normal

Details

Reference
bz65428

Event Timeline

bzimport raised the priority of this task from to Needs Triage.Nov 22 2014, 3:18 AM
bzimport set Reference to bz65428.

MwEmbedSupport/MwEmbedModules/MwEmbedSupport/jquery/jquery.triggerQueueCallback.js
On the first glance it looks like someone was trying to reinvent promises.

mdale wrote:

jquery.triggerQueueCallback.js indeed pre-dates promises. It's intent is to trigger all events bidded to a given event name and once all respective events issue the supplied callback, continue code execution.

Is it possible to implement an event bind callback list with promises? .. i.e imagine you want to let plugins supply player sources, and multiple such plugins can add to your available sources.

Within your get sources you have something like:

$(this.embedPlayer).triggerQueueCallback('getSources', function(){

// This callback is called after all plugins issue callback for getSources
_this.handleSources();

});

Within plugins that supply sources you presently have:

$(this).bind('getSources', function(event, callback){

 _this.asyncCallToGetSources(function( sources ){
     _this.embedPlayer.addSources( sources );
     callback();
});

});

Within triggerQueueCallback, we have the stack of binded getSources, and await each callback before issuing the master callback.

Most promise examples I have seen seem to pass a single event callback, is there a clean way that each plugin to not have to know about the rest of the promise chain?

To maintain a list of functions and fire them in some manner, you can use $.Callbacks. This jQuery is also used internally by jQuery Event, Promise, Deferred and Ajax.

http://api.jquery.com/jQuery.Callbacks/

var list = $.Callbacks( /* optional behaviour modifier flags */ );

list.add( fn );

list.fire( /* optional */ data );

mdale wrote:

right but you still need to wait for all to resolve before continuing. you just described adding a stack of functions to be executed.

seems $.Deferreds may work ( at the end of that jQuery.Callbacks doc )

Hard to tell how exactly it should be done without knowing how you track when all plugins are loaded, but probably something along the line of all plugins returning a promise which is used to create a master promise saying "all plugins loaded".

E.g.

var pluginLoadingPromises = [];

for ( plugin in this.plugins ) {

pluginLoadingPromises.push( plugin.getLoadingPromise() );

}

$.when.apply( $.when, pluginLoadingPromises ).then( allPluginsLoadedCallback );

mdale wrote:

there is no master for-loop other then the triggered event ... i.e the core get sources implementation should not have to know what plugins are choosing to add at event trigger time.

But $.when looks helpful. i.e: we trigger each plugin adds to the array its promise, then once all promises are resolved we continue execution now that all plugins have had an async opportunity to add their sources.

If you have some sort of plugin registry (i.e. you know the full list of plugins, even if you don't know which plugins will act on this event), you can do a for loop over it, and plugins which pass can just return a resolved promise ( $.Deferred().resolve() ).

But there are many possible ways to do this, depending on various details (e.g. what do you want to happen if some plugins fail to load but others succeed). Generally you want every plugin to return a promise which will resolve when it finishes loading, and want to have some helper function/class which collects all those promises and returns a master promise. If you can get all the plugin promises at the start and are OK with failing the whole process of any of the plugins fail, then $.when is a convenient way of doing that; otherwise you will have to create a deferred, and manually handle plugin loading status and resolve the deferred when all is loaded.

(In reply to Tisza Gergő from comment #5)

Hard to tell how exactly it should be done without knowing how you track
when all plugins are loaded, but probably something along the line of all
plugins returning a promise which is used to create a master promise saying
"all plugins loaded".

E.g.

var pluginLoadingPromises = [];

for ( plugin in this.plugins ) {

pluginLoadingPromises.push( plugin.getLoadingPromise() );

}

$.when.apply( $.when, pluginLoadingPromises ).then( allPluginsLoadedCallback
);

If you end up using $.when, please get it right.

$.when.apply( $, promises );

or:

$.when.apply( null, promises );

not:

$.when.apply( $.when, promises );

Also, I'd recommend attaching to .done() instead of .then() if you don't need *another* deferred to be instantiated with all handlers proxied. Unnecessary overhead.

Alternatively, if you have control over the flow, you can chain deferreds instead of queueing them up in an array:

$.when(
thing1,
thing2
).then( function ( data1, data2 ) {
return $.ajax( url, { x: data1.y + data2.z } );
}).then( function ( response ) {
return response.success;
}).etc.

Change 145391 had a related patch set uploaded by Gilles:
Remove data('events') usage in jquery.triggerQueueCallback

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

Change 145393 had a related patch set uploaded by Gilles:
Use the new .bindQueueCallback() instead of .bind()

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

Change 145391 merged by jenkins-bot:
Remove data('events') usage in jquery.triggerQueueCallback

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

Change 145393 merged by jenkins-bot:
Use the new .bindQueueCallback() instead of .bind()

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

I now just get "JQMIGRATE: jQuery.fn.andSelf() replaced by jQuery.fn.addBack()" when seeking, which is an improvement!

James: that warning is actually coming from jQuery UI in core. I'm guessing there's a separate plan to update that, isn't there?

(In reply to Gilles Dubuc from comment #14)

James: that warning is actually coming from jQuery UI in core. I'm guessing
there's a separate plan to update that, isn't there?

Aha, cool. Does that mean we can mark this as fixed? :-)

Gilles triaged this task as Unbreak Now! priority.Dec 4 2014, 10:11 AM
Gilles moved this task from Untriaged to Done on the Multimedia board.
Gilles lowered the priority of this task from Unbreak Now! to Needs Triage.Dec 4 2014, 11:23 AM