Page MenuHomePhabricator

mw.loader.using always calls ready callback for unregistered modules
Closed, DeclinedPublic

Description

Author: darklama

Description:
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.


Version: unspecified
Severity: normal

Details

Reference
bz30446

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 11:58 PM
bzimport set Reference to bz30446.
bzimport added a subscriber: Unknown Object (MLST).

Presumably we should call the error callback in this case...?

Adding needs-unittest keyword as these things should be tested in the loader tests.

darklama wrote:

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.

darklama wrote:

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:

  1. Some modules are known about and others are not.
  2. Some dependencies are known about and others are not.
  3. Some dependencies have reached ready status and others have not.
  4. Some dependencies have reached error status.
  5. 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?

darklama wrote:

(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 ?

darklama wrote:

(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.

darklama wrote:

(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.)