Page MenuHomePhabricator

Minify assets via build step using node tooling
Closed, ResolvedPublic5 Story Points

Description

Given Page-Previews uses a pre-compilation step, there are clear benefits to using node based uglifyJS regarding size of bundle set to the client and source maps generation (see Compressed JS comparison)

In order to serve smaller payloads to users and improve the debugging experience in production
We need to use uglifyjs when running npm run build and ensure source-maps are served in production

AC

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 9 2017, 12:04 PM
phuedx moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.
phuedx added subscribers: ovasileva, bmansurov, phuedx.

^ I moved this to Needs Analysis as I'm uncertain of the priority. @bmansurov, @ovasileva: Over to you, I guess.

I'd say the priority is normal.

Jdlrobson triaged this task as Normal priority.Mar 21 2017, 6:37 PM
Jdlrobson added a subscriber: Jdlrobson.

Agreed. Moved to triaged but future.

MBinder_WMF set the point value for this task to 5.Apr 25 2017, 4:24 PM
Jdlrobson raised the priority of this task from Normal to High.May 4 2017, 12:12 AM
phuedx updated the task description. (Show Details)May 4 2017, 8:14 AM
phuedx added a comment.EditedMay 4 2017, 8:24 AM

ResourceLoader doesn't re-minify ("filter" in RL parlance) the already minified asset

I added this as an AC because minifying more than once is advised against and, even if we're confident that the RL minifier won't alter the minified bundle, it's pretty trivial to get this working using webpack's BannerPlugin plugin:

webpack.config.
module.exports = {
  /* ... */
  plugins: [
    /* ... */
    new webpack.BannerPlugin( {
      banner: '/*@nomin*/', // ResourceLoader::FILTER_NOMIN
      raw: true // Don't wrap the banner in a comment. It's easier to keep the banner in sync with ResourceLoader::FILTER_NOMIN this way.
    } )
  ]
}

@phuedx Awesome, much better! Link to the webpack v2 docs on webpack.js.org, webpack.github.io are the docs for v1

phuedx added a comment.May 4 2017, 9:45 AM

@Jhernandez: Updated my comment accordingly. Thanks 👊

Jhernandez updated the task description. (Show Details)May 4 2017, 9:57 AM

Change 353136 had a related patch set uploaded (by Jhernandez; owner: Jhernandez):
[mediawiki/extensions/Popups@master] Hygiene: Make webpack.config.js conform to eslint

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

Change 353137 had a related patch set uploaded (by Jhernandez; owner: Jhernandez):
[mediawiki/extensions/Popups@master] Enable production settings for the production bundle

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

Change 353138 had a related patch set uploaded (by Jhernandez; owner: Jhernandez):
[mediawiki/extensions/Popups@master] Tools: Add pre-commit git hook to auto test and build

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

I'm missing a patch for the documentation on the repo and moving my on-wiki comparison to a page under Page previews, I'll get to that soon.

Change 353136 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Hygiene: Make webpack.config.js conform to eslint

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

Change 353137 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Enable production settings for the production bundle

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

Change 353138 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Tools: Add pre-commit git hook to auto test and build

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

To add ADR and wiki docs now.

Sorry, I'll leave it in QA now, we just crossed comments.

Change 353563 had a related patch set uploaded (by Jhernandez; owner: Jhernandez):
[mediawiki/extensions/Popups@master] Docs: Add ADR about using webpack's production mode and UglifyJS

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

Change 353563 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Docs: Add ADR about using webpack's production mode and UglifyJS

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

I did some testing on the beta site. Using browserstack, I ran some basic testcases for page previews. Everything worked normally. Here are the systems/browser that I tested.

Mac Sierra - Safari 10
Mac El Capitan - Firefox 53
Mac Sierra - Chrome 58
Mac Sierra - Opera 44
Mac Mountain Lion - Firefox 10

Windows 10 - Edge 15
Windows 10 - Chrome 58
Windows 8.1 - Firefox 53
Windows 8.1 - Opera 44
Windows 7 - Opera 10.6

@phuedx I can try some more systems or browsers or whatever. If there are any other tests I can run, let me know.

Thanks, @ABorbaWMF. I've also tested PP in IE10 and 11 on Windows 7 using BrowserStack so I think we're good 👍

Jhernandez updated the task description. (Show Details)May 15 2017, 1:13 PM
phuedx reassigned this task from ABorbaWMF to Jdlrobson.May 15 2017, 5:14 PM
Jdlrobson closed this task as Resolved.May 16 2017, 9:44 AM

LGTM. Signing off.