Skip to content
This repository has been archived by the owner on Feb 19, 2022. It is now read-only.

Add babel and webpack lodash plugins #59

Merged
merged 3 commits into from
May 10, 2016

Conversation

okonet
Copy link
Contributor

@okonet okonet commented May 10, 2016

This PR added 2 lodash plugins in order to improve DX and reduce the file size.

From now on it's possible to import lodash modules using import { map, filter } from "lodash" syntax instead of cherry-picking them without size penalties.

Also, the distributive file size is reduced by ~15%.

Before:

[builder:proc:start] Command: webpack --bail --config node_modules/builder-victory-component/config/webpack/webpack.config.dev.js --colors
Hash: 6eca23c94c3a283b5c5b
Version: webpack 1.13.0
Time: 3762ms
         Asset     Size  Chunks             Chunk Names
    victory.js   982 kB       0  [emitted]  main
victory.js.map  1.22 MB       0  [emitted]  main
    + 484 hidden modules

After:

[builder:proc:start] Command: webpack --bail --config node_modules/builder-victory-component/config/webpack/webpack.config.dev.js --colors
Hash: 58f87e564ac06112b7b5
Version: webpack 1.13.0
Time: 3723ms
         Asset     Size  Chunks             Chunk Names
    victory.js   836 kB       0  [emitted]  main
victory.js.map  1.03 MB       0  [emitted]  main
    + 364 hidden modules


// **WARNING**: Mutates base configuration.
// We do this because lodash isn't available in `production` mode.
config.output.filename = config.output.filename.replace(/\.min\.js$/, ".js");
config.plugins = [
new LodashModuleReplacementPlugin(),
new webpack.optimize.OccurenceOrderPlugin(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not do this. It often is worse / no op for min + gz and muddles things up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean the OccurenceOrderPlugin? I can remove it

@ryan-roemer
Copy link
Member

@okonet -- The dev build isn't our main target. We'd ideally like min+gz numbers as that's the "real deal". I'm running some, but if you have some right now would be great to add to your description.

@okonet
Copy link
Contributor Author

okonet commented May 10, 2016

The reason I'm posting dev numbers is the fact I can't build current master without errors having this module symlinked. Not sure how to test it otherwise.

@ryan-roemer
Copy link
Member

@okonet -- I usually test via the wonderful npm pack which creates a tarball which you can then npm install. I'll put up full shell steps and experiment results in a few minutes.

@ryan-roemer
Copy link
Member

ryan-roemer commented May 10, 2016

Summary

Here are all my numbers:

Size master pr/59 pr/59 *
Dev 983180 836806 837055
Dev + Gz 197954 170474 167686
Min 304265 286025 286268
Min + Gz 81565 75620 75952

*: Remove OccurrenceOrderPlugin from pr/59

Thoughts:

  • Size-wise this is a good reduction.
  • Let's remove the OccurrenceOrderPlugin. It's miniscule when it helps and it often hurts when gzipped (see dev + gz numbers are better when removed). And my strong intuition is that it will keep flipping numbers wise with any code changes of the underlying stuff.

So, I'm good with this from an infrastructure / size standpoint. I'll leave you to @boygirl for a substantive evaluation.

Thanks for this contribution!

Base

  • victory@master
  • builder-victory-component@master
  • builder-victory-component-dev@master
$ cd builder-victory-component
$ git checkout master
$ cd ..

$ npm pack builder-victory-component
$ mv builder-victory-component-2.1.3.tgz builder-victory-component-master-2.1.3.tgz

$ npm pack builder-victory-component/dev
$ mv builder-victory-component-dev-2.1.3.tgz builder-victory-component-dev-master-2.1.3.tgz

$ cd victory
$ npm install
$ npm install \
  ../builder-victory-component-master-2.1.3.tgz \
  ../builder-victory-component-dev-master-2.1.3.tgz

$ builder run build-dist

$ cat dist/victory.js | wc -c
  983180
$ cat dist/victory.js | gzip -c | wc -c
  197954
$ cat dist/victory.min.js | wc -c
  304265
$ cat dist/victory.min.js | gzip -c | wc -c
   81565

PR

  • victory@master
  • builder-victory-component@pr/59
  • builder-victory-component-dev@pr/59
$ cd builder-victory-component
$ git checkout pr/59
$ cd ..

$ npm pack builder-victory-component
$ mv builder-victory-component-2.1.3.tgz builder-victory-component-pr59-2.1.3.tgz

$ npm pack builder-victory-component/dev
$ mv builder-victory-component-dev-2.1.3.tgz builder-victory-component-dev-pr59-2.1.3.tgz

$ cd victory
$ npm install
$ npm install \
  ../builder-victory-component-pr59-2.1.3.tgz \
  ../builder-victory-component-dev-pr59-2.1.3.tgz

$ builder run build-dist

$ cat dist/victory.js | wc -c
  836806
$ cat dist/victory.js | gzip -c | wc -c
  170474
$ cat dist/victory.min.js | wc -c
  286025
$ cat dist/victory.min.js | gzip -c | wc -c
   75620

Additional experiment: No OccurenceOrderPlugin

$ builder run build-dist

$ cat dist/victory.js | wc -c
  837055
$ cat dist/victory.js | gzip -c | wc -c
  167686
$ cat dist/victory.min.js | wc -c
  286268
$ cat dist/victory.min.js | gzip -c | wc -c
   75952

@boygirl
Copy link
Contributor

boygirl commented May 10, 2016

@okonet thanks for taking on this task! I will update the lodash imports across victory repos in the course of my work for the upcoming release (Friday).

@boygirl boygirl merged commit 367ccbf into FormidableLabs:master May 10, 2016
@boygirl
Copy link
Contributor

boygirl commented May 13, 2016

: / I just realized that babel-plugin-lodash isn't supported in Node < 0.12. @ryan-roemer thoughts on keeping this change and dropping Node 0.10 from our travis matrix?

@ryan-roemer
Copy link
Member

ryan-roemer commented May 13, 2016

Yeah we can drop 0.10 from Travis. But, this should only affect git installs of victory components, not installs from NPM, so we should perhaps update our docs with that nuanced guideline?

@boygirl
Copy link
Contributor

boygirl commented May 13, 2016

Will do!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants