Optimizations for loading JS files, including Lodash (more of a doc issue)

[chrisvfritz]

I see this pattern in the constructors of example components:

_.bindAll(this, 'myFunction');

Putting aside the fact that all of lodash was being imported when only a single function was needed, I think even that single function is unnecessary, since a class property (stage-1) can be used for the same effect:

myFunction = (arg1, arg2) => {
  // ...
};

Even if you decide to later remove the stage-0 preset from .babelrc, I'd personally much prefer the use of ES5's .bind, since it'll also do the job.

this.myFunction = this.myFunction.bind(this)

Then Lodash can be completely removed as a dependency.

[justin808]

Hi @chrisvfritz, the reason we add it is that we use many functions in lodash. Yes, you can do some fancier importing to trim it out.

I tell you what. If you can do a comparison of performance with and without lodash for the super simple generated example case, and if there is real difference, we'll switch.

CC: @jdalton, @alexfedoseev

[justin808]

@chrisvfritz One other factor is that the npm react-on-rails does not include lodash, partly so we don't pull it in. On our production system, we found that with using many libraries, and we were already including lodash.

[jdalton]

I can tell you upfront that the creation of bound methods is more expensive in lodash than the execution of them. We push the cost to creation because it's a 1 time hit that enables us to optimize for execution.

That said, if you're only using 1 function and are ES5+ you could, and probably should, drop the dependency (esp. if you're only depending on bindAll).

As @chrisvfritz suggested, this is fine

this.myFunction = this.myFunction.bind(this);

[alex35mil]

@chrisvfritz We tried class property way, but there is an issue with it: it works if the method is used inside component, but when we pass it to children via props — it's not bound to context, so we must do it explicitly (you can take a look at this commit and 2 branches in the repo to reproduce the issue). So we found that it's easier to use only one way (to avoid accidental bugs) — bind methods to context inside the constructor (which is also standards-compliant).

[justin808]

@jdalton Thanks for commenting. If you're using react-on-rails, it's ES6 all the way!

[chrisvfritz]

Thanks for hopping in for the definitive explanation of bindAll @jdalton! 😃 And thanks @justin808 and @alexfedoseev for the great response time. Very interesting to hear about the issue with class property binding - I haven't encountered that issue before and am now wondering if I've just narrowly avoided it. Will have to dive in later and check.

In that case, assuming stage-0 is staying in .babelrc, how about this compromise in the constructor:

this.myFunction = ::this.myFunction

It's a little briefer and still allows trimming lodash from the dependencies. Thoughts?

[justin808]

@chrisvfritz Please refer to: https://github.com/shakacode/style-guide-javascript

We went down that road. We had issues. A lot of thought (and practice) went into our recommendations.

Main issue is that :: creates a new function each time. We're caching the bound functions, so if you ever need to unlisten, you can do that.

[chrisvfritz]

Then how about just changing:

import _ from 'lodash'

to:

import { bindAll } from 'lodash'

It should cut down the initial build size considerably and it could be argued that even in a project that really needs every lodash function, it's a best practice to only import what you need.

[jdalton]

import { bindAll } from 'lodash/bindAll'

[chrisvfritz]

😃 👍

[justin808]

@chrisvfritz I'd be curious to see if the difference is perceptible. How many bytes are we talking about? I suspect that compared to a few images, there's really no difference measurable.

[chrisvfritz]

For some reason, @jdalton's suggested import was giving me Module not found: Error: Cannot resolve module 'lodash/bindAll'. So I used this instead:

import bindAll from 'lodash/function/bindAll'

On a fresh Rails app after following the Getting Started instructions:

	_	bindAll
app-bundle.js	4.1M	3.2M
precompiled (min)	2.1M	1.8M
precompiled (min+gz)	460K	400K

I personally feel that saving 60K gzipped is not insignificant, especially considering that the size of JavaScript files is much, much more important than the size of an image. I don't think they can be compared.

For example, if I'm waiting on an image to load, I can still see the rest of the content and fully interact with the page. Not a huge deal in most apps. If a user is waiting on the JavaScript to load, they can't do anything if the page heavily relies on React or other JS.

For most active applications, JavaScript is also much less likely to be cached. Images tend not to be updated very often - even spritesheets are rarely changed in most projects I've been a part of. But it's not uncommon to push out builds with newly compiled JavaScript several times per day.

So even if it were only 3K, I've seen the 3Ks add up. And they do make a measurable difference for users, especially those on (still common) slow and unreliable Internet. I'm using React and not Riot, so I'm not expecting micro build sizes. But in this case, all we have to do to avoid unnecessary bloat is follow a best practice that happens to be just as easy as not following the best practice. It's changing a line. I'm kind of baffled at the resistance, to be honest.

[justin808]

@chrisvfritz The reason why we don't like to spend time optimizing the size of lodash is:

1. It's premature optimization for the types of apps we're working on.
2. I've seen plenty of yak shaving like you just did.
3. We may be requiring other libraries that depend on lodash.
4. We plan to use turbolinks and load the JS once, so it won't be loaded on every page refresh.

We'd rather stay consistent and simple, and then optimize once we have real data. I'd be curious if you could come up with a _time_ difference that shows the 60K to load the JS really makes a difference. It would be interesting to see based on some hypothetical scenarios of mobile vs. broadband.

Here's a good article on the topic from @nateberkopec:

https://www.nateberkopec.com/2015/10/07/frontend-performance-chrome-timeline.html

Thanks. This is a great discussion.

[jdalton]

For some reason, jdalton's suggested import was giving me Module not found: Error: Cannot resolve module 'lodash/bindAll':

I gave you lodash v4's path. You must be using v3.