In T208980#4954700, @phuedx wrote:

@nray: See @Krinkle's detailed explanation here: T208980#4792762

In T211724#4947145, @Jdrewniak wrote:

Following up from yesterdays discussion, one thing we talked about is when it's appropriate to use and event bus and when it's not.

In general, an event bus is best used to create loose coupling between components that have distinct responsibilities (best example is event-logging, where one component is mainly responsible for analytics, while the other is responsible for UI, but we want them to talk to each other, sometimes).

In MF and Minerva however, we sometimes use events to call methods within the same class or subclass (i.e. within the same inheritance chain).

An example of this is how Panel.js emits a show/hide event self.emit( 'show' ); but this event is actually only bound in it's child class of Drawer.js. This pattern makes it difficult to understand where a method is actually executed, because not only do you have to understand the whole inheritance chain, you also need to identify where the method is bound and where in the chain the events are being triggering as well.

I think this kind of behaviour could be replaced with calling class methods directly instead of through an event bus.

IIRC, in some mobile browsers, the unload event doesn't fire before the JavaScript VM is torn down after a tab is closed. However, we're more confident that the visibilitychange event does fire

In T214180#4928062, @Edtadros wrote:

@Niedzielski can you answer a few questions please:

1. Which environment does this need to be tested in?

Before signing off on this I would also like this small patch to be reviewed :)

critical js size (mobile.startup.js + mobile.common.js) has not increased. Beware of any modules that are shared among the lazy loaded chunks and are not in mobile.startup.js. Webpack will excise these into mobile.common.js and increase our critical js size

git checkout e1c212dbe1d583952b2de1eb7ee347108ba68d06 (this patch's commit)
curl -i 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=mobile.startup&skin=minerva' | wc -c
90376

When I briefly looked into this, I thought it was https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/MobileFrontend/+/472368/ that introduced the regression. I couldn't reproduce before that commit

I plan to carve out time to look at this on Monday, Jan. 14

Commit: f4387e9dfa5a55f3af681f0aabd3b17ead464693 (this card's commit)
curl -i -H 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=mobile.startup&skin=minerva' | wc -c
22436

@Jdrewniak Am noticing that after commit 12f24ee7abe0f18af33f2729b0308193e467cbc0, clicking the languages icon no longer shows the white screen loader. Please let me know if you have trouble reproducing

100% code coverage now. Good job all!

git checkout c5a8dfd769ede9b3484a168a57058583ce1d092f
curl -i -H 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=mobile.languages.structured&skin=minerva&version=0hym8m0' | wc -c
2785 bytes

@alexhollender thanks for looking into the text size stuff! Looks like this might not be an issue then

Width - the width of each tab name must fit within the screen for all common screen sizes

In T210210#4829803, @Jdlrobson wrote:

We talked about this today:

• Webpack runtime bundle doesn't seem to be for splitting sharing code
• Webpack does seem to support de-duplicating JS via SplitChunksPlugin
• Many ways to do this seem to exist:
  • could bypass RL and access JS directly
  • We could use webpack async loader
  • ... but beware referencing urls because cache problems (and that will get the attention of the performance team)
    
    We're all lacking in experience in this area so we will probably need to talk again about this when better informed, but the minimum we need to do is make sure that we don't ship code e.g. Overlay twice (inside mobile.startup and mobile.languages) and maintain the same level of bytes. Have added a bullet point to capture that.

Looks like we are done here. Reviewed the rest of our node qunit tests to ensure that the files that are using jquery also call its teardown method in the afterEach hook.

In T210517#4823055, @Jdrewniak wrote:

This is second time I've attached a task to a spike, bad me! (but I didn't want this failing View.test.js to block other patches like T210208).

The patch adds the teadown in Skin.test.js and fixed View.test.js by adding the JSDom setup to that test. JQuery was failing because of no global window object. More specifically, the lack of a window object causes jQuery to export a factory function in Node instead of the proper jQuery class.