Page MenuHomePhabricator

Editor code is built via webpack
Closed, ResolvedPublic5 Story Points

Description

Blockers: https://gerrit.wikimedia.org/r/483200 (mobile.editor.ve module has been removed)

MobileFrontend's JavaScript is currently being migrated to webpack. The benefits of this are numerous, but include eradicating the use of Hogan templates via the use of ES6->5 transpiling, headless Node.js unit tests and easier to write tests, allow for easier refactoring and release versioning. Given the importance and complexity of the editor, we plan to migrate this last. Note, since all editing code lives in MobileFrontend there should be no impact on Minerva with these changes.

Acceptance criteria

  • There will be one JS bundle packaged inside the ResourceLoader mobile.editor.overlay module which is compiled by webpack
  • As is the case currently, code for mobile.editor.overlay is not loaded on the critical path - it is delayed until edit icon is clicked (see also T210210)
  • 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. (there was a slight increase but we are aware of it)
  • tests in tests/qunit/mobile.editor.overlay/ and mobile.editor.api/ are ported to node-qunit and run when npm run test:unit is executed
  • mobile.editor.common and mobile.editor.api modules no longer exist
  • mobile.editor should be ported to webpack or consider merging into mobile.init if that makes sense

Notes

After this change, developers will be expected to run

npm start

to watch and build assets in the repo and commit them to the codebase (note we are looking to remove this requirement as part of the RFC in T199004)

Event Timeline

Jdlrobson created this task.Jan 9 2019, 9:13 PM
Jdlrobson triaged this task as High priority.
Jdlrobson updated the task description. (Show Details)

Heads up @Esanders we're planning on working on this in the latter week of January. Let me know if you have any questions/concerns!

Jdlrobson set the point value for this task to 5.Jan 22 2019, 5:23 PM
Jdlrobson updated the task description. (Show Details)Jan 23 2019, 5:00 PM
Jdlrobson removed Jdlrobson as the assignee of this task.Jan 23 2019, 10:34 PM

free for anyone to work on :)

Change 486349 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] skins.minerva.editor is never loaded

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

Change 486375 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] mobile.editor module merged into mobile.init

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

Change 486349 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] skins.minerva.editor is never loaded

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

Change 486375 merged by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] mobile.editor module merged into mobile.init

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

Change 486531 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Hygiene: Improve EditorGateway tests

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

Change 486535 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Port editor.overlay code to webpack

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

Change 486531 merged by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] Hygiene: Improve EditorGateway tests

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

Change 486535 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Port editor.overlay code to webpack

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

nray claimed this task.Feb 4 2019, 6:05 PM
nray added a comment.EditedFeb 4 2019, 9:05 PM

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

I did noticed a slight increase in critical js size as a result of I0bb7e5f77240748f5dd1d593c0ed1c8f1a79c0a8 :

git checkout d13353879a30c79095c54b904987898650a27f8c (Port editor.overlay code to webpack)
curl -i -H 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=mobile.startup&skin=minerva' | wc -c
26745

git checkout HEAD^1 (Hygiene: Improve EditorGateway tests)
curl -i -H 'Accept-Encoding: gzip' 'http://localhost:8181/w/load.php?debug=false&lang=en&modules=mobile.startup&skin=minerva' | wc -c
26531

After comparing the nonuglified mobile.common.js file in the dist folder, I noticed that webpack is excising mobile.editor.overlay/parseSaveError.js into mobile.common.js because it is being used in both the mobile.editor.overlay and mobile.editor.ve entry points.

@Jdlrobson My main question is should we leave this as is or should we consider moving parseSaveError.js into the mobile.startup folder?

Change 487973 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Fix EditorGateway test throwing an error when run in isolation

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

nray added a comment.Feb 4 2019, 10:39 PM

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

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

nray updated the task description. (Show Details)Feb 4 2019, 10:40 PM
nray removed nray as the assignee of this task.
nray added a subscriber: nray.

Change 487973 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Fix EditorGateway test throwing an error when run in isolation

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

My main question is should we leave this as is or should we consider moving parseSaveError.js into the mobile.startup folder?

Let's leave it be. It's doing exactly what it should be doing. We may want to consider changing the chunk rules for the editor given editor.ve depends on editor.overlay but that should be a new freshly cut task.

nray closed this task as Resolved.Feb 5 2019, 9:35 PM
nray updated the task description. (Show Details)