Page MenuHomePhabricator

Step 1: minification of common.js (formerly: Investigate common.js bundle content/size)
Closed, ResolvedPublic3 Estimated Story Points

Description

Apply uglify.js to common.js file.

Old description:
The common.js build of bridge does not seem to receive app-build-level optimization.
It is not minified and contains comments - which has certain benefits (think: debugging) and should be mitigated by ResourceLoader.
However, it contains information that is not essential to the app (e.g. the entirety of HTTP status codes, even the ones not used in the app). How can we avoid this?
How can we further code-split, e.g. providing conditional modules of polyfills for modern browsers via ResourceLoader's skipFunction.

Findings should also be communicated to the termbox team for T228528: Termbox: Use "vue" ResourceLoader module from core.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 24 2019, 11:52 AM
Pablo-WMDE updated the task description. (Show Details)Aug 30 2019, 5:30 PM

There’s a fairly simple way to see what webpack is including in the built files (I’m checking the init module here because I’m doing this as part of investigating T233305, but the app module should work similarly):

node_modules/.bin/vue-cli-service build --mode development
grep -F '!*** ' dist/data-bridge.init.js

I’ve pasted the full list at P9267 – mostly, it’s a whole bunch of core-js polyfills, including for things like Function.prototype.bind which we are really, really guaranteed to have already (by MediaWiki) and things like RegExp.prototype[@@match] that we never use directly. That list also doesn’t change if I trim the .browserslistrc, so I’m not sure where @babel/preset-env gets its browser list from that it thinks we need to be compatible with.

I looked into the bind thing a bit further, and as far as I can tell the chain of dependencies is:

  • ./node_modules/core-js/library/modules/_bind.js
  • is imported by ./node_modules/core-js/library/modules/es6.reflect.construct.js
  • is imported by ./node_modules/core-js/library/fn/reflect/construct.js
  • is imported by ./node_modules/@babel/runtime-corejs2/core-js/reflect/construct.js
  • is imported by ./node_modules/@babel/runtime-corejs2/helpers/esm/construct.js
  • is imported by ./node_modules/@babel/runtime-corejs2/helpers/esm/wrapNativeSuper.js
  • is imported by:
    • ./src/data-access/error/EntityNotFound.ts
    • ./src/data-access/error/EntityWithoutLabelInLanguageException.ts
    • ./src/data-access/error/JQueryTechnicalError.ts
    • ./src/data-access/error/TechnicalProblem.ts

Three of those last four classes are completely empty, by the way. But Babel still thinks their (implicit) constructors need to “wrap native super”, whatever that is, which requires four levels of Reflect.construct shims, which ultimately require Function.prototype.bind. And at no point in this chain are there any checks if the browsers list already includes the needed code – these are all regular, unconditional require() imports.

As far as I can tell, @babel/preset-env is good for checking which parts of core-js your own code actually needs, and only including those parts – but then the various parts of core-js (and @babel/runtime-corejs2, apparently) still merrily depend upon one another, so that you still end up pulling in a lot of code that you don’t actually need.

@Lucas_Werkmeister_WMDE With regards to browserslist behaving funny: try, to rule something out, to run the build in an env where the directories from data-bridge up are not present.

Thanks, that’s… good to know… but I copied data-bridge into /tmp and it looks like it’s still generating the same modules. I think it’s because of the cross-module imports that I mentioned in the second half of the comment, not because of the browsers list directly.

I spent what felt like (and maybe even was) hours on this only to be hit at the head with the realization that npm/node for some unfathomable reason caches the contents of .browserlistrc and babel.config.js. This cache can be deleted with
$ rm -rf node_modules/.cache

Then changes to .browserlistrc are correctly recognized for me.

I made this exact mistake a few months ago when the hike started and forgot about it again 🤦. Now there is a post-it at my monitor. Maybe it helps.

darthmon_wmde renamed this task from Investigate common.js bundle content/size to Step 1: Investigate common.js bundle content/size.Dec 10 2019, 2:29 PM
Pablo-WMDE updated the task description. (Show Details)Jan 2 2020, 10:22 AM

Change 571482 had a related patch set uploaded (by Pablo Grass (WMDE); owner: Pablo Grass (WMDE)):
[mediawiki/extensions/Wikibase@master] bridge: minify app build

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

Pablo-WMDE updated the task description. (Show Details)Feb 14 2020, 4:24 PM

https://gerrit.wikimedia.org/r/571482 is an attempt at reducing the size, but without changing the content. "Tree shaking" (e.g. https://github.com/indutny/webpack-common-shake/) is a much harder problem, and - if it were added - would have to come as an earlier step (i.e. before uglify).

https://gerrit.wikimedia.org/r/571482 is an attempt at reducing the size, but without changing the content. "Tree shaking" (e.g. https://github.com/indutny/webpack-common-shake/) is a much harder problem, and - if it were added - would have to come as an earlier step (i.e. before uglify).

That patch looks good! I tried the tree shaking plugin. Getting it to work was suspiciously easy, but it had basically no effect :/. See https://gerrit.wikimedia.org/r/572277

Decision from story time today: We tend to the minification and then close this ticket. More work will be potentially done as part of the performance review outcome.

Lydia_Pintscher renamed this task from Step 1: Investigate common.js bundle content/size to Step 1: minification of common.js (formerly: Investigate common.js bundle content/size).Mar 24 2020, 2:25 PM
Lydia_Pintscher set the point value for this task to 3.

I tried to move the patch along a bit (enough that I think it can be reviewed now), but anyone else is free to steal it from me to continue working on it.

Change 584964 abandoned by Lucas Werkmeister (WMDE):
bridge: switch from uglify-js to terser

Reason:
squashed into parent change (uglifyjs-webpack-plugin is deprecated, so it would make sense to use terser-webpack-plugin even if it didn’t slightly decrease the compressed size)

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

Change 571482 merged by jenkins-bot:
[mediawiki/extensions/Wikibase@master] bridge: minify app build

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

Pablo-WMDE added a comment.EditedApr 2 2020, 7:58 AM

Minification is applied (via terser) - we don't do any fancyness with debug mode but there was some indication (not in version control though) that source maps do their job, and we do have the dedicated dev-entry point - where we have full control over all levers (incl. ones not easily achieved in mediawiki integration) - which should put us in a good position to reproduce potentially confusing behavior.
I'm in favor of deferring that question and, if we get to it, look at it using an even wider lens, incl. VueDevelopmentMode, integration of app hotreloading into mediawiki, and T85805: ResourceLoader: Redesign how debug mode operates.

Lydia_Pintscher closed this task as Resolved.Apr 3 2020, 10:27 AM
Lydia_Pintscher moved this task from Verification to Done on the Wikidata-Bridge-Sprint-17 board.

\o/