Page MenuHomePhabricator

mw.util.$content is undefined in MediaWiki:Common.js
Closed, ResolvedPublic

Description

From: http://labs.wikimedia.deployment.wmflabs.org/w/index.php?diff=237

This is about http://commons.wikimedia.deployment.wmflabs.org

  1. mw.util is undefined when attempting to access from MediaWiki:Common.js without using mw.loader.load
  2. despite wrapping all inside mw.loader.using('mediawiki.util', function(){ mw.util.$content is null. But it shouldn’t since the code wrapped inside mw.loader.using('mediawiki.util'... should be executed after mw.util is initialized

Version: 1.20.x
Severity: normal

Details

Reference
bz33711

Event Timeline

bzimport raised the priority of this task from to Unbreak Now!.Nov 21 2014, 11:59 PM
bzimport set Reference to bz33711.

Just FYI, seen on twn (more info if requested):

 2 'mw.util.$content' ist Null oder kein Objekt
 2 Uncaught exception: TypeError: Cannot convert 'mw.util' to object
18 'util' ist Null oder kein Objekt

(In reply to comment #0)

From: http://labs.wikimedia.deployment.wmflabs.org/w/index.php?diff=237

This is about http://commons.wikimedia.deployment.wmflabs.org

  1. mw.util is undefined when attempting to access from MediaWiki:Common.js

without using mw.loader.load

I could reproduce this, but INVALID bug.

This is correct and expected behavior and has been since 1.17. Only difference is that due to the loader being improving and loading faster, race conditions are more likely to ocurr, thus requiring that dependencies are always declared. This is best done by modularizing code into gadgets. Alternatively one can use mw.loader.using inline. Previously mw.util was usually loaded before 'site'. It can happen that 'site' arrives first. Order is (and should be) asynchronous, dependency declaration is mandatory.

  1. despite wrapping all inside mw.loader.using('mediawiki.util', function(){

mw.util.$content is null. But it shouldn’t since the code wrapped inside
mw.loader.using('mediawiki.util'... should be executed after mw.util is
initialized

There is a difference between mw.util being undefined and mw.util.$content being null. mw.util is defined in the mediawiki.util module, which can be at any point in time. Including before the body content is parsed. For that reason mediawiki.util is and always has populated mw.util.$content from a document-ready hook. Again, due to things loading faster now (yay), it is even more important that less is assumed and more is declared.

I've reverted Rillke's changed to MediaWiki:Common.js on commons.wikimedia.beta.wmflabs back to the original, and wrapped it in mw.loader.using.

Since mw.util.$content wasn't used there it's fine now. It is used within a $(document).ready hook but that's fine.

  • Bug 33712 has been marked as a duplicate of this bug. ***

mw.loader.using('mediawiki.util',function($){
console.log($)
});

$ is undefined.

So mw.util.$content can't be reached because code fails before.

  • Bug 33669 has been marked as a duplicate of this bug. ***

If noone cares, I have to reopen this bug.

  1. Dependencies. Ok. Just wanted a confirmation. Please note that also the user's common.js / vector.js are affected. This will cause a lot of trouble since MediaWiki:Common.js waits for mw.util while user's js is executed. This will cause all "importScript"s and other calls to be rewritten by each user. Or we invent something to queue the import-requests and execute them when ready.
  1. mw.util.$content is still null in http://commons.wikimedia.beta.wmflabs.org/wiki/MediaWiki:Common.js *despite being wrapped inside a $(function() {})*. I added debug code and uploaded a screenshot: http://commons.wikimedia.beta.wmflabs.org/wiki/MediaWiki:Common.js

(In reply to comment #4)

mw.loader.using('mediawiki.util',function($){

$ is undefined.

So mw.util.$content can't be reached because code fails before.

Your logic fails... $ and $content are unrelated. $ is an alias for jQuery, while $content is an alias for #bodyContent (depending on skin). function($) is pointless, as the callback from mw.loader.using() does not pass anything (which is fortunate, as that would break jQuery).

Something seems not to be working quite right. Below are two queries for both mw.util.$content and $('#bodyContent'), both in Monobook and Vector.

querie format:
console.log( 'Begin of Common.js: mw.util.$content is', typeof( mw.util.$content ), mw.util.$content );
console.log( 'Begin of Common.js: #bodyContent is', typeof( $( '#bodyContent' ) ), $( '#bodyContent' ) );

Result in Monobook:
[18:59:47.760] Begin of Common.js: mw.util.$content is object null
[18:59:47.766] Begin of Common.js: #bodyContent is object ({length:1, 0:({}), context:({}), selector:"#bodyContent"})
[18:59:47.777] End of Common.js: mw.util.$content is object null
[18:59:47.783] End of Common.js: #bodyContent is object ({length:1, 0:({}), context:({}), selector:"#bodyContent"})
[18:59:47.789] On document.ready: mw.util.$content is object null
[18:59:47.796] On document.ready: #bodyContent is object ({length:1, 0:({}), context:({}), selector:"#bodyContent"})

Result in Vector:
[19:00:35.994] Begin of Common.js: mw.util.$content is object null
[19:00:36.001] Begin of Common.js: #bodyContent is object ({length:1, 0:({}), context:({}), selector:"#bodyContent"})
[19:00:36.012] End of Common.js: mw.util.$content is object null
[19:00:36.017] End of Common.js: #bodyContent is object ({length:1, 0:({}), context:({}), selector:"#bodyContent"})
[19:00:36.024] On document.ready: mw.util.$content is object null
[19:00:36.031] On document.ready: #bodyContent is object ({length:1, 0:({}), context:({}), selector:"#bodyContent"})

(In reply to comment #8)

Your logic fails... $ and $content are unrelated.

I suggest you read the changes Krinkle made. I know that they are not the same.

But you should also read what Krinkle wrote here:

Since mw.util.$content wasn't used there it's fine now. It is used within a

$(document).ready hook but that's fine.
Krinkle thinks that when adding a dependency to mw.util AND wrapping the code accessing mw.util.$content inside a $(document).ready(function(){...}) or its short form $(function(){...}), mw.util.$content must be defined, which is not the case here why I reopened the bug.

http://svn.wikimedia.org/viewvc/mediawiki/trunk/phase3/resources/mediawiki/mediawiki.util.js?view=markup#l58
If you look at the source-code of the mw.util -module you will see that $content is predefined with null and there is a "init"-method that sets $content to what it should be.

The problem? is that init can only be invoked by $(document).ready() because RL-modules may be loaded before the DOM is ready.

So the question to solve is, why mw.util.$content is null.

I've reverted Rillke's changed to MediaWiki:Common.js on

commons.wikimedia.beta.wmflabs back to the original, and wrapped it in
mw.loader.using.
http://commons.wikimedia.beta.wmflabs.org/w/index.php?title=MediaWiki%3ACommon.js&diff=22461&oldid=22442
sans commentaire

Also interesting:

/**

  • Initialisation
  • (don't call before document ready) */

init: function() { ...

I don't *when* RL calls init, but is it is before document.ready, perhaps we will have to wrap common.js itself in $(document).ready, then load util within.

Wrapping mw.loader.using() itself in a $(document).ready works:

Begin of Common.js: mw.util.$content is object [<div id=​"bodyContent">​...</div>]
Begin of Common.js: #bodyContent is object [<div id=​"bodyContent">​...</div>]

How does ResourceLoader ensures document.ready before loading mw.util? If it doesn't, then $content will never be properly defined.

mw.util.$content is initialized by mediawiki.page.ready, so if you need mw.util.$content, you should register $(document).ready after mediawiki.page.ready does so.
So I think it will work if you wrap the whole code into
mw.loader.using(['mediawiki.util', 'mediawiki.page.ready'], function () {

});

(which I didn't try and which seems ugly to me).

document-ready is a jQuery event (queue). Both your handler and the mw.util.init are added to that queue. Depending on in which order jQuery executes them, it will mean that for your script it's undefined or not.

I remember a bug about this a few months back and we fixed (it was a bug about the unit test being unable to test the initializer becuase it initilized upon loading the script). As a good practice its recommended to separate the definitions of methods and actually executing code, and so we (I) did.

As a result the mediawiki.page.ready module was added, which would actually initilize stuff (i.e. one should be able to load jquery.checkboxShiftClick without having it do something by default that can't be undone).

However this doesn't change the fact that document-ready is still an event queue of which the order is practically uncontrollable.

Wrapping the mw.loader.using call inside document-ready instead of the other way around doens't sound like something that should work or is supported so I don't recommend relying on that. It actually seems even counter-intuitive as it would add it to the queue before loading mw.util which would enqueue itself after yours.

Anyway I remember a fix for this, but I can't think of what it was. I'll take a fresh look at this tonight.

(mid-air collision)

(In reply to comment #10)

(In reply to comment #8)

Since mw.util.$content wasn't used there it's fine now. It is used within a

$(document).ready hook but that's fine.
Krinkle thinks that when adding a dependency to mw.util AND wrapping the code
accessing mw.util.$content inside a $(document).ready(function(){...}) or its
short form $(function(){...}), mw.util.$content must be defined, which is not
the case here why I reopened the bug.

That is indeed what I thought.
Having mw.util available is good, but $content isn't available until document-ready. init() is being ccalled in document-ready, becuase before that event there is nothing $content can be set to, the elements need to exist before they can be selected and cached in $content.

Why not make $content a function queued in document.ready? That should ensure it always returns populated.

I think I found a good solution for the problem. I didn't test it, but it should work and additionally solve several other bugs (bug 30713, bug 33399):

  1. In mediawiki.util.js create a jQuery.Callback( 'memory' ) and make it a public member of mw.util, perhaps as mw.util.callback
  2. In mediawiki.page.ready.js call mw.util.callback.fire() for it after mw.util.$content has been initialized.
  3. Whenever the content is changed (at least Live Preview does this) call mw.util.callback.fire() again.
  4. Replace $( document ).ready( callback ) with mw.util.callback.add( callback ) everywhere where the callback relies on $content and should be called when the content changes (in mediawiki.page.ready.js at least makeCollapsible and tablesorter).

(In reply to comment #18)

Is this a duplicate of bug 33746?

No, the users reporting this bug do have the module loaded. But a certain variable isn't initialized when accessed before the document is ready.

(In reply to comment #17)

I think I found a good solution for the problem. I didn't test it, but it
should work and additionally solve several other bugs (bug 30713, bug 33399):

  1. In mediawiki.util.js create a jQuery.Callback( 'memory' ) a [...]

The general concept of these kind of hooks is good, but I'd rather not do it ad hoc inside mw.util. There is is a general bug about the need for this, among the reasons is the ability to have ajax preview represent a normal view by having javascript handlers run on a 'mw.wikipage.onload' hook instead of window.document.ondomready. This is requested in bug 23580 and in bug 23580 comment 13 I had the same idea as you, using jQuery.Callbacks indeed.

Created attachment 9900
Patch for a simple implementation

The patch just does what I suggested in comment 17. Please note that I ran into huge troubles with the edit toolbar, I opened bug 33922 for this. Feel free to do whatever you think should be done with the patch.

Attached:

Created attachment 9901
Sample MediaWiki:Common.js

Example MediaWiki:Common.js to show that/how it works.

Attached:

Resolved in r110542.

Advanced callback system will also soon be done by bug 23580.

Now that mw.util is loaded at the top as a dependency of mw.page.startup, does this not in fact negate bug 33746/r110254 ?