Last modified: 2014-07-15 15:10:37 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 65428 - Fix "JQMIGRATE: Use of jQuery.fn.data('events') is deprecated" in TimedMediaHandler
Fix "JQMIGRATE: Use of jQuery.fn.data('events') is deprecated" in TimedMediaH...
Status: RESOLVED FIXED
Product: MediaWiki extensions
Classification: Unclassified
TimedMediaHandler (Other open bugs)
master
All All
: Unprioritized normal (vote)
: ---
Assigned To: Gilles Dubuc
:
Depends on:
Blocks: code_quality jqmigrate
  Show dependency treegraph
 
Reported: 2014-05-17 07:48 UTC by James Forrester
Modified: 2014-07-15 15:10 UTC (History)
11 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description James Forrester 2014-05-17 07:48:59 UTC
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
Comment 1 Tisza Gergő 2014-05-17 16:17:28 UTC
MwEmbedSupport/MwEmbedModules/MwEmbedSupport/jquery/jquery.triggerQueueCallback.js
On the first glance it looks like someone was trying to reinvent promises.
Comment 2 Michael Dale 2014-05-19 14:00:17 UTC
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?
Comment 3 Krinkle 2014-05-19 14:11:52 UTC
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 );
Comment 4 Michael Dale 2014-05-19 15:05:19 UTC
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 )
Comment 5 Tisza Gergő 2014-05-19 18:31:57 UTC
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 );
Comment 6 Michael Dale 2014-05-19 18:41:06 UTC
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.
Comment 7 Tisza Gergő 2014-05-19 18:52:31 UTC
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.
Comment 8 Krinkle 2014-05-23 00:06:28 UTC
(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.
Comment 9 Gerrit Notification Bot 2014-07-10 18:22:49 UTC
Change 145391 had a related patch set uploaded by Gilles:
Remove data('events') usage in jquery.triggerQueueCallback

https://gerrit.wikimedia.org/r/145391
Comment 10 Gerrit Notification Bot 2014-07-10 18:26:01 UTC
Change 145393 had a related patch set uploaded by Gilles:
Use the new .bindQueueCallback() instead of .bind()

https://gerrit.wikimedia.org/r/145393
Comment 11 Gerrit Notification Bot 2014-07-11 20:34:54 UTC
Change 145391 merged by jenkins-bot:
Remove data('events') usage in jquery.triggerQueueCallback

https://gerrit.wikimedia.org/r/145391
Comment 12 Gerrit Notification Bot 2014-07-11 20:34:59 UTC
Change 145393 merged by jenkins-bot:
Use the new .bindQueueCallback() instead of .bind()

https://gerrit.wikimedia.org/r/145393
Comment 13 James Forrester 2014-07-11 23:51:04 UTC
I now just get "JQMIGRATE: jQuery.fn.andSelf() replaced by jQuery.fn.addBack()" when seeking, which is an improvement!
Comment 14 Gilles Dubuc 2014-07-12 00:14:14 UTC
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?
Comment 15 James Forrester 2014-07-12 00:16:49 UTC
(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? :-)

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links