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

In T156333#2976947, @Jdlrobson wrote:

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?

In T156333#2979562, @Jdlrobson wrote:

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

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…

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

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

Patches for review:

• IIFEs:
  • https://gerrit.wikimedia.org/r/#/q/status:open+project:mediawiki/extensions/Popups+branch:master+topic:iife1-h8-gerrit
  • https://gerrit.wikimedia.org/r/#/q/status:open+project:mediawiki/extensions/Popups+branch:master+topic:iife3-h8-gerrit
• QUnit unit browser tests to run in qunit node:
  • https://gerrit.wikimedia.org/r/#/q/status:open+project:mediawiki/extensions/Popups+branch:master+topic:qunit-tests-in-node
  • I'll be migrating more tests, but those patches are ready for review

IIFEs mergerd, qunit tests in node up for review

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

@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.

Thanks @bmansurov

The issue is tracked here I think T117710#3047699

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

In T156333#3058005, @Jhernandez wrote:

• actions.test.js
• integration.test.js

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

In T156333#3058005, @Jhernandez wrote:

• 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

• userSettings.test.js

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

• schema.test.js

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

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

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

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

• https://gerrit.wikimedia.org/r/340729 Tests: Document why processLinks tests are integration tests
• https://gerrit.wikimedia.org/r/340730 Hygiene: Fix eslint warnings on processLinks.test.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

• Optimize redux and redux-thunk packaging https://gerrit.wikimedia.org/r/340948
• Use webpack 2: https://gerrit.wikimedia.org/r/340722

• Remove the exposed keys on the global object that are not needed.

• Hygiene: Remove stubs files from browser qunit tests https://gerrit.wikimedia.org/r/340951
• Hygiene: Remove global variable mw.popups in most places https://gerrit.wikimedia.org/r/340952

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

https://gerrit.wikimedia.org/r/340722 has a trivial -1 from @phuedx and needs a rebase which blocks https://gerrit.wikimedia.org/r/340948

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

Templates next.

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.

In T156333#3067630, @Jhernandez wrote:

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 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.

Done in T160406: Migrate processLinks.test.js to node-qunit