Maniphest T191652

ResourceLoader addModuleStyles() ignores dependencies.
Closed, DeclinedPublic
Actions

Assigned To

None

Authored By

	Alexia
	Apr 6 2018, 6:09 PM

Description

Per ResourceLoader's documentation addModuleStyles() is required to be used to load CSS that will be linked through a style tag and not loaded through Javascript. However, using addModuleStyles() also ignores module dependencies. As well, using addModuleStyles() is required to avoid flash of unstyled content(FOUC) which makes the lack of dependency processing undesirable. Currently the choices to get all CSS styles to load is to use dependencies with addModules() that causes FOUC due to it loading through Javascript or not use dependencies manually defining every resource loader module when calling addModuleStyles(); both of which are undesirable for different reasons.

So instead adding one call in the code to addModuleStyles() for 'ext.extension1.styles' on pages that need it:

$output->addModuleStyles(['ext.extension1.styles']);

The dependencies have to be manually added resulting in poor code maintainability and requiring updating many places when changing dependencies.

$output->addModuleStyles(
	[
		'ext.extension1.styles',
		'ext.ext2.dep1.styles',
		'ext.ext2.dep2.styles',
		'ext.ext3.dep3.styles',
		'ext.ext3.dep4.styles',
		'ext.ext3.dep5.styles'
	]
);

Related Objects

Mentioned In: T357836: [Spike] Explore one-module-per-component approach to code splitting
T344386: Decide how to implement code splitting in Codex, and how to integrate it in ResourceLoader
T195544: [Regression pre-wmf.6] UI for dialogs with multiple tabs in VE is broken, but the problem is rare and intermittent
Mentioned Here: T63577: OutputPage::addModuleStyles should support module dependencies
T188076: Versioned load.php url for long caching gets short-cached instead

Event Timeline

Alexia created this task.Apr 6 2018, 6:09 PM

Restricted Application added a project: Performance-Team. · View Herald TranscriptApr 6 2018, 6:09 PM

Restricted Application added a subscriber: Aklapper. · View Herald Transcript

I put in a work around for this on the Hydra Wiki Platform that removes FOUC and puts all the CSS into one minified file.

diff --git a/includes/resourceloader/ResourceLoader.php b/includes/resourceloader/ResourceLoader.php
index 827c8f3c2..ff8b1b04f 100644
--- a/includes/resourceloader/ResourceLoader.php
+++ b/includes/resourceloader/ResourceLoader.php
@@ -711,6 +711,13 @@ class ResourceLoader implements LoggerAwareInterface {
         // Find out which modules are missing and instantiate the others
         $modules = [];
         $missing = [];
+
+        //Hydra Wiki Platform Hack: Shove all the dependencies into the request context.
+        if ($context->getOnly() === 'styles') {
+            $_modules = $this->getStyleDependencies($context->getModules(), $context);
+            $context->addModulesPostConstruct($_modules);
+        }
+
         foreach ( $context->getModules() as $name ) {
             $module = $this->getModule( $name );
             if ( $module ) {
@@ -808,6 +815,25 @@ class ResourceLoader implements LoggerAwareInterface {
         echo $response;
     }
 
+    /**
+     * Recurse through modules to get dependencies for styles only that should be concatenated together.
+     *
+     * @access  public
+     * @param   array   Module Names
+     * @param   object  ResourceLoaderContext
+     * @return  array   Module Names
+     */
+    public function getStyleDependencies($modules, $context) {
+        foreach ($modules as $name) {
+            $module = $this->getModule($name);
+            $dependencies = $module->getDependencies();
+            if (count($dependencies)) {
+                $modules = array_merge($modules, $this->getStyleDependencies($dependencies, $context));
+            }
+        }
+        return array_unique($modules);
+    }
+
     /**
      * Send main response headers to the client.
      *
diff --git a/includes/resourceloader/ResourceLoaderContext.php b/includes/resourceloader/ResourceLoaderContext.php
index 8955b8c2a..c4773f0af 100644
--- a/includes/resourceloader/ResourceLoaderContext.php
+++ b/includes/resourceloader/ResourceLoaderContext.php
@@ -131,6 +131,17 @@ class ResourceLoaderContext {
         return $retval;
     }
 
+    /**
+     * Add modules post construct phase mainly for adding style dependencies in style only mode.
+     *
+     * @access  public
+     * @param   array   New modules to add.
+     * @return  void
+     */
+    public function addModulesPostConstruct($modules) {
+        $this->modules = array_unique(array_merge($this->modules, $modules));
+    }
+
     /**
      * Return a dummy ResourceLoaderContext object suitable for passing into
      * things that don't "really" need a context.

@Alexia, could you submit a patch for that in Gerrit?

This sub-feature intentionally does not allow dependencies. This is a documented limitation (see RL/DEV). This patch would apply logic for JS modules to CSS modules, however these are not compatible, and would eventually break styles on websites. Allow me to explain.

When calling $out->addModuleStyles([ 'foo', 'bar' ]), the following is output:

wiki/Example

<link rel=stylesheet href="load.php...foo,bar...">

Then imagine deploying a change to stylesheets with dependencies enabled:

Make foo depend on quux, and change foo to assume quux.
At some point, the short-lived 5min cache for the stylesheet url will expire.
From that moment on, users viewing a cached copy of the article will receive a fresh response of the stylesheet. This stylesheet will contains the newest code for foo, but will not have quux. This is because the page HTML does not yet know about "quux". The page is now broken.
Other articles, created after the deployment, will output a different stylesheet url that does include quux, those pages will be fine.

The only way to make style dependencies work, is if a website is in development mode with caching disabled. This is not recommended and would break professional use of MediaWiki at-scale, including Wikipedia. See https://www.mediawiki.org/wiki/Manual:Performance_tuning#Page_view_caching for more information.

Adding them manually to addModuleStyles() is the recommended way, and has the known impact of being cached. Which means if internal assumptions change, then it must be done in two steps. For example, if we change foo to need quux, it would be done as follows:

Make sure that foo will work with and without quux.
Add quux via addModuleStyles().
Wait for a few days to allow page HTML to gradually update (exact duration depends on your configuration. For Wikipedia, this is 4 days).
Remove extra code from foo that is not needed when quux is always available.

I would welcome new ideas to make this work in an automatic way, but in the current form this is an acceptable limitation. This limitation has the benefit that ResourceLoader is fast, and can guarantee consistency between pages - assuming code is loaded through methods that are supported and documented.

For JavaScript modules, the above problem is solved via the startup module. $out->addModules([ 'foo', 'bar' ]), where foo depends on mediawiki.util:

/wiki/Example

<script async src="load.php...startup"></script>
<script>RLQ.push( ..
  mw.loader.load( [ 'foo', 'bar'] ); ...

/w/load.php...startup

mw.loader.register([
  ['foo', 'v1', ['mediawiki.util']],
  ['bar', ...

The HTML output does not contain version information or dependencies. (They are added at run-time, via Startup.) This is important. Imagine deploying this change:

We update foo.js to also make use of mediawiki.Title, and also declare its dependency.
Users viewing a cached copy of the article and a cached copy of Startup will continue to get the same old version of foo. This is fine.
At some point, the short-lived 5min cache of Startup will expire.
From that moment on, users viewing a cached copy of the article with the new Startup data will get the newer version of foo, and their browser knows about the extra dependency. ResourceLoader will load the dependencies before foo.

The dependencies have to be manually added

That is correct. But note that also needs to happen if we allow dependencies. The only difference is whether the list is in a module definition (extension.json,skin.json or Resources.php) or in the addModuleStyles call.

[..] resulting in poor code maintainability and requiring updating many places when changing dependencies.

I suspect this may be a side-effect of other aspects of the code. I can help and advise ways to avoid this, if you specify more context on your use case.

(I am trying to follow along, but poorly.) If you remove "quux" from the repository then Javascript would not be able to load it either. It is missing. Though if you mean removing "quux" as a dependency and just hoping it gets included somewhere that would be an odd choice to make. I just deployed this ResourceLoader change out to live Gamepedia/Hydra if you wish to see it live. We have a test wiki available: https://meeseeks.gamepedia.com/Meeseeks_Wiki

On ResourceLoader speed:
"This is not recommended and would break professional use of MediaWiki at-scale, including Wikipedia. ... This limitation has the benefit that ResourceLoader is fast ..." I(We) do not encounter this issue on Gamepedia/Hydra. We replaced the incredibly slow less.php implementation that comes with MediaWiki with Lessoid(Less.js implementation) that fixes all of ResourceLoader's slowness. We also fixed several other context bugs that caused ResourceLoader to rapidly evict the cache due to it not handling cache keys correctly.(Thus causing slow downs due to having to recache constantly.) After those fixes calls to load.php went from the #1 slowest request to the #2 fastest request behind api.php.

We also fixed several other context bugs that caused ResourceLoader to rapidly evict the cache due to it not handling cache keys correctly.(Thus causing slow downs due to having to recache constantly.)

Is this referring to T188076?

In T191652#4117820, @Nirmos wrote:

We also fixed several other context bugs that caused ResourceLoader to rapidly evict the cache due to it not handling cache keys correctly.(Thus causing slow downs due to having to recache constantly.)

Is this referring to T188076?

No, it is a patch we did not submit to MediaWiki since it was deemed that it would not be worth our time to submit.

@Alexia I believe you misunderstood my example. I've altered the second example below, in hope to provide clarity. If needed, I can also elaborate on the JavaScript example, but let us focus on the styles issue first.

Situation s1:

addModuleStyles( [ 'foo' ] )
Dependencies supported (e.g. with your patch).
foo depends on bar.

Outputs:

/wiki/Example

<link rel=stylesheet href="load.php... foo, bar">

This HTML may be cached. For example, by user web browsers, or by own File Cache, by own cache proxies, or by company cache proxies (office, WiFi, ISP). This cache is public, typically expires after some days, and is validated based on contents and/or timestamp of the wiki page revision.

Situation s2:

foo no longer depends on bar, but now depends on quux.
foo.css is changed to assume and override styles from quux. Its logic for "bar" is removed.

Expected outcome:

Either the page looks the same as before (bar with v1 of "foo", unchanged), or the page looks like the new situation s2.

Actual outcome:

Users will view "Example" (still cached) with a request for modules=foo,bar (cache expired, result will be fresh).
The response will not contain "quux".
The response will contain "bar" and the new version of "foo".

In T191652#4117782, @Alexia wrote:

We replaced the incredibly slow less.php implementation that comes with MediaWiki with Lessoid

The Less.php implementation from our upstream is indeed not very fast. However, with caching, we have not found it to add much overhead. And so far I haven't yet found a viable alternative. Can you confirm the following three basics?

PHP has APCu installed and enabled. (This is for "Local server" cache, which ResourceLoader uses for Less parsing, minification, and for fast validation checks).
MediaWiki has an object cache enabled. E.g. $wgMainCacheType is enabled and not "db". For example, Memcached or Redis are good options.
Enable caching for page HTML (/wiki) and load.php. E.g. MediaWiki File Cache, or Squid/Varnish, or Nginx reverse proxy, or an external CDN.

Looking at https://meeseeks.gamepedia.com it seems there is a CDN (CloudFront), but I found two possible issues with its configuration when testing load.php urls: It seems to vary the cache by User-Agent and cookie. Which means even for logged-out users, I expect cache is almost never used. User-Agent differs by operating system, device, browser, and version of all that, and language/plugin configuration. The cookies vary for basically everybody, primarily due to the use of GeoIP cookies and third-party advertisements. Those will need to be excluded and/or only include cookies for which the wiki will change itself (e.g. logged-in user cookies). And even for logged-in users, load.php will never vary by cookies. It can be cached for all users without vary.

$ curl --compressed -I -H 'User-Agent: Example3' 'https://meeseeks.gamepedia.com/load.php?debug=false&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=hydra&version=1rkfg73'
cache-control: public, max-age=2592000, s-maxage=2592000
[..]
x-cache: Miss from cloudfront

$ curl --compressed -I -H 'User-Agent: Example4' 'https://meeseeks.gamepedia.com/load.php?debug=false&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=hydra&version=1rkfg73'
cache-control: public, max-age=2592000, s-maxage=2592000
[..]
x-cache: Miss from cloudfront

$ curl --compressed -I -H 'User-Agent: Example4' -H 'Cookie: x=y' 'https://meeseeks.gamepedia.com/load.php?debug=false&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=hydra&version=1rkfg73'
cache-control: public, max-age=2592000, s-maxage=2592000
[..]
x-cache: Miss from cloudfront

[..] bugs that caused ResourceLoader to rapidly evict the cache due to it not handling cache keys correctly.

It would be most awesome if you could share more details about these in separate tasks and patches, so we can work to improve MediaWiki in this regard.

In T191652#4117908, @Krinkle wrote:
@Alexia I believe you misunderstood my example. I've altered the second example below, in hope to provide clarity. If needed, I can also elaborate on the JavaScript example, but let us focus on the styles issue first.

Situation s1:

addModuleStyles( [ 'foo' ] )

Dependencies supported (e.g. with your patch).

foo depends on bar.

Outputs:
/wiki/Example
<link rel=stylesheet href="load.php... foo, bar">
This HTML may be cached. For example, by user web browsers, or by own File Cache, by own cache proxies, or by company cache proxies (office, WiFi, ISP). This cache is public, typically expires after some days, and is validated based on contents and/or timestamp of the wiki page revision.

Situation s2:

foo no longer depends on bar, but now depends on quux.

foo.css is changed to assume and override styles from quux. Its logic for "bar" is removed.

Expected outcome:

Either the page looks the same as before (bar with v1 of "foo", unchanged), or the page looks like the new situation s2.

Actual outcome:

Users will view "Example" (still cached) with a request for modules=foo,bar (cache expired, result will be fresh).

The response will not contain "quux".

The response will contain "bar" and the new version of "foo".

I would just avoid making those kinds of breaking CSS changes without first purging the page cache. Accepting that a small percentage of users may unintentionally have a broken page for one page load is better than injecting CSS with JS when not necessary to do so. Assuming the front facing caching solutions are configured correctly this should not be much of an issue.

In T191652#4117908, @Krinkle wrote:

In T191652#4117782, @Alexia wrote:

We replaced the incredibly slow less.php implementation that comes with MediaWiki with Lessoid

The Less.php implementation from our upstream is indeed not very fast. However, with caching, we have not found it to add much overhead. And so far I haven't yet found a viable alternative. Can you confirm the following three basics?

PHP has APCu installed and enabled. (This is for "Local server" cache, which ResourceLoader uses for Less parsing, minification, and for fast validation checks).

MediaWiki has an object cache enabled. E.g. $wgMainCacheType is enabled and not "db". For example, Memcached or Redis are good options.

Enable caching for page HTML (/wiki) and load.php. E.g. MediaWiki File Cache, or Squid/Varnish, or Nginx reverse proxy, or an external CDN.

Yes, we have all of that. There were still speed issues with Less.php that were causing outages for Hydra. The other reason we made Lessoid was to be able to use the newest versions of Less.js that fixes bugs present in Less.php.

In T191652#4117908, @Krinkle wrote:
Looking at https://meeseeks.gamepedia.com it seems there is a CDN (CloudFront), but I found two possible issues with its configuration when testing load.php urls: It seems to vary the cache by User-Agent and cookie. Which means even for logged-out users, I expect cache is almost never used. User-Agent differs by operating system, device, browser, and version of all that, and language/plugin configuration. The cookies vary for basically everybody, primarily due to the use of GeoIP cookies and third-party advertisements. Those will need to be excluded and/or only include cookies for which the wiki will change itself (e.g. logged-in user cookies). And even for logged-in users, load.php will never vary by cookies. It can be cached for all users without vary.
$ curl --compressed -I -H 'User-Agent: Example3' 'https://meeseeks.gamepedia.com/load.php?debug=false&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=hydra&version=1rkfg73'
cache-control: public, max-age=2592000, s-maxage=2592000
[..]
x-cache: Miss from cloudfront

$ curl --compressed -I -H 'User-Agent: Example4' 'https://meeseeks.gamepedia.com/load.php?debug=false&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=hydra&version=1rkfg73'
cache-control: public, max-age=2592000, s-maxage=2592000
[..]
x-cache: Miss from cloudfront

$ curl --compressed -I -H 'User-Agent: Example4' -H 'Cookie: x=y' 'https://meeseeks.gamepedia.com/load.php?debug=false&lang=en&modules=jquery%2Cmediawiki&only=scripts&skin=hydra&version=1rkfg73'
cache-control: public, max-age=2592000, s-maxage=2592000
[..]
x-cache: Miss from cloudfront

If you curl those each twice in a row you will see they do get cached. However, our Cloudfront caching is set to aggressively vary and evict. The Varnish servers handle the vast majority of the caching work and Cloudfront acts as a DDOS protection layer.

In T191652#4117908, @Krinkle wrote:

[..] bugs that caused ResourceLoader to rapidly evict the cache due to it not handling cache keys correctly.

It would be most awesome if you could share more details about these in separate tasks and patches, so we can work to improve MediaWiki in this regard.

The only reason I opened this Phabricator ticket is because I was talking about this issue in the MediaWiki-General IRC channel and Mark asked me to open it. I have reviews open in Gerrit from 2015 and 2016. One of them is a casing correction for an array index that broke extensions that rely on it. A couple times reviews got merged immediately, but the outside those outliers the current time to merge is about six months. Which causes additional work to go back, rebase, refix based on other changes, and resubmit for review. That is why we have guidelines for what patches we take time to submit.

Krinkle mentioned this in T195544: [Regression pre-wmf.6] UI for dialogs with multiple tabs in VE is broken, but the problem is rare and intermittent.Aug 17 2018, 7:25 PM

(These two highly related old tasks didn't refer to each other for some reason, I thought I should fix that.)

Restricted Application added a project: MediaWiki-Platform-Team. · View Herald TranscriptAug 15 2023, 11:47 PM

Catrope mentioned this in T344386: Decide how to implement code splitting in Codex, and how to integrate it in ResourceLoader.Aug 16 2023, 10:41 PM

Catrope mentioned this in T357836: [Spike] Explore one-module-per-component approach to code splitting.Feb 17 2024, 1:53 AM

ResourceLoader addModuleStyles() ignores dependencies.Closed, DeclinedPublicActions

Description

Related Objects

Event Timeline

ResourceLoader addModuleStyles() ignores dependencies.
Closed, DeclinedPublic
Actions