Last modified: 2012-06-20 08:28:18 UTC
mw.loader.using always calls ready callback for unregistered modules, when the callback shouldn't get called at all. Examples: mw.loader.using('', function() { alert('running...'); }); mw.loader.using([], function() { alert('running...'); }); mw.loader.using([''], function() { alert('running...'); }); mw.loader.using('foobar', function() { alert('running...'); }); mw.loader.using(['foobar'], function() { alert('running...'); }); I've been able to trace this back to a few problems that are probably also causing bugs in other parts of mediawiki.js as well. First, resolve() returns [] for each module that isn't previously registered. In most places the return value of resolve() is put back into the same variable as was passed (like dependencies = resolve(dependencies)), when the original value might be needed for comparison. Second, the variable passed as the second argument of filter() is replaced with all registered modules when passed []. As can happen when the return value of resolve() is used and it is passed unregistered modules. Again the original value of the second argument might be needed for comparison. Third, compare() may return true or false in ways not intended at times. Like when used to compare filtered and unfiltered return results of resolve(unregistered modules). The filtered and unfiltered results may be the same at times depending on the current state of all registered modules. The ready() or error() callback function is called immediately if the filtered and unfiltered list of all registered modules happens to be the same at the time. Four, if somehow both compare tests manage to fail, every registered module will be passed to request(). Whatever happens to be the first registered module will have its dependencies appended to everything else, this new list will be added to jobs and queue, and work() will do a lot of extra work. Finally, AFAIK if request() is somehow called neither callbacks are called because request() doesn't call them and doesn't pass them onto queue, so work() has no way of calling them either.
Presumably we should call the error callback in this case...? Adding needs-unittest keyword as these things should be tested in the loader tests.
No. An unregistered module needs to inject a request and let the load system know we want to know about any future modules that are registered by the given name, so it can call the ready or error callback when an attempt to load the module is finally made and either succeeds or fails. mw.loader.using could presumably return true if at the time it is called all requested modules are registered or false otherwise.
To explain a bit more, mw.loader.using might be called before: 1. One or more modules are known about. 2. Any dependencies are known about. 3. Dependencies have reached ready or error status. 4. One or more modules have reached ready or error status. mw.loader.using might be called after: 5. Some modules are known about and others are not. 6. Some dependencies are known about and others are not. 7. Some dependencies have reached ready status and others have not. 8. Some dependencies have reached error status. 9. All modules and dependencies have reached ready status. 1 through 7 require deferred checking until 8 or 9 is true. 8 can call the error callback immediately. 9 can call the ready callback immediately.
... so you agree with me that the error callback should be called at load time when you did a mediawiki.loader.using() call for an unregistered module that stays unregistered?
(In reply to comment #4) > ... so you agree with me that the error callback should be called at load time > when you did a mediawiki.loader.using() call for an unregistered module that > stays unregistered? Yes, if you mean mw.loader.load(module) is called before or without mw.loader.register(module) or mw.loader.implement(module).
(In reply to comment #5) > (In reply to comment #4) > > ... so you agree with me that the error callback should be called at load time > > when you did a mediawiki.loader.using() call for an unregistered module that > > stays unregistered? > > Yes, if you mean mw.loader.load(module) is called before or without > mw.loader.register(module) or mw.loader.implement(module). .load() indeed, as well as for .using()
(In reply to comment #3) > mw.loader.using might be called before: > 1. One or more modules are known about. What about that case though ?
(In reply to comment #7) > (In reply to comment #3) > > mw.loader.using might be called before: > > 1. One or more modules are known about. > > What about that case though ? Deffer, as I said in comment #3. mw.loader.using should work similarly for modules as how $(document).ready does for a document.
(In reply to comment #6) > (In reply to comment #5) > > (In reply to comment #4) > > > ... so you agree with me that the error callback should be called at load time > > > when you did a mediawiki.loader.using() call for an unregistered module that > > > stays unregistered? > > > > Yes, if you mean mw.loader.load(module) is called before or without > > mw.loader.register(module) or mw.loader.implement(module). > > > .load() indeed, as well as for .using() I disagree. .using() should be safe to call before .register and .implement. Just as $(document).ready is safe to call before the document is ready and the callback isn't called until the document is ready.
(In reply to comment #0) > mw.loader.using always calls ready callback for unregistered modules, > when the callback shouldn't get called at all. > > Examples: > > mw.loader.using('', function() { alert('running...'); }); > mw.loader.using([], function() { alert('running...'); }); > mw.loader.using([''], function() { alert('running...'); }); > mw.loader.using('foobar', function() { alert('running...'); }); > mw.loader.using(['foobar'], function() { alert('running...'); }); > This has been fixed in 1.19.0 or 1.20-alpha (not sure) and was recorded under another bug as well. These examples now result in: > Error: Unknown dependency: foobar (except for mw.loader.using( [] ), which as expected requires no modules.)
