Page MenuHomePhabricator

Bundle Hogan.js
Closed, ResolvedPublic3 Story Points

Description

This task encompasses the work to Webpackify the mediawikit.template.hogan module across two patches:

  • Split out the mobile.runtime build product to a distinct module
  • Pack mediawikit.template.hogan with Hogan v2.0.0. Unlike other modules, the new module should not export Hogan itself since its side-effect only and all usages are via mw.template, and doing so would inhibit Webpack's ability to optimize outputs.

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/MobileFrontend/+/461470

Precursors

[X ] Per developer notes, I'd like to hold off making this change until we've done T206027 since I'd like us to keep reducing the amount of RL modules
Edit: no new modules introduced so we can ignore this.
[ x] Let's also get T205582 done given risk of errors

Developer notes

The introduction of any module to MobileFrontend will be met with suspicion by the performance team. Ideally any change here should not increase the number of ResourceLoader modules (or should be done at a time when MobileFrontend has far less than the current 60).

AC

  • Diff the NPM 2.0.0 (unminified) release with libs/hogan.js/hogan.js to verify the version is correct.

Related Objects

StatusAssignedTask
OpenNone
OpenNone
ResolvedJdlrobson
OpenNone
ResolvedJdlrobson
ResolvedJdlrobson
ResolvedJdlrobson
ResolvedJdlrobson
ResolvedJdrewniak
Resolvedpmiazga
ResolvedJdlrobson
ResolvedNone
ResolvedNone
ResolvedJdlrobson
ResolvedJdrewniak
ResolvedJdlrobson
ResolvedNone
ResolvedNone
Resolvedphuedx
ResolvedJdlrobson
ResolvedJdrewniak
ResolvedJdlrobson
OpenNone
OpenNone
StalledNone
DeclinedNone
OpenNone
OpenNone
ResolvedKrinkle
Resolvedovasileva
ResolvedJdlrobson
OpenNone
ResolvedNone
ResolvedNone
Resolved Niedzielski
Resolvedovasileva
OpenJdlrobson
ResolvedJdlrobson
Resolvedphuedx
ResolvedJdlrobson
Resolvedovasileva
DeclinedJdlrobson
Resolvednray
StalledNone
ResolvedNone
ResolvedNone
OpenNone
OpenNone
OpenNone
DuplicateNone
OpenNone
Resolvedpmiazga
ResolvedJdlrobson
ResolvedJdlrobson
DeclinedNone
DuplicateReedy
Resolved Niedzielski
Resolved Niedzielski
OpenNone
Resolved Niedzielski

Event Timeline

Restricted Application changed the subtype of this task from "Deadline" to "Task". · View Herald TranscriptSep 21 2018, 6:09 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
ovasileva set the point value for this task to 3.Sep 25 2018, 4:48 PM

@ovasileva, @Jdlrobson, please consider this task for the board. It's small-ish, independent of other tasks, introduces a second production entry to our Webpack config, and has a tiny but immediate performance benefit to users. The change itself is low-risk and comes with a POC but we need to identify how to handle sharing the Webpack runtime module best.

Jdlrobson updated the task description. (Show Details)Oct 3 2018, 8:46 PM

I've recently run into the question of how to compile templates for unit-tests (for T206226 ). In a Node environment such as node-qunit, we have to import the templates somehow, which led me to discover the mustache-loader for webpack (which uses Hogan under the hood). Have we considered using a loader like this for templates?

I've recently run into the question of how to compile templates for unit-tests (for T206226 ). In a Node environment such as node-qunit, we have to import the templates somehow, which led me to discover the mustache-loader for webpack (which uses Hogan under the hood). Have we considered using a loader like this for templates?

Sort of : https://phabricator.wikimedia.org/T94086
The challenge is QuickSurveys uses hogan, so we need to make sure we don't break that and don't ever ship two versions of Hogan. Provided we use the templates in the tests only, I think that should be fine and could provide us with a future upgrade path.

What problems do we hope mustache-loader will solve? Is it just a performance and dependency thing? If it's the former, I'd like to understand if we plan to keep Mustache templates in MobileFrontend for the foreseeable future. We've migrated away from them in Popups. I didn't seem to have much problem with the latter in 104322ed3. I understand there are other Hogan.js consumers but it seems like a lower priority to me if we're not planning to use templates in MobileFrontend. I'm not opposed to Mustache necessarily but I do hope to avoid polishing what we won't actually use and I thought we were previously considering migrating away from templates.

Definitely a conversation for later. In the mean time, using a shared version of hogan via npm inside tests and mediawiki.template.hogan seems sensible.

Change 467772 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Upgrade to Hogan 2.0.0

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

I'm curious if and how this relates to T194684. I understand that the switch away from HTML templates and its feature set toward simple string templates is deferred to later, but a fairly significant investment in Hogan is still surprising.

If there is resourcing available for this, would you be interested in also switching to Mustache while at it? Based on previous estimation, the switch is expected to be trivial in terms of complexity (we know they're mostly compatible and MF uses a subset supported by both, and differences are known and can be ported as needed).

Continued investment in Hogan raises a number of risk factors (security, performance, stability support). It also makes code execution potentially incompatibility between mobile and desktop (T158181, T127268), which incurs added maintenance and testing overhead on other teams, as well subsequent workarounds or bugfixes as expected. Is this worth it? If so, I'd like to collaborate on a simple, quick but solid analysis of the perceived performance benefit this brings to mobile and whether it still holds up, is significant, or can be easily bought back through other means.

Not so long ago, Mustache was seemingly 0.5KB smaller than Hogan, but even if's changed again in Hogan's favour, we should know what the trade off looks like, because transfer size isn't everything.

Jdlrobson added a comment.EditedOct 16 2018, 9:22 PM

I'm curious if and how this relates to T194684. I understand that the switch away from HTML templates and its feature set toward simple string templates is deferred to later, but a fairly significant investment in Hogan is still surprising.

We are simply moving Hogan from a lib folder to a pinned package.json version (we already use 2.0.0). We are not updating it. I don't think this is a significant investment. What am I missing?

Edit: I see my commit message was misleading and said "upgrade". Have updated that.

If there is resourcing available for this, would you be interested in also switching to Mustache while at it? Based on previous estimation, the switch is expected to be trivial in terms of complexity (we know they're mostly compatible and MF uses a subset supported by both, and differences are known and can be ported as needed).

I've looked into this and it's not trivial (it seems there are some differences between how Hogan/Mustache treat blocks). We have resourcing available, but we'll be evaluating this early next year. This is a stepping stone towards being able to re-evaluate this code. There are a lot of things we need to consider first (one being whether we want to continue making use of templates at all).

Okay, sounds good. It's unfortunate that the differences are that significant, but nothing we can do about that now. Thanks for explaining.

Jdlrobson updated the task description. (Show Details)Oct 18 2018, 10:17 PM

Change 467772 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Use npm for Hogan 2.0.0

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

Stephen could you please sign this off?

Diff the NPM 2.0.0 (unminified) release with libs/hogan.js/hogan.js to verify the version is correct.

diff node_modules/hogan.js/web/builds/2.0.0/hogan-2.0.0.js libs/hogan.js/hogan.js.js checks out.

Split out the mobile.runtime build product to a distinct module

I believe this piece is missing.

The source map, mediawiki.template.hogan.js.map.json, appears to be missing from master!

I like how you've broken out the compiler registration but it highlights that the remaining code doesn't do much:

var Hogan = require( 'hogan.js' );

/**
 * Hogan template compiler
 * @class hogan
 */
module.exports = {
	/**
	 * Compiler source code into a template object
	 *
	 * @memberof hogan
	 * @instance
	 * @param {string} src the source of a template
	 * @return {Hogan.Template} template object
	 */
	compile: function ( src ) {
		return Hogan.compile( src );
	}
};

Did you consider moving Hogan.compile.bind(Hogan) where needed instead? It'd let us drop the file and other requires, in the tests, will be slightly clearer.

Niedzielski removed Niedzielski as the assignee of this task.Oct 19 2018, 7:12 PM
Niedzielski claimed this task.
Niedzielski removed Niedzielski as the assignee of this task.

Change 469151 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Add missing Hogan build product (source map)

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

Change 469152 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Simplify the templating code

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

Change 469153 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Move the runtime to mediawiki.template.hogan

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

Change 469151 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Add missing Hogan build product (source map)

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

Change 469153 merged by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] Move the runtime to mediawiki.template.hogan

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

all pactches are merged, is it ready for QA ? @Jdlrobson @Niedzielski

Change 469152 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Simplify the templating code

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

This can skip qa @pmiazga.

Im pretty confident about selenium/qunit catching major regressions and we have a lot of other tasks which are in qa that i suspect would uncover any issues here if they exist (if so we reopen and address).

In an ideal world we would do general qa on every webpack change, but i dont think we are in that world and i cannot justify unnecessarily overwhelming Rummana with all these webpack changes since she's already got quite a bit on her plate.

Niedzielski closed this task as Resolved.Oct 24 2018, 5:13 PM
Niedzielski claimed this task.

LGTM