Page MenuHomePhabricator

mw.loader.using should not throw an exception when an optional module doesn't exist
Closed, DeclinedPublic

Description

To give an example:
mediawiki.page.ready.js runs on mobile (it specifies targets=>mobile) however attempts to lazy load the tablesorter plugin which is not available on mobile (it specifies targets=>desktop)

This currently throws an exception.

Avoiding a bike shedding conversation about whether jquery.tablesorter should be available on mobile, it is wrong to make the assumption that a module will always be available.

Expected: Instead of seeing an exception I would like to allow the user to handle the error themselves - allowing optional modules to die silently.

e.g.
mw.loader.using( 'jquery.tablesorter', function () {
$sortableTables.tablesorter();
}, function( e ) {
throw e;
});

or
mw.loader.using( 'jquery.tablesorter' ).done( function () {
$sortableTables.tablesorter();
});

Another example of this being a problem is on the edit page which asumes mediawiki.action.edit is available:

mw.loader.using("mediawiki.action.edit"

Currently there is a hack in MobileFrontend to get around this [1]

[1] https://gerrit.wikimedia.org/r/gitweb?p=mediawiki/extensions/MobileFrontend.git;a=blob;f=includes/skins/SkinMobile.php;h=27ab4ef44fc138566bbf3e1ab32cd5af20daf246;hb=HEAD#l241


Version: 1.22.0
Severity: normal

Details

Reference
bz47296

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 1:42 AM
bzimport set Reference to bz47296.
bzimport added a subscriber: Unknown Object (MLST).

"allow the user to handle the error themselves" should read "allow the developer to handle the error themselves"

jgonera wrote:

So what is the problem exactly? How the proposed solutions fix it? Why wouldn't the developer do:

try {

mw.loader.using( ...

} catch ( e ) {

...

}

Well it's more a historic problem. If a module has been added via out->addModules previously requesting a module vs mw.loader.using was unlikely to throw an exception.

Now with the targets property this becomes more muddled.. a module may or may not be there - it's at the mercy of the current target.

My suggestion is to alter the implementation mw.loader.using so instead of throwing an error it accepts an error callback or even better is rewritten to return a deferred object.

It should be up to the developer to throw an exception.

Related URL: https://gerrit.wikimedia.org/r/60346 (Gerrit Change Ic2f81b725acac2086178e33033f10c4c80ae1573)

That change should go out during next Tuesday's regular deployment

Re-opening. Please don't mark bug fixed until they are approved and landed in master.

I'm not convinced that this should be solved by suppressing the exception and turning it into an optional callback.

When a dependency is specified in the module manifest that doesn't exist[1], we throw. This is currently the case and (afaik) acceptable behaviour because it is a fatal problem. The code must not be attempted to run without that other module.

When a dependency is specified inline at runtime (wrapping the code in a function) which doesn't exist, we throw. I don't see why that would be any different.

Yes, mw.loader.using has an error callback. But aside from this being almost never used (I haven't seen it in use, ever), it is unlikely that an implementation would be anything other than throwing an error. The module ought to be there.

When a module is added to the page by an extension or by core, it is with the intention to make something happen on that page. If that module is unable to load its code, that should throw.

ext.wikiLove.init, ext.articleFeedback.init, VisualEditor, mediawiki.page.ready. All of these load modules after a check or event. They should not be loaded on the page if their dependencies are not available, and loading it anyway should throw.

I'd recommend: Look at this error and fix it.

In this case it means a module is loaded on mobile but one of its dependencies is not yet compatible with mobile. That means it (=jquery.tablesorter) should either be made compatible, or the containing module removed from mobile for the mean while (=mediawiki.page.ready). This isn't my own idea, I've seen this happen before with various other modules and I'm merely suggesting we act the same.

This error is good, because without it you wouldn't have known that we made a mistake and forgot one of the dependencies of mediawiki.page.ready when you enabled it on mobile. This is how we found them before as well, except before they were server-side dependencies and now they're inline.

Having said that, I'd like a view from Roan and/or Trevor on this as well. Personally I'd like to keep this exception and instead deprecate the error callback.

[1] Exist as in, in the current execution. Because since the 'target' property was created a module can exist in general, but be invisible to the current request context based on the 'target' parameter, thus making any errors related to it somewhat ambiguous (not visible in this target vs. module not existing in general - very different problems).

(In reply to comment #5)

That change should go out during next Tuesday's regular deployment

For the deployment I think it is more feasible to simply enable jquery.tablesorter on mobile. We can decide whether to surpress error reporting on missing dependencies later.

jgonera wrote:

Krinkle, I agree. If we deprecate the error callback, then throwing an error is OK. But let's make it consistent.

Also, Jon, this particular mediawiki.action.edit case will become less important to us because we can, and hopefully soon will, provide our own custom edit form, instead of the one generated by EditPage.php.

jQuery.tablesorter seems to be an optional dependency - the code is loaded in the client only on pages which need it (otherwise the 'mediawiki.page.ready' resource loader module would specify it as a hard dependency in its dependencies)

If jquery.tablesorter was listed in the module manifest [1] for mediawiki.page.ready I could understand it throwing an exception when mediawiki.page.ready uses it - but it is /NOT/ thus the code in mediawiki.page.ready is making a bad assumption that that module exists and is available. In this case it is an optional module (otherwise it would be in the module manifest of a module that /has/ been knowingly added to the page) and the page will survive without it. I am sure however in other cases an exception might make more sense e.g. mw.using( 'mediawiki.modulethatwasrenamed' ). I don't know the resource loader code well enough but it seems it would be useful to distinguish between modules that do not exist and modules that have the wrong target.

On a side note mediawiki.page.ready doesn't look very useful to mobile - input placeholders are not really a problem on modern mobile browsers, we don't use the collapsible code, as this bug reveals the tablesorter isn't expected to work, access keys are useless on mobile and CheckboxShiftClick - I have no idea what that is but doesn't sound very mobile friendly. For our particular problem we should probably simply disable this module on the mobile site - yet it seems to be needed for qunit (wtf)?! [2]

[1]
'mediawiki.page.ready' => array(

		'scripts' => 'resources/mediawiki.page/mediawiki.page.ready.js',
		'dependencies' => array(
			'jquery.checkboxShiftClick',
			'jquery.makeCollapsible',
			'jquery.placeholder',
			'jquery.mw-jump',
			'mediawiki.util',
		),
		'targets' => array( 'desktop', 'mobile' ),

),

Krinkle I disagree that we should enable jquery.tablesorter on mobile and we certainly shouldn't do it 'simply' - but that's another bug/discussion (see my comments on bug 47858) - let's not discuss that here.

[2] Change-Id: I84e0512590de9ff2dbdf519d983a0c1c2d007194

See bug 47882 to discuss my side note about mediawiki.page.ready - please leave discussion on this bug simply about behaviour for when a module doesn't exist.

This is now less of a problem in mobile as we have reduced the possibility of this happening to only situations where a module has been renamed/typed incorrectly/ is not available due to being in mobile mode rather than desktop. The tablesorter exception is no longer called.

My main worry about this is that since ResourceLoader concatenates code one module failing can prevent the execution of another module. This seems wrong. I can understand an entire module from throwing an exception and not executing but stopping all javascript from executing seems wrong.

I really don't understand the need for an exception under all circumstances. Could we not default the error handler to throwing an exception to at least allow developers to override it. For example a gadget might want to do something with a module if it's available but fail silently if it's not so it can be used on as many wiki's as possible that may or may not have the module it does something with)

// module 1
mw.loader.using( 'foobar', function() {
} );

// module 2
(function(){ alert( 4 ); })()

On a similar subject it would be great to see mw.loader.using return a jQuery.deferred object.

As the Target Milestone on this ticket has been set to 1.22.0:

According to http://lists.wikimedia.org/pipermail/wikitech-l/2013-September/072030.html "MediaWiki 1.22 is slated for release on November 30th, at the very latest."

If this is still intended to get fixed for 1.22.0, a patch is needed soon.

Is this blocking the release, and if yes, why has nobody done anything about it? I sympathise with Krinkle's comment 7.

I am happy for someone to agree with Krinle and close this as WONTFIX
The mobile skin doesn't load mediawiki.page.ready now since we introduced Skin::getDefaultModules so the original problem isn't a problem.

If someone really wants to load something optionally they can use a try / catch

I'll tentatively close this, then, to have less stuff seemingly blocking the release.