In T281431#7044011, @Krinkle wrote:

Perhaps as a separate task, but consider whether this skins.minerva.messageBox.styles is really deservant of another module. Minerva is booming at a rather high total of 23 registered bundle entry points.

[…] let's say for now it's worth it in the long run to […] omit [this] from page views. That means it should move out of the main payload. That doesn't mean it should get its own bundle entry point. The default approach should be, unless dictated otherwise by a demonstrated performance need, to add it to a different payload. Really any secondary minerva CSS payload of your choosing would work. […]

I'd recommend considering something like one bundle for all the styles loaded on "most" page views, and then move the rest to a "secondary" bundle that is mostly reserved for non-pageview scenarios. Possibly carving things out of the latter if they are considerably large in size and rarely loaded even for contributors.

Change 683457 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/skins/MinervaNeue@master] Move skins.minerva.content.styles into skins.minerva.base.styles

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

/cc @ovasileva

The main hurdle here is the Android and iOS apps which use these. The modules were originally broken up for apps. Renaming modules has broken them in the past so we need to coordinate with those parties first.

We want to adjust some styles in the mobile web interface to satisfy a performance recommendation but we are concerned what impact this may have on apps. Jazmin and Josh could you check with your engineers how mobile web styles are used (if at all) ?

Thanks, we should double check with them indeed. I did check Codesearch while drafting the patch, and did not find any live or regular/automated references to these modules outside of Minerva and MobileFrontend.

I did indeed find a word match in the mobileapps service repository, however for some years now those contain a forked copy of the Minerva styles, kept server-side, and exposed from a stable module-agonostic URL to apps. I don't know exactly how long ago this change was made and whether we still support app installs predating that change. I do note however, that assuming those apps referenced both modules names (like the mobileapps service does today), then the change is safe even for those old app installs since I'm not renaming any modules. The change is only considered breaking if there are live consumers that use (only) use skins.minerva.content.styles, without skins.minerva.base.styles. ResourceLoader handles this gracefully from load.php.

I don't know how regularly the forked copy in mobileapps is updated, but I do note that it is a full copy of the source. Including internal files that are not module names, such as "tables.less" and "hacks.less". And this was last synced over a year ago. I imagine that when that time comes around to update this fork, it would naturally end following this change the same as any other internal changes, ending up, among other changes, with the response handler including one few LESS files.

Just spoke with @Dbrant and it appears that Product Infrastructure would actually have to look at this first @ssastry

Just piping in with my iOS digging, we currently request these styles from mobileapps:

https://meta.wikimedia.org/api/rest_v1/data/css/mobile/base
https://{wiki-specific-host}/api/rest_v1/data/css/mobile/site
https://meta.wikimedia.org/api/rest_v1/data/css/mobile/pcs

Agreed PI will need to look at this for these endpoints, this document looked helpful. As late as April 2019 (version 6.2.3), we had some code in our repo that asked for minerva styles directly, but even then it was in a Makefile and appears to be a one-off download into a local copy of the styles, so I don't think older versions will be a problem. @JMinor I can dig back further if needed.

I've checked minerva theme handling in mobileapps and it seems that all it does is just getting .less files from private/styles/minerva and transpile/concat them into base.css during build process.
As per commit [1] minerva styles were added manually. However, here [2] was added functionality to handle request directly to ResourceLoader module. Regarding commit message from [2] we are unable to access load.php explicitly via preq module to get the relevant minerva theme from the ResourceLoader.
I was expecting that within the scope of this task I need to implement some sort of async function to upload particular theme styles (as a single bundle file), but last research confused me about whether it is appropriate.
@MSantos , @Mholloway could you assist?

[1] - https://github.com/wikimedia/mobileapps/commit/effc70b2c0abb412a46de33f81f7a5f1da8798a7
[2] - https://github.com/wikimedia/mobileapps/commit/d8ecc6369170844bb3df5e44097c6ae9ed6a2723

@MSantos @Mholloway @vadim-kovalenko if you are relying on files, then I think we can verify this by you checking if your scripts still work after https://gerrit.wikimedia.org/r/c/mediawiki/skins/MinervaNeue/+/683457 is merged.

Change 700384 had a related patch set uploaded (by Vadim Kovalenko; author: Vadim Kovalenko):

[mediawiki/services/mobileapps@master] Minerva should use a single ResourceLoader module for shipping its styles

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

I've uploaded a preliminary patch with updated minerva styles for mobileapps, pls check.
cc: @MSantos, @Jdlrobson

@vadim-kovalenko thanks! Could you also check the acceptance criteria box in the task description as well?

I've merged the first patch from @Krinkle but that only covers bullet point 1.
Given the analysis above, there should be nothing blocking the other bullet points, and skins.minerva.mainMenu.styles can be safely merged into the same module. Please note the desire to name the new module consistently with other skins.

Change 683457 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Move skins.minerva.content.styles into skins.minerva.base.styles

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

Change 700993 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/MobileFrontend@master] Fix `npm run doc` to reflect change in Minerva

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

Change 700993 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] Fix `npm run doc` to reflect change in Minerva

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

Change 701574 had a related patch set uploaded (by Vadim Kovalenko; author: Vadim Kovalenko):

[mediawiki/services/mobileapps@master] Retrieve styles from MinervaNeue extension

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

In T266361#7170518, @gerritbot wrote:

Change 683457 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Move skins.minerva.content.styles into skins.minerva.base.styles

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

When this was deployed as part of wmf.12, it worked as expected.

Then when production was briefly rolled back to wmf.11, the pages that were cached in the CDN whilst we were on wmf.12 started suffering from T285966. The problem is quite obvious in retrospect:

• The wmf.12 branch changes skins.minerva.base.styles to include content styles, and stops queueing the content module name in the stylesheet queue.
• When we revert back to wmf.11, the good news is that the old module definitions also naturally come back. The bad news is that we now have some cached articles from during wmf.12 that only refer to base and not to base+content.

What I should have done instead is: Move styles from content to base, but keep queing content as empty style module. This would have been harmless since it is part of the same URL batch and costs 0 bytes extra in the response. That way, all relevant combinations woudl have been fine:

• HTML generated by wmf.11, with stylesheet urls served by wmf.11 backends. (Before, clean)
• HTML generated by wmf.11, with stylesheet urls served by wmf.12 backends. (Old cache)
• HTML generated by wmf.12, with stylesheet urls served by wmf.12 backends. (After, clean)
• HTML generated by wmf.12, with stylesheet urls served by wmf.11 backends. ("Future" cache, while rolled back)

This last one I went wrong. I did think about this during development, and we've done this without issue before. This is exactly why I kept the content definition in non-deprecated form, but I had forgotten to keep queueing it as well.

The hotfix could have been to backport the new base.styles definition to wmf.11 (stylesheet responses are refreshed by CDN within 5 minutes), so that the wmf.12-cached HTML asking for "base" correctly gets the content styles as well. I decided against this because by this time, the train was rolling forward again within the next 10 minutes.

Hi @Mholloway ! Could you provide some input regarding this patch?

Change 700384 merged by jenkins-bot:

[mediawiki/services/mobileapps@master] Minerva should use a single ResourceLoader module for shipping its styles

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

In T266361#7255781, @vadim-kovalenko wrote:

Hi @Mholloway ! Could you provide some input regarding this patch?

Sorry I've been unresponsive on this. It's been a long time since I've looked at this code, and I had to find some time to get back up to speed.

The only module that mobileapps requests "live" from ResourceLoader is site.styles, which is essentially a ResourceLoader wrapper for site styles managed on-wiki (see ResourceLoaderSiteStylesModule.php). As @Krinkle mentions above, mobileapps keeps full local copies of various other modules, including the ones in question in this ticket, which are compiled into a base.css file which is provided via REST API to the apps, and which are updated as needed/convenient. (We had experimented previously with providing "live" versions of all relevant style modules to the apps, but this proved more disruptive than beneficial, so we adjusted to the current setup, which, while unglamorous, appears to have served its purpose adequately for a few years now.)

To make a long story short, I don't think this ticket requires any immediate change to mobileapps. Or am I missing something?

This is on web now per @Mholloway. Untagging apps.

Change 724549 had a related patch set uploaded (by DannyS712; author: DannyS712):

[mediawiki/skins/MinervaNeue@master] Inline single use module skins.minerva.icons.images.scripts

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

^ Not sure if the icon modules are covered by this task, but thats an easy elimination - only used in one place and only contains dependencies

Change 724549 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Inline single use module skins.minerva.icons.images.scripts

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

Change 732306 had a related patch set uploaded (by Vadim Kovalenko; author: Vadim Kovalenko):

[mediawiki/skins/MinervaNeue@master] Move skins.minerva.content.styles into skins.minerva.base.styles

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

Change 732306 merged by jenkins-bot:

[mediawiki/skins/MinervaNeue@master] Move skins.minerva.content.styles into skins.minerva.base.styles

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

@Krinkle would it make sense for https://gerrit.wikimedia.org/r/c/mediawiki/services/mobileapps/+/701574 to do an HTTP request to the production ResourceLoader for /just/ the minerva module, and use that as the basis for the compiled-in styles that mobileapps ships, instead of trying to fetch the various .less files from checked out sources and manually build them?

(That's how standalone Parsoid syncs its siteinfo and sitematrix, via a tools script that fetches these from the action API on the production machines.)

@cscott I think the main part that's important to keep (or at least, the part that's intentional and significant) is for the mobileapps copy to be offline, versioned, and standalone in source.

That could take various shapes:

• copy over files from Gerrit and compile them in a local build script (status quo),
• have the local build script do a quick temporary plain MW core install and commit both the script and its result (e.g. mobileapps in-repo shell script that does git clone --depth 1 + composer install-sqlite + composer serve + add loadSkin Minerva to LocalSettings + make local curl request; Example: QuickMW).
• have the local build script make request against a production wiki and store its result, possibly non-minified by using load.php?…&only=styles&debug=2.

I have no preference for how the apps team handles this.

I do note that, to the best of my knowledge, there are no longer any changes in mobileapps needed within the scope of this task. Whatever we've done so far, is enough afaik and is working fine for them too.

By the way, is there a specific interest from your team in seeing this task progress? I think your suggestion is useful – I'm just making sure I'm not missing something implied that might help me better answer your question.

Change 701574 abandoned by Vadim Kovalenko:

[mediawiki/services/mobileapps@master] Retrieve styles from MinervaNeue extension

Reason:

Patch doesn't provide a robust solution for the ticket because of .less files parsing issue.

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