Page MenuHomePhabricator

[EPIC] MinervaNeue's starter modules should be grouped by page not feature
Closed, DeclinedPublic8 Estimated Story Points

Description

Note: as part of this change I recommend adopting webpack in Minerva.

Currently MobileFrontend(Minerva) has various starter modules.

They are arranged by feature or page.
For instance if the user cannot watch a certain page we do not load the module skins.minerva.watchstar
After discussing this as a team we concluded that we would prefer to group these by page and move this logic into frontend code. This will mean more JS is loaded by default, but there will be less fragmentation across pages.

To start with the following modules will be merged into a single starter module skins.minerva.scripts which will be renamed skins.minerva.page

A/C

Folder restructure

  • Merge the following modules into one single entry module:
  • skins.minerva.scripts
  • skins.minerva.scripts.top
  • skins.minerva.tablet.scripts
  • skins.minerva.notifications
  • skins.minerva.newusers
  • skins.minerva.fontchanger
  • skins.minerva.editor
  • skins.minerva.categories
  • skins.minerva.backtotop
  • skins.minerva.talk
  • skins.minerva.toggling
  • skins.minerva.watchstar

Use this script to ensure we do not break the experience for cached HTML

Loading the new stable features code

  • Add skins.minerva.page to any page which is not a SpecialPage.
  • Update the directory structure so all files are listed under skins.minerva.page (rename any init files)
  • The logic inside getContextSpecificModules should be moved from the server side to the client.
  • Inside SkinMinerva->getContextSpecificModules we check if a page is watchable with SkinMinerva->isAllowedPageAction. Move that check to the client - into the file that was previously skins.minerva.watchstar/init.js
  • Inside SkinMinerva->getContextSpecificModules we check if a page has Echo installed and the user is logged in. Move that check to the client - into the file that was previously skins.minerva.notifications/init.js
  • Inside SkinMinerva->getContextSpecificModules we check query string parameters and load the newusers module if necessary. Move that check to the client - into the file that was skins.minerva.newusers/init.js
  • Inside SkinMinerva->getContextSpecificModules we only load talk page code if a page allows talking and the page is wikitext.

More complicated module moves

  • Toggling code is loaded inside SkinMinerva->getDefaultModules.
  • Inside SkinMinerva->getContextSpecificModules we check if a page is editable with SkinMinerva->isCurrentPageContentModelEditable. Move that check to the client - into the file that was previously skins.minerva.editor/init.js
  • Inside SkinMinerva->getContextSpecificModules we load back top, font changer or categories if enabled. Now we must rely on client side code - add config variables inside SkinMinerva::getSkinConfigVariables and move the logic for these checks into the corresponding client side code.

Test plan

  • The entry points should load without errors under the following circumstances
  • MobileFormatter is not available e.g. useformat=desktop&useskin=minerva (impacts toggling)
  • A feature is disabled (for example...):
    • if categories is disabled the categories code should not work
    • if talk is disabled the talk code should not work
    • if back to top is disabled back to top should not work

Developer notes

  • Rename skins.minerva.scripts to skins.minerva.page and make sure an alias is left (with single dependency) to deal with cached HTML.
  • Any entry point that loads in stable/anon must have an alias to avoid JS errors in cached HTML
  • Document any increase in JS bytes before merging.
  • Please take this opportunity to follow up with patchsets that shift more logic into the generic modules e.g. mobile.toggling, provided that they remain testable and without side effect~ don't do this. Keep scope small.

Open questions

  • How do we check content model on the client? Is there any existing config option or do we need to surface our own?
  • Can we detect whether content has been mobile formatted on the client when deciding to run the toggling code?
  • Is not grouping by feature ever a bad thing?
  • Is a tablet page different from a normal page?
  • What are the benefits of grouping feature (if any)?
  • What implications does this have for code that we load via mw.loader.using from MobileFrontend ? Any?

Sign off checklist

  • How did this impact performance in particular first paint / bytes shipped
  • Create cleanup task to remove aliases in a week
  • Update status of T173454

Related Objects

StatusSubtypeAssignedTask
ResolvedJdlrobson
ResolvedJdlrobson
ResolvedJdlrobson
ResolvedJdlrobson
DeclinedNone
ResolvedJdlrobson
Resolvedphuedx
ResolvedJdlrobson
ResolvedJdlrobson
DuplicateSpikeNone
ResolvedJdrewniak
DeclinedNone
ResolvedJdlrobson
DeclinedNone
Resolved Jhernandez
ResolvedJdrewniak
Resolvedpmiazga
ResolvedJdrewniak
ResolvedJdlrobson
ResolvedJdrewniak

Event Timeline

Jdlrobson updated the task description. (Show Details)

@bmansurov @Jhernandez @pmiazga did I capture our conversation well? Ready for dev?

Isn't this too big to be just one task?

Merge the following modules into one startup module:

I think we should split those modules between the startup module and their corresponding modules. For example, split skins.minerva.toggling into two and add relevant bits to skins.minerva.scripts, and the other bits to mobile.toggle. The goal is to keep skins.minerva.scripts lean and keep all mobile.toggle related stuff in that module. So, the lines 11 - 31 would be moved to mobile.toggle and the rest would be moved to skins.minerva.scripts while adding an option for lazy loading the mobile.toggle code.

Isn't this too big to be just one task?

Not in my opinion. What looks complicated to you? This should just be a case of merging extension.json dependencies and moving files into one folder.

So, the lines 11 - 31 would be moved to mobile.toggle and the rest would be moved to skins.minerva.scripts while adding an option for lazy loading the mobile.toggle code.

Sure, but to start with let's just get them merged. We can optimise/clean up after wards. Then we can work out which bits can be pulled out. mobile.toggle is not lazy loaded and I don't see any advantages to do that right now. I think it's also fine as part of this task to take the opportunity to do that hygiene. I've updated the description to be less restrictive. Hope that works.

Estimates ranged from 3 to 8 points. There were concerns that this was a scary change and a little open ended but we couldn't reach a true consensus on quantifying that. We'll try again asynchronously.

We'll try again asynchronously.

Want to try this synchronously after today's standup meeting?

Jdlrobson updated the task description. (Show Details)

We tried again today, but we're still a bit stuck here with estimating. There's still concern this is large and should be split up. I honestly don't think it should, and this might benefit from a patch showing how trivial the change is or is not to help with estimation.

Estimation was a spread again. @Jdlrobson feels confident about the task, may or may not explain it for others asynch. Otherwise, it's Needs Analysis until the devs can chat.

I'm fine with the task as it is, if we agree on points. We can have big tasks.

And if it is smaller than what the team agreed, then all the better!

Just for the record, I don't think I was proposing to split up the task. I said we need to be vigilant and not rush a half-baked solution. The proposed solution touches almost all aspects of the user interface and I'd like to take the time necessary to make sure that we're not breaking anything in the process.

So I had a stab at this to check the complexity ^^^. It's definitely not a 3 and it does seem there is some hidden complexity in here. For instance, I noticed that after attempting the move, the table of contents code was being loaded on Special:Nearby on tablets and throwing a JS exception

Everyone should take a look at the patch and let's have another go at estimating. Hopefully we'll have a shared understanding now :)

Change 363441 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] WIP: Merge MobileFrontend's starter modules

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

Change 363441 abandoned by Jdlrobson:
WIP: Merge MobileFrontend's starter modules

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

Jdlrobson set the point value for this task to 8.Jul 11 2017, 4:52 PM

We decided this does have a lot of hidden complexity and finally estimated consistently :)

Jdlrobson renamed this task from MobileFrontend's starter modules should we grouped by page not feature to MinervaNeue's starter modules should we grouped by page not feature.Jul 13 2017, 5:44 PM
Jdlrobson edited projects, added MinervaNeue; removed MobileFrontend.

Change 381098 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Load tablet device code on mobile devices

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

Change 381098 abandoned by Jdlrobson:
Load tablet device code on mobile devices

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

Jdlrobson renamed this task from MinervaNeue's starter modules should we grouped by page not feature to MinervaNeue's starter modules should be grouped by page not feature.Nov 2 2017, 5:47 PM
Jdlrobson renamed this task from MinervaNeue's starter modules should be grouped by page not feature to [EPIC] MinervaNeue's starter modules should be grouped by page not feature.Mar 26 2018, 6:01 PM
Jdlrobson added a project: Epic.

I'm closing this out as it doesn't seem relevant now. The majority of Minerva's modules now are icon packs or style modules.
Only two starter modules contain JS - skins.minerva.options and skins.minerva.scripts. The former module is loaded only in beta/AMC mode to load the categories feature.

I think Minerva's big issue now is how it handles CSS and icons. There is a lot of room for improvement there but that's not in scope for this particular task.