Add babel plugin lodash

[lukeasrodgers]

Attempts to address #204

I am new to babel and webpack, so I may have screwed some obvious things up here.

See http:https://forum.shakacode.com/t/generated-bundle-size/491/4 for some relevant back and forth with @justin808

Below are some before and after screenshots. Note that to get the "prod" packages, I edited lib/assets.rake to explicitly use a prod build, which it didn't seem to be doing:

  desc "Compile assets with webpack"
  task :webpack do
    sh "cd client && npm run build:production:client"
    sh "cd client && npm run build:production:server"
    sh "mkdir -p public/assets"

    # Critical to manually copy non js/css assets to public/assets as we don't want to fingerprint them
    sh "cp -rf app/assets/webpack/*.woff* app/assets/webpack/*.svg app/assets/webpack/*.ttf "\
       "app/assets/webpack/*.eot* public/assets"

    # TODO: We should be gzipping these
  end

Curiously, with the lodash plugin, the dev app bundle actually increases in size by 300kb, though with the prod build it goes down by 400kb.

Also the syntax mentioned by @jdalton on #206 (import { bindAll } from 'lodash/bindAll') did not work, and appeared to cause bindAll to be undefined.

dev before

dev after

prod before

prod after

[jdalton]

Also the syntax mentioned by jdalton on #206 (import { bindAll } from 'lodash/bindAll') did not work, and appeared to cause bindAll to be undefined.

Are you using Lodash v4?
Are you troubleshooting babel-plugin-lodash? \cc @megawac

[lukeasrodgers]

@jdalton client/package.json shows "lodash": "^4.3.0", so, I believe the answer to that question is "yes."

I am not specifically attempting to troubleshoot that plugin here, no -- this PR just arose out of some attempts on my part to a) understand why the js bundles generated by react_on_rails seemed so large, and b) follow some suggestions to reduce those sizes by using babel-plugin-lodash.

cheers

[megawac]

The bindall issue is new afaik, ill look into it. As for the size increasing, are multiple versions of lodash getting imported now (e.g. from a dep module?)

[lukeasrodgers]

@megawac multiple versions of lodash would be my guess, yes, I'll see if I can track that down.

[lukeasrodgers]

Last two commits should a) fix specs, b) fix linter complaint, c) address increased file size in dev build.

@megawac I believe you were right -- one of my changes was incorrectly importing find from lodash, and this caused app/tests to break, as well as (probably) including lodash twice.

To the maintainers -- I'd be happy to squash these junk commits later, if/when you'd want to merge this.

[justin808]

@lukeasrodgers Absolutely awesome! Let's squash to one commit. If you can write up a few notes in the docs directory, that would be a bonus! I actually want to reconcile the docs between here and react_on_rails/docs eventually. The very best option doc wise is to add a note in the README.md and to open a PR in react_on_rails.

Naturally, we'd like to have most of these changes eventually make it to the app generator in react_on_rails, but our priorities lie in:

1. Just create a great demo of solving the problem, such as this PR in this repo.
2. Create the underlying framework allowing easy integration.
3. Document 1 and 2 well.
4. Add to the generators for future app builders.

Thanks again!

CC: @alexfedoseev @jbhatab @aaronvb @robwise @yorzi

[lukeasrodgers]

@justin808 happy to try to write some documentation

• what exactly do you want do be documented? just the fact that users can/should selectively import lodash code?
• bit confused as to whether it should go in this repo's README or be a new markdown file in docs/

I will squash after we've finalized the docs.

cheers

[justin808]

@lukeasrodgers Let's summarize these recommendations in a doc in the react_on_rails repo and we can link to this PR for more details. We should name the doc:

/docs/additional_reading/minimizing_bundle_size.md

If you click to edit your comments with images, you can copy the markdown that creates the image. I think that is helpful to show the benefit.

I'd recommend that we put a code comment that mentions the importance of using the

import { bindAll } from 'lodash';

rather than

import _ from 'lodash'

Just squash, put a little bit of docs, and if we need further revisions, no worries. Thanks again so much!

[alex35mil]

BTW if you don't like unscoped bindAlls and prefer _.bindAll (as I do, b/c it's clear right here right now where it comes from), you can create index file and use it like this:

// libs/lodash.js
import { bindAll, ... } from 'lodash';

export default { bindAll, ... };

// some.js
import _ from 'libs/lodash';

_.bindAll(...);

[justin808]

@alexfedoseev @lukeasrodgers @jdalton I really like Alex's idea for 2 reasons:

1. All files can still use the familiar looking _.bindAll.
2. Since the project standardizes on one file defining what is coming from Lodash, it's much easier to ensure that all usages are correct.

I'd vote for going with @alexfedoseev recommendation and calling the file:

/client/app/libs/slim_lodash.js

and a file will have

import _ from 'libs/slim_lodash.js';

_.bindAll(...);

Agree?

[lukeasrodgers]

@alexfedoseev and @justin808 sounds good to me, your repo, so your call. One potential drawback of that approach (afaict) is that now if I want to use a new function from lodash I have to change two files (the lib and my own file) instead of just changing my own and letting the lodash plugin sort it out. But I don't think it matters too much either way.

[jdalton]

sounds good to me

👍

[lukeasrodgers]

Okay, the last commit attempts to implement the lib functionality discussed above.

Oddly, the lodash import syntax that worked previously broke when trying this approach -- but only when compiling a production build (caused the functions to be undefined). The syntax in this commit appears to work for both dev and prod builds.

Also, surprisingly, this change alone appears to have shaved ~1 MB off the dev app bundle, down to 4.5 from 5.6

Slightly OT: is webpack + babel always this much of a black box? I'm used to "run these files through closure compiler; adding a new file to that list will make the generated code slightly larger." I get that much more is going on here, but am just wondering whether this "make some minor change and all of a sudden your generated code is 1MB smaller" experience is typical for this kind of webpack+babel pipeline.

[megawac]

I just reread the thread:

Also the syntax mentioned by @jdalton on #206 (import { bindAll } from 'lodash/bindAll') did not work, and appeared to cause bindAll to be undefined.

Thats an erroneously formed import. You want import bindAll from 'lodash/bindAll' or import { bindAll } from 'lodash'; (note where the { ... } are. In the bindAll package the method is the default export)

[megawac]

You shouldn't need to do this

[lukeasrodgers]

I agree. Does this syntax seem correct to you?

import { bindAll, find } from 'lodash';
export default { bindAll, find };

This seems correct to me, and works fine on the dev build, but breaks the production build on this repo: Uncaught ReferenceError: bindAll is not defined. I unfortunately don't have enough experience with these technologies to be able to determine why.

[megawac]

Not sure, I can reproduce though. Looking into it

---

The issue is that babel-plugin-lodash does not recognize/support export specifiers; I don't plan to support it either, when using the babel plugin you should use lodash as normal and let the plugin handle the grunt work of importing the modules directly. If you used the methods directly rather than building the slim_lodash file, babel-plugin-lodash would work as expected

[lukeasrodgers]

See previous discussion on the PR. The idea is to avoid including all of lodash when only specific functionality is required. I appreciate you taking time to look into this, thanks.

[lukeasrodgers]

Thanks for clarifying @megawac.

@justin808 @alexfedoseev I'm going to close this PR. I'm not the right person to try to implement this functionality, I don't have enough experience with webpack/babel/es6 modules, and I think my efforts are just causing more confusion and wasting reviewers' time. I suspect it will be easy for someone else with more experience.
cheers