Page MenuHomePhabricator

Technical: Use bundler to generate Popups JavaScript code
Closed, ResolvedPublic13 Story Points

Description

I'm curious if we're open to using webpack in the Popups extension. While reviewing https://gerrit.wikimedia.org/r/#/c/333320/2 I felt that it's a shame we use mw.popup in so many places and now have so many JS files listed in a ResourceLoader module. Using webpack would make development much easier and nicer.

It also seems like it may help with our use of Mustache templates - we could compile these rather than pull in the Mustache library and the templates (note currently Wikipedia doesn't load Mustache on pageload which would happen once Popups goes live).

Benefits:

  • Reduce need for mw.popup global
  • require files rather than rely on RL
  • expected optimisations to size of code
  • Fix T147306

Cons

  • adds a compilation phase:

Suggested switchover strategy

  • Create folder build_resources with folder ext.popups
  • Generate ext.popups/reducers using bundler generated from build_resources that assumes mw.popups global and assigns to mw.popups.reducers (ext.popups script RL definition should now be one file for reducers.js)
  • Generate changeListeners using bundler generated from build_resources that assumes mw.popups global and assigns to mw.popups.changeListeners (ext.popups script RL definition should now be one file for changeListeners.js)
  • Generate preview and top level JS so that it is bundled from build_resources (ext.popups RL script definition should now be one file)
  • Add developer documentation
  • Add CI check so that built assets have to be up to date
  • Get rid of IIFEs
  • Migrate QUnit unit browser tests to run in qunit node with common JS and remove the exposed keys on the global object
  • Use webpack 2 instead of the outdated v1

De-scoped

See T160059: Popups technical improvements related to frontend assets

  • Precompile templates
  • Use uglifyJS on bundling
  • Improve bundled-assets-in-repo workflow

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
phuedx raised the priority of this task from Normal to High.Feb 2 2017, 12:41 PM
phuedx moved this task from Triaged but Future to Upcoming on the Readers-Web-Backlog board.

Change 335591 had a related patch set uploaded (by Jdlrobson):
webpack: Complete transition to webpack for ext.popups module

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

Change 335723 had a related patch set uploaded (by Jdlrobson):
Redux and redux-thunk now packaged inside ext.popups

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

phuedx added a comment.EditedFeb 3 2017, 9:38 AM

The name of the folder can easily be changed yes I agree.

What about the other two points I raised in T156333#2975423? I see that you've dropped the "Hygiene" tag. Thanks!

I did all 4 points.. switched to a build folder, dropped hygiene, wrote an adr (did I do that correctly) and added source map support: Tooling: Begin to use webpack for JS code generation what did I miss?

OOjs UI uses its own build process to generate its JS for bundling inside and outside ResourceLoader. I don't see how this is any different nor the relationship to ResourceLoader. We're still using it, just simplifying the delivery by delegating the concatenation of files and dependency management to a modern widely used (for good reason) build tool.

ResourceLoader already provides concatenation of files and dependency management. While for non-MediaWiki developers it could be easier to understand your development workflow, for MediaWiki developers it could be actually harder. I wonder what the environment would look like if each and every extension used its own build process...

Change 336877 had a related patch set uploaded (by Jdlrobson):
webpack: Build gateway via webpack

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

@Ricordisamoa sure but you don't need to use ResourceLoader that way :) There are various examples of JS packaging/building across Wikimedia extensions - OOjs, Wikidata ..there's building going on all over our extensions.

I see this as complementing ResourceLoader not replacing it. You can also save yourself issues with dependency management by allowing webpack or some other bundler to do that - thus avoiding sending errors to real users or sending unused JavaScript/CSS which RL requires you to manually control. Concatenation of files also comes with a cost - if you've ever used Vagrant you may notice locally on NTFS file systems packaging up ResourceLoader modules can be slow - as it's limited to the performance of file operations on the box.

I'm mostly curious about your concern "for MediaWiki developers it could be actually harder."
We already rely heavily on npm - pretty much every repository uses npm to enforce code standards via eslint/jshint/jscs for example. A MediaWiki developer needs to understand how to run npm install and npm test to submit code. Thus adding a build step doesn't seem too much of a stretch from there. Is it webpack you are uncomfortable with or the general idea? Although it might be a minor inconvenience, I'm fairly confident when MediaWiki developers not familiar with webpack will quickly see how better their development process becomes.

There's also some discussion over on T143463 about using a bundler in core so I think this is something that could become more common in our repos.

Change 334430 merged by jenkins-bot:
Tooling: Begin to use webpack for JS code generation

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

Change 334431 merged by jenkins-bot:
Hygiene: Reducers packaged via webpack

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

Change 336877 merged by jenkins-bot:
webpack: Build gateway via webpack

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

Change 335591 merged by jenkins-bot:
Webpack: Complete transition to webpack for ext.popups module

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

Change 335723 merged by jenkins-bot:
Redux and redux-thunk now packaged inside ext.popups

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

Jhernandez updated the task description. (Show Details)Feb 13 2017, 5:52 PM
Jhernandez updated the task description. (Show Details)

Change 337581 had a related patch set uploaded (by Phuedx):
Add missing ext.eventLogging.Schema dependency

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

I'm not sure that the above is strictly related to this task but it became apparent after @Jdlrobson's changes were merged…

phuedx added a comment.EditedFeb 14 2017, 2:30 PM

There's a missing step from the strategy: remove the IIFE's from all of the source files. In scope?

Change 337581 merged by jenkins-bot:
Add missing ext.eventLogging.Schema dependency

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

Jhernandez updated the task description. (Show Details)Feb 15 2017, 6:06 PM

I've updated the TODOs

  • Get rid of IIFEs
  • Migrate QUnit unit browser tests to run in qunit node with common JS and remove the exposed keys on the global object
  • Migrate templates

I've WIP for the first two

IIFEs mergerd, qunit tests in node up for review

@Jhernandez one of the tests in the last patch set is failing locally only.

@bmansurov I've submitted an updated version that doesn't use sinon fake timers, which were causing problems.

Also if you can npm update mw-node-qunit since there are some changes to that runner that may be the cause. npm view mw-node-qunit version should give you 1.0.6. I'll submit a patch updating the min version on our package.json.

Here are the changes per version

1.0.6
Add globals to window too
1.0.5
Don't reorder or run tests in parallel
1.0.4
Don't autostart the tests, they are manually started
1.0.3
Invoke sandbox restore after hooks have been called
1.0.2
Default fake timers and server to false

Specifically 1.0.5 and 1.0.2 may be causing differences in the test runs.

I've added a patch bumping up the version of the test runner, and submitted another batch of migrations.

https://gerrit.wikimedia.org/r/#/q/status:open+project:mediawiki/extensions/Popups+branch:master+topic:qunit-tests-in-node

@Jhernandez some unrelated php error is preventing merging some of the patches. Feel free to self-merge the ones I've reviewed if the error goes away by the time you read this message.

The issue is tracked here I think T117710#3047699

hashar added a subscriber: hashar.Feb 23 2017, 12:04 PM
Jhernandez updated the task description. (Show Details)

This are the QUnit tests missing migration, not many to go:

  • actions.test.js
  • checkin.test.js
  • integration.test.js
  • pageVisibility.test.js
  • processLinks.test.js
  • schema.test.js
  • userSettings.test.js
Jhernandez updated the task description. (Show Details)Feb 27 2017, 3:12 PM
  • actions.test.js
  • integration.test.js

https://gerrit.wikimedia.org/r/#/c/340136/

  • checkin.test.js

Removed on master

  • pageVisibility.test.js
  • processLinks.test.js
  • schema.test.js
  • userSettings.test.js
  • userSettings.test.js

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

  • processLinks.test.js

Needs discussion, right now it is an integration test that tests mw.Uri, mw.Title and mw.Regexp.escape. Not sure if I should leave it as integration in browser QUnit or make it a unit test that checks that those core constructs are called and migrate it. Or maybe both. cc/ @phuedx

  • schema.test.js

Doing

ovasileva set the point value for this task to 13.Mar 1 2017, 6:11 PM

3 patches for review at https://gerrit.wikimedia.org/r/#/q/status:open+project:mediawiki/extensions/Popups+branch:master+topic:qunit-tests-in-node


That should pretty much finish the qunit node migration, except for processLinks.test.js. Re-CCing @phuedx

Needs discussion, right now it is an integration test that tests mw.Uri, mw.Title and mw.Regexp.escape. Not sure if I should leave it as integration in browser QUnit or make it a unit test that checks that those core constructs are called and migrate it. Or maybe both. cc/ @phuedx

phuedx added a comment.EditedMar 2 2017, 6:37 AM

mw.Uri and mw.Title should be considered value objects and, consequently, fair game for constructing in unit tests IMO – e.g. you wouldn't test that the Date constructor function had been called with the correct arguments, you'd simply test that a Date with the correct timestamp had been returned.

For the moment, I'm going to leave processLinks.test.js as an integration test in tests/qunit/ given it heavily relies on things like mw.Uri and mw.Title for the test cases which in turn rely on a running MediaWiki instance with a properly configured mw.config, which makes it an integration test.


  • Use webpack 2 instead of the outdated v1

https://gerrit.wikimedia.org/r/#/c/340722/

I'll see how we can pre-compile templates next

  • Use webpack 2 instead of the outdated v1

https://gerrit.wikimedia.org/r/#/c/340722/

  • Migrate QUnit unit browser tests to run in qunit node with common JS

Next steps

  1. Remove the exposed keys on the global object that are not needed.
  2. Set up template pre-compilation and remove mustache compiler and templates from RL config.

Patches for review (if the built files conflict after a merge leave a +2 and I'll rebuild them):

  • Use webpack 2 instead of the outdated v1
  • Remove the exposed keys on the global object that are not needed.

There's a question from @bmansurov around how you've calculated the savings in rEPOP3bb685114f2e: Bundle dist versions of redux and redux-thunk.

Jhernandez updated the task description. (Show Details)Mar 8 2017, 5:24 PM

It was the other way around. https://gerrit.wikimedia.org/r/340948 is merged, and https://gerrit.wikimedia.org/r/340722 has PS10 which addresses the comment.


This is done now:

  • Migrate QUnit unit browser tests to run in qunit node with common JS and remove the exposed keys on the global object

We've talked about it and I'm going to create followup tasks for Minification and Template precompilation since those require measuring before and after the changes to analyze the effects.

Then I'll move this task to sign-off to be closed.

Added T160059: Popups technical improvements related to frontend assets as an epic to track the followup work. This task has lived long enough.

Off to signoff for the tech lead @bmansurov to sign off.

For the moment, I'm going to leave processLinks.test.js as an integration test in tests/qunit/ given it heavily relies on things like mw.Uri and mw.Title for the test cases which in turn rely on a running MediaWiki instance with a properly configured mw.config, which makes it an integration test.

@Jhernandez are we tracking this work in a task?

bmansurov closed this task as Resolved.Mar 9 2017, 8:41 PM

@bmansurov You mean moving the processLinks test to node-qunit? It is not in a task right now since it doesn't seem to be worth the effort. Do you want me to create one?

@Jhernandez Yes. Since moving all tests to node was one of the A/C, we should create a task (similar to how we did for other parts, e.g. templates, of this task) to track the remaining work. I'd appreciate it if you can create a task.