Page MenuHomePhabricator

Comment advising developers to add ResourceLoader dependent modules directly
Closed, ResolvedPublic

Description

Right now, ResourceLoader does not walk the dependencies on the server. As noted in ResourceLoaderModule:

"When adding a module on the server side, dependency information is NOT taken into account and YOU are responsible for adding dependent modules as well. If you don't do this, the client side loader will send a second request back to the server to fetch the missing modules, which kind of defeats the purpose of the resource loader."

It seems that if this is actually a performance issue, we should walk (expand) the dependency tree on the server. Otherwise, it's essentially pushing work onto the clients of ResourceLoader.


Version: 1.22.0
Severity: normal

Details

Reference
bz51853

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 2:01 AM
bzimport set Reference to bz51853.

Comments don't run. And I don't think that comment is accurate (or ever was?), if it isn't inaccurate, it is ambiguous and means something else than you think.

Can you elaborate on an actual issue that supposedly results from this? (e.g. a second request, or dependencies missing).

I could talk for a while about why it by design and actually improving performance that we do not do dependencies on the server. However I'll save that for later if it is needed, let's get a grip on an actual problem first.

The part about "When adding a module on the server side, dependency information is NOT taken into account" is accurate. It is only taken into account on the client.

I don't think I've seen a confirmed example of it adding extra requests, which is why I said "if this is actually a performance issue".

If this is by design, and developers need not "add[...] dependent modules as well" the comment should obviously be removed.

Change 80408 had a related patch set uploaded by Mattflaschen:
Remove comment saying code should add RL dependencies directly

https://gerrit.wikimedia.org/r/80408

Change 80408 merged by jenkins-bot:
Remove comment saying code should add RL dependencies directly

https://gerrit.wikimedia.org/r/80408