Page MenuHomePhabricator

[Spike, 8hrs] How do we organize and manage our ResourceLoader modules?
Closed, ResolvedPublic0 Estimated Story Points

Description

Going into desktop refresh we will soon be working on features which will likely need JS and CSS resources. Currently Vector has 3 modules: skins.vector.styles, skins.vector.styles.responsive and skins.vector.js while Minerva has many more. More ResourceLoader modules leads to a larger initial JS payload which can negatively impact performance. Much time has been invested reducing modules in MobileFrontend and Minerva so it would be good to be proactive rather than reactive as we build out Vector.

How should we organize and manage ResourceLoader modules in Vector (and across all our projects, learning from MobileFrontend and Minerva).

Topics to discuss

  • How will we separate styles for new Vector VS old Vector
  • What code do we load on page load?
    • What are the advantages of deferring the loading of code? When is it better not to?
    • What code do we defer the loading of?
  • How we bundle defer code?
    • What are the advantages to merging modules? When is it better not to?
  • Why do we use components folder and resources folder in Minerva
  • Should we follow the components+resources pattern in Vector or revisit this pattern in Minerva?
  • How big are the existing bundles loaded on page load?
  • How can we shrink the size of the existing bundles?
  • What changes in ResourceLoader can be made to reduce the number of modules in Vector learning from the modules we have in MobileFrontend and Minerva?

Output

  • Decide how to organise CSS+JS in Vector
  • Document existing patterns in reading web extensions
  • Talk about it with the team
  • Document agreed best practices (if any) on Reading page on mw.org

Related Objects

StatusSubtypeAssignedTask
ResolvedJdlrobson
ResolvedJdlrobson
ResolvedJdlrobson
ResolvedJdlrobson
DeclinedNone
ResolvedJdlrobson
Resolvedphuedx
ResolvedJdlrobson
ResolvedJdlrobson
DuplicateSpikeNone
ResolvedJdrewniak
DeclinedNone
ResolvedJdlrobson
DeclinedNone
ResolvedJhernandez
ResolvedJdrewniak
Resolvedpmiazga
ResolvedJdrewniak
ResolvedJdlrobson
ResolvedJdrewniak

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Jhernandez reassigned this task from BMacZero to bmansurov.Apr 11 2017, 3:08 PM
Jhernandez added a subscriber: BMacZero.

@bmansurov - we were discussing to turn this into a spike - could you expand on the description with specific questions and then we can move this to triaged but future?

ovasileva renamed this task from How should we manage minerva starter scripts to [Spike] How should we manage minerva starter scripts.Apr 11 2017, 3:09 PM

I'm not sure what else I can add. What needs clarification? This task is a spike. As a result of the spike I hope to see solutions to the problem in the task description.

What should we do so that we don't have a long list of modules like above?

Why is a long list of modules a problem?

Now that feature flagging is in action, we should consider alternative solutions.

Why? How is feature flagging related to this?

This would also help us clean up SkinMinerva.php that loads modules conditionally, thus reducing cache splits.

Could you elaborate on this? I'm not entirely sure I understand the downsides of the current approach. I'm not sure what the concern about "reducing cache splits" is. There's no caching implication of the way we architect our code in this way.

Background

The reason we are keeping them separate is to avoid the client loading the bytes of unnecessary code for extensions.
For instance, if a user is in stable why should they load code from beta (e.g. fontchanger) and if they don't have Echo installed we should probably not serve them (skins.minerva.notifications).

Looking at the modules most of them are appropriately living in their own modules. The only questionable ones that we can and might want to consider merging are the device specific modules; page; unknown (the last 3), but I'm not sure what the advantage of that is (yet).

User specific

only needed when a user is logged in and meets a certain criteria)

  • skins.minerva.newusers
  • skins.minerva.talk (we only show to logged in users)

Beta specific feature modules

Only needed if user is in beta.

  • skins.minerva.backtotop
  • skins.minerva.fontchanger
  • skins.minerva.categories

Style modules

These are style modules. They can be merged with other style modules but in most cases the styles are specifically loaded on the page they are needed to avoid increase in first paint as these are top loaded.

  • skins.minerva.base.reset
  • skins.minerva.base.styles
  • skins.minerva.content.styles
  • skins.minerva.mainPage.styles
  • skins.minerva.print.styles
  • skins.minerva.special.search.styles
  • skins.minerva.special.styles
  • skins.minerva.special.userlogin.styles
  • skins.minerva.special.watchlist.styles
  • skins.minerva.userpage.styles

Extension specific

  • skins.minerva.notifications (only works if Echo is enabled)

ResourceLoaderImage modules

These need to be in separate modules.

  • skins.minerva.icons.images
  • skins.minerva.icons.images.scripts
  • skins.minerva.icons.images.variants
  • skins.minerva.userpage.icons

Device specific

  • skins.minerva.tablet.scripts (only loaded when device is big enough)
  • skins.minerva.tablet.styles

Page specific

These modules are only loaded if the page supports them, but they could probably safely be merged

  • skins.minerva.editor
  • skins.minerva.special.watchlist.scripts
  • skins.minerva.toggling

Unknown

I'm not sure why these 3 exist and cannot be merged.

  • skins.minerva.scripts
  • skins.minerva.scripts.top
  • skins.minerva.watchstar

Why is a long list of modules a problem?

a) Complexity - the more files, the more lines of code, the more bugs. "Code-Complete-Practical-Handbook-Construction" by Steve McConnell has empirical evidence. This blog post also backs up the claim with data.
b) Performance - Wrapper functions, multiple jQuery document ready functions has performance issues. Here is a simple case I came up with. Hit "Run tests" to see the results. I'm attaching the screenshot of my test below:

Now that feature flagging is in action, we should consider alternative solutions.

Why? How is feature flagging related to this?

In order to remove clutter from extension.json. We have code like this, see how much boilerplate code we have.

"skins.minerva.special.watchlist.scripts": {
	"targets": "mobile",
	"dependencies": [
		"mobile.startup",
		"mobile.watchlist"
	],
	"scripts": [
		"resources/skins.minerva.special.watchlist.scripts/watchlist.js"
	]
},

If we include the contents of watchlist.js and control the behavior of the file using feature flags, we'll be able to get rid of this kind of code from extension.json.

This would also help us clean up SkinMinerva.php that loads modules conditionally, thus reducing cache splits.

Could you elaborate on this? I'm not entirely sure I understand the downsides of the current approach. I'm not sure what the concern about "reducing cache splits" is. There's no caching implication of the way we architect our code in this way.

See how modules are being added in here, for example? We can avoid this kind of code using feature flags and lazy loading modules. I can't remember why I mentioned cache splits, but I'll have to do some more research to find out.

Background

The reason we are keeping them separate is to avoid the client loading the bytes of unnecessary code for extensions.
For instance, if a user is in stable why should they load code from beta (e.g. fontchanger) and if they don't have Echo installed we should probably not serve them (skins.minerva.notifications).

I think we still can achieve this without having so many starter scripts. We can shift the logic from these scripts to their appropriate modules. For example, rather than having both the mobile.fontchanger and skins.minerva.fontchanger modules, we can move the contents of the former to the latter and only execute that piece of code in certain situations, like on page load, and not when testing the module. The function that needs to be executed can register itself to be executed and some other function executes such functions.

As for style modules, there is no need to create a module for a specific piece of functionality. For example, the first seven modules in getSkinStyles don't have to be separate modules. They can be separate files, sure, but why modules?

Jdlrobson renamed this task from [Spike] How should we manage minerva starter scripts to [Spike] How should we organize and manage our ResourceLoader modules?.May 21 2017, 10:16 AM
Jdlrobson updated the task description. (Show Details)

^ Since there's a lot of overlap with these tasks I've merged them into one.
It seems like this will benefit from a group conversation.
Thanks for your input @bmansurov

Suggested agenda:

  • Document why modules are split up
    • I believe some of the styles are done this way for apps
    • Some relate to how we do feature flagging
    • Some relate to avoiding unnecessarily shipping code that doesn't do anything
    • Third party support is another consideration (we support Minerva skin on repos where Echo is not installed)
  • Document why it is bad to split them up
  • Talk about trade offs - If we merge all the code into one file we bloat the js of all pages unnecessarily so this is a trade off.
Jdlrobson updated the task description. (Show Details)Jun 15 2017, 5:54 PM
bmansurov removed bmansurov as the assignee of this task.Jul 5 2017, 7:55 PM
Jdlrobson moved this task from Backlog to Team:Growth on the MobileFrontend board.Jul 13 2017, 5:54 PM

We talked about this https://etherpad.wikimedia.org/p/readingwebfrontendcode
Conversation is still ongoing.. I think?
but we agreed on doing T167713

Mholloway added a subscriber: Mholloway.

This came up again today, during our code review session when we looked at Skin.js. In particular, we found the way we conditionally add scripts for tablet mode (which is just to add table of contents) strange and unnecessarily complicated. Something that merging modules would fix.

I personally don't see a problem with conditionally loading tablet modules. Optimizing for the constrained mobile experience seems like the best idea, and there are certain devices that when rotating from portrait to landscape become tablet, so loading tablet modules on resize is a great idea. What we don't do right now that we should is disable the tablet enhancements when the screen is resized back to mobile again.

The code is pretty tiny and non-render blocking. I can't help but think the additional JS request (especially given it is cached locally) is not worth the hassle.

Krinkle added a subscriber: Krinkle.
Jdlrobson raised the priority of this task from Medium to High.Sep 7 2018, 5:13 PM
Restricted Application added a subscriber: Masumrezarock100. · View Herald TranscriptJan 17 2020, 10:21 PM

We never answered this in the MobileFrotnend architecture project but seems even more important now as we build out Vector.

Jdlrobson moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.
Jdlrobson updated the task description. (Show Details)Feb 3 2020, 8:45 AM
Jdlrobson renamed this task from [Spike] How should we organize and manage our ResourceLoader modules? to [Spike] How do we organize and manage our ResourceLoader modules?.Feb 3 2020, 8:53 AM
Jdlrobson updated the task description. (Show Details)

This spike is ready to work on. It however involves organizing meetings for the whole team.

Jdlrobson updated the task description. (Show Details)Feb 26 2020, 5:52 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson renamed this task from [Spike] How do we organize and manage our ResourceLoader modules? to [Spike, 8hrs] How do we organize and manage our ResourceLoader modules?.Feb 26 2020, 5:54 PM
Jdlrobson set the point value for this task to 0.
Jdlrobson removed ovasileva as the assignee of this task.Mar 18 2020, 5:27 PM

Ahhh I wrote a long essay but it doesn't look like it got posted and is no longer here in draft. I guess I'll have to write that again and hope i'm just as thorough!

Jdlrobson added a subscriber: Stephen.

tldr: Finding support for bundlesizes T244276 is essential. Keep the baseline and talk more as we deviate from it. Keep to the docs and add missing docs (T248718). Cleanup legacy code T127268 and improve ResourceLoader (T248912).

How will we separate styles for new Vector VS old Vector

We have 2 ResourceLoader entry points - https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/Vector/+/579075/
As we create/modify components we should fork the modules by renaming components e.g. LegacyLogo.less and Logo.less with the latter included in new Vector.

Why do we use components folder and resources folder in Minerva

Should we follow the components+resources pattern in Vector or revisit this pattern in Minerva?

@Stephen when the components folder was setup was it to keep the newer code separate from older code or for another reason? Is there any reason we could not keep that folder inside the resources folder with a name such as minerva.components or inside resources/skins.minerva/components to achieve the same goal? Having 2 folders components and resources is a little confusing to me and it's not clear what should live where. A README.md would help here.

It has also created problems with double loading code (e.g. I6d33a83f20a20956427f261f1900f3cfd8e0153b) as it breaks the intention with the resources folder directory structure to isolate code to single ResourceLoader modules. Files in resources have no choice but to import LESS files from the components folder by importing from outside what is meant to be an isolated directory) (e.g. '../../components/foo.less')

Inside Vector I would propose a resources/vector.components or resources/skins.vector.styles/components for the component library. Given we are eventually hoping to build a shared component library I think doing this will keep the code contained and Vector's component needs well documented.

I think we should strive to align with https://www.mediawiki.org/wiki/ResourceLoader/Developing_with_ResourceLoader#Registering and modify that documentation with any additional thoughts we have.

In MobileFrontend the use of webpack and a src folder and spitting out files to resources obeys the rules of this documentation while allowing more flexibility on code layout as it separates the building of code from the shipping of code.

> What code do we load on page load?

Right now the Vector skin itself that we run in production is very simple. It has one JS module - skins.vector.js and one CSS module (either skins.vector.styles or skins.vector.styles.legacy. However there is lots of code loaded by core itself via the module mediawiki.page.ready

This core code loads various modules separately based on detecting HTML in the page. For example the existing search code is loaded if the element ID 'searchInput' is detected in the HTML (this will create challenges for us if we are rewriting this in Vue).

What are the advantages of deferring the loading of code? When is it better not to?
>      What code do we defer the loading of?
>  How we bundle defer code?
  >    What are the advantages to merging modules? When is it better not to?

I think the existing mobile site shows some examples of where loading code can be good and not so good. Clicking the editor or the Echo notification leads to a big delay while expensive libraries are pulled down making the feature not so responsive. All special pages load their own JS modules which keeps code that will never need to be run on article pages out of the main CSS bundles. I think we should continue to do that.

On the other hand talk and category features are loaded asynchronously and feel far more responsive given a basic Overlay component is loaded in initial payload.
I think there are trade offs to consider here.

In MobileFrontend I think we have too many modules and our mobile.startup module has become bloated (14.5kb mobile.common.js VS 5.0KB mobile.startup.js) with too many components that are not needed on startup. While a 9.0KB mobile.editor.overlay.js bundle seems justified it seems less justified mobile.mediaViewer.js (2.5kb), mobile.languages.structured.js (1.9kb), mobile.categories.overlays.js (2.5kb) and mobile.talk.overlays.js (2.5kb)

However as an experiment I explored what a single lazy loaded entry point would look like - merging mobile.languages.structured, mobile.mediaViewer.js, mobile.talk.overlays.js and mobile.categories.overlays.js. This led to little to no shrink in mobile.startup but a new 14.56KB module and noticeably less responsive UI.

I recommend using bundlesize (T244276) to monitor in Vector so we can judge when to split. 9KB of asynchronously loaded JS seems about the maximum bundlesize we should look to lazy load.

However more concerning is the existing experiences that plug into Vector e.g. ULS.
For example right now ULS should be loaded on demand rather than upfront.

I had a chat @Krinkle on this subject who had this to say:

In general, nothing should load on the page, unless it has to do something on that page and that it has to do it immediately when the page loads.

I've requested some documentation to encapsulate this conversation on wiki (T248718)

How big are the existing bundles loaded on page load?
How can we shrink the size of the existing bundles?

On Minerva mobile CSS is 9.38kb contributing to first paint
On desktop Minerva (with AMC) it jumps to 12.03kb.
On Vector CSS is 9.01kb however editor gadgets and styles make it much larger.
Ignoring those for the time being, 9-10kb of CSS seems like a good baseline. We should monitor bundle sizes (T244276) as we know from experience that an increase in CSS leads to an increased delay in first paint.

With regards to JS, with an empty cache (localStorage and disable cache mode) an anonymous user loads 176kb of JS on mobile after gzip compared to 209kb after gzip on desktop. The bulk of this code lives outside the Vector repository.

To shrink existing bundles our best approach is outlined in T127268 - which would reduce JS for mobile and desktop by encouraging best practices in legacy extensions. ULS (T237036, T237061) being the most impactful.

In terms of newly introduced code, it's unclear how to go about that without knowing where the code lives however keeping an eye on that seems like the most important thing we can do.

What changes in ResourceLoader can be made to reduce the number of modules in Vector learning from the modules we have in MobileFrontend and Minerva?

MobileFrontend and Minerva have a large amount of ResourceLoaderImage modules. New modules are created because of lack of standardisation but various modules could be shipped as part of a ResourceLoaderFileModule. I've recommended T248912 as something concrete we can do to help that.

Jhernandez removed a subscriber: Jhernandez.Apr 2 2020, 6:46 PM
ovasileva lowered the priority of this task from High to Medium.Apr 27 2020, 5:10 PM
nray added a subscriber: nray.May 13 2020, 11:44 PM

The very thoughtful writeup from @Jdlrobson makes sense to me although some of the recommendations (e.g. Merge ResourceLoaderFileModule and ResourceLoaderImageModule (or make latter a mixin)) I can't really speak to since my knowledge of those topics is pretty limited right now. Overall:

  • I'm all for adding bundlesizes/bandwidth tests for css/js in Vector and being cognizant of the 9 kb CSS / 209 kb JS baseline
  • I think the Provide performance guidelines on when and how to load code ticket is great and contains multiple interesting topics with valuable insight (and more to come I'm sure)
  • Some of the organizational questions have been answered since this writeup, some conventions remain tbd in T249073, and some like the components vs resources question I don't have a strong opinion on as I'm still trying to figure out what the pros/cons/motivations associated with each method are (apart from the cons mentions about the components approach in the writeup).
Jdlrobson moved this task from Needs triage to Technical on the Vector board.May 15 2020, 4:59 PM
Jdrewniak claimed this task.Jun 3 2020, 5:09 PM
Jdrewniak closed this task as Resolved.Jun 8 2020, 12:47 PM