Page MenuHomePhabricator

[Spike] How should we organize and manage our ResourceLoader modules?
Open, HighPublic

Description

skin.minerva prefixed modules

We have a pattern in MobileFrontend where we include starter modules such as skins.minerva.backtotop and skins.minerva.newuser that do something on page load. We also have a mega starter module skins.minerva.scripts that does multiple things on page load. What should we do so that we don't have a long list of modules like above? Now that feature flagging is in action, we should consider alternative solutions. Since these scripts don't usually weigh a lot, should we consider loading them in the front end unconditionally and executing them depending on the feature flag. This would also help us clean up SkinMinerva.php that loads modules conditionally, thus reducing cache splits.

Note that on startup we load the following modules:

  • skins.minerva.editor
  • skins.minerva.fontchanger
  • skins.minerva.notifications
  • skins.minerva.scripts
  • skins.minerva.scripts.top
  • skins.minerva.tablet.scripts
  • skins.minerva.talk
  • skins.minerva.toggling
  • skins.minerva.watchstar

mobile. prefixed modules

Currently we load the following non ResourceLoaderImage modules on startup.
Given they are loaded by default it's possible it may be advantageous to package these in a single module (size of startup module/size of resulting JS may be smaller/remove fragmentation of JS files used across pages). We should investigate.

Modules:

  • mobile.betaoptin
  • mobile.editor.api
  • mobile.issues
  • mobile.mainMenu
  • mobile.pagelist.scripts
  • mobile.references
  • mobile.references.gateway
  • mobile.search
  • mobile.search.util
  • mobile.startup
  • mobile.toc
  • mobile.toggle
  • mobile.watchstar

deferring the load of code

(was T131082)
It seems we can make various savings in the JavaScript we load initially by avoiding loading any modules which depend on mobile.overlays at the cost of loading code for search and page issues and disentangling the LoadingOverlay and moduleLoader from the overlay module. This could cut initial JavaScript by 10kb at the cost of a potential delay in using search for users visiting for the first time.

Is there benefit in doing this?

Discussed questions

The following questions were discussed directly/indirectly:

  • Why is the existing architecture the way it is?
  • Does it still make sense?
  • Can skins.minerva.scripts.top and skins.minerva.scripts be merged after recent RL changes? Is there any advantage to having them separate?
  • On what pages does the script url differ (e.g. where do we load different JavaScript based on context.. do we only load toggling code on main namespace - if so what disadvantage does that have on client side caching/load time of subsequent page loads?

Outcome: T167713

Questions to answer:

  • Which modules have side effects (e.g. when loaded make changes to the current page)
  • Which modules are used by 2 or more features?
  • What are the advantages to merging modules?
  • What are the advantages of deferring the loading of code
  • Should modules define features or components?
  • What code is used outside MobileFrontend? Is there justification for sharing it via its own ResourceLoader module?

Related Objects

StatusAssignedTask
OpenNone
OpenNone
ResolvedJdlrobson
ResolvedJdlrobson
StalledNone
ResolvedJdlrobson
Resolvedphuedx
ResolvedJdlrobson
ResolvedJdlrobson
DuplicateNone
OpenNone
OpenNone
ResolvedJdlrobson
StalledNone
ResolvedJhernandez
ResolvedJdrewniak
Resolvedpmiazga
ResolvedJdrewniak
ResolvedJdlrobson
ResolvedJdrewniak

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 16 2016, 4:59 PM
ovasileva triaged this task as Normal priority.Dec 21 2016, 5:14 PM
ovasileva moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.

@bmansurov Do you need any input in particular?

Can you comment on the specific changes you are proposing? That way we can move it to the triaged but future column.

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 Tasks 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 Normal to High.Sep 7 2018, 5:13 PM