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! :smiley: 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]

:smiley: :+1:

[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.

[nateberkopec]

Couple of things:

• Gzipped size is not the end-all of the discussion. Yes, a smaller gzipped file can be downloaded faster across the network, but the minified, non-gzipped size must also be taken into account because that Javascript must be executed as well. Download time and execution time are both important, especially on computing-constrained devices like phones.
• @chrisvfritz is right that script download/execution time is more important than image weight. External scripts block the document parser. Given that I'm sitting in the react_on_rails repo here, I'm guessing most people are using a simple script tag without async or defer attributes because they need to react to render the page. Fair enough. But that means any additional weight added to application.js delays initial page render and load times. The amount of this delay may or may not be acceptable to you.
• I would suggest to @chrisvfritz that if your Javascript is churning that hard, you should split your Javascript into multiple script tags, organized by how often they churn. For example, group all of your libraries and vendored javascript into a "libraries.js" script tag, then all of your app js into an "app.js" script tag. With proper HTTP caching, this would reduce download times for your repeat users. If you can, get an HTTP/2 enabled CDN (such as cloudflare), and split your Javascript up as much as you want.
• These are my suggested bandwidth numbers for plugging into Chrome DevTools throttling feature., derived from Akamai data and others.

@justin808 have you written anywhere about how you're using react and turbolinks on the same app? that's interesting.

[chrisvfritz]

Thanks for the suggestion @nateberkopec. That is exactly what I do on heavier apps. :smiley: Also :+1: for Chrome's throttling on the Network tab - was just about to suggest that feature. Before even running perf comparisons though, some simple math will tell us that 60K on the average 4Mbps for mobile in the US adds 120ms to load time, just for the download, before we even get to execution.

Something most of these stats assume though is that the user has a strong connection to LTE or 4G. I've seen that with many carriers, in many parts of the United States, connections to those networks are spotty. That means 1 Mbps or lower is typical for many users and now that 60K adds at least 480ms to load time.

I already integrate Turbolinks 3 into some of my apps (it allows much finer control than Turbolinks 2) and split assets into vendor and app. These help a lot. They make 95% of page loads really fast. But even with all these optimizations, that critical minority of first-time visits (which for some apps, won't even be a minority) is unaffected. It's still slow. Before it even does anything, the footprint of this project's JavaScript is bigger than for most of my applications after months of development.

Here's a case where we can make a significant and measurable impact and for free. It's not premature optimization, because a critical criterion in the definition is that the optimization has "a strong negative impact when debugging and maintenance are considered." There's zero negative impact here.

Knuth rightly notes that:

We should forget about small efficiencies, say about 97% of the time: premature optimization is the root of all evil. Yet we should not pass up our opportunities in that critical 3%.

This is a relatively small efficiency, but it's in the rare 3% where we get it for free. No downsides. For your apps, it sounds like it won't make a difference. But this is a public project and for my apps and probably many others, it will.

---

@nateberkopec: As an aside on Turbolinks, Turbolinks 3 combined with React has been great for me. I can still use Rails for all my routing, but get fantastic page loads and I use nprogress-rails for even greater perceived performance.

Some Turbolinks 3 features I frequently take advantage of in my React components:

• Turbolinks.cacheCurrentPage() whenever new data is pulled in or a persisted change is made.
• Turbolinks.visit(someURL) for finer control over when Turbolinks caching is used.

[justin808]

@chrisvfritz If you want to throw in a PR that moves the lodash to v4 and uses the new syntax, super. I can also do it as well.

@nateberkopec and @jdalton Thanks so much for your insights. It's too bad that @sokra's amazing Webpack can do the job automatically with sticking to the basic import 'lodash'.

In terms of turbolinks, ReactOnRails has the code to work with it, so you just turn it on:

https://github.com/shakacode/react-webpack-rails-tutorial/blob/master/app%2Fviews%2Flayouts%2Fapplication.html.erb#L5

  <%= env_stylesheet_link_tag 'application_prod', 'application_dev', media: 'all', 'data-turbolinks-track' => true  %>
  <%= env_javascript_include_tag 'application_prod', 'application_dev', 'data-turbolinks-track' => true %>

[alex35mil]

@chrisvfritz @justin808 We paired with @robwise a bit on class properties + fat arrows issue. I was wrong and the problem in repo (that I linked previously) was not with wrong context, but with equality check in shouldComponentUpdate of Column component of fixed-data-table lib (component wasn't re-rendered b/c method was always the same). So class property way works, @chrisvfritz don't waste your time on checking it.

[chrisvfritz]

@alexfedoseev Phew. :smiley: That's great news! @justin808 Should we go that route after all then and remove lodash as a dependency?

[jdalton]

@justin808 See https://github.com/lodash/babel-plugin-lodash

[justin808]

@jdalton I love the simplicity of that solution!

@chrisvfritz Let's see the relative bundle sizes if we we apply the babel-plugin-lodash. Are you onboard with that solution? Or would you prefer the non-plugin way?

Longer term, we do like defining the callbacks with fat arrows. However, we just got bit by a babel issue with static props needing semicolons (Stage 1) that was a real hassle to change. Maybe once we get to stage 2 for this proposal?

Class properties are Stage 1 as of 1/17/2016:

• https://github.com/jeffmo/es-class-fields-and-static-properties
• https://github.com/tc39/ecma262

[chrisvfritz]

babel-plugin-lodash does look pretty clever! For a public project though, I worry that it might be a bit too magical. Others could open new issues to "fix" the faux-global import, not realizing that a special lodash-specific plugin is being used to make lodash imports work differently from imports of any other library. So I'd personally prefer the more explicit, non-plugin way.

I hear ya on that @#%$ing semicolon issue. I'm fine waiting for stage 2. It's times like this I'm reminded how crazy it is that everyone has accepted we should all use the most unstable and feature-limited compile-to-JS language yet developed. Can't deny that the tooling is great though. :disappointed:

[jdalton]

Others could open new issues to "fix" the faux-global import, not realizing that a special lodash-specific plugin is being used to make lodash imports work differently from imports of any other library.

Closing an issue is cheap and so is documentation, but I hear ya.

[chrisvfritz]

@jdalton I'm definitely sharing it for internal projects! And I've bookmarked it for later because it's given me some other ideas. :smiley: I can't wait for the day when this is just how importing works, but right now, most people don't even realize it's possible. And for public projects, I generally strive for intuitive over elegant - and have usually regretted it when I haven't.

[justin808]

@chrisvfritz I sort of expect that Webpack would be able clean out unused code, so I don't think it's too unexpected given that we're heavily based on Webpack.

[alex35mil]

Tree shaking is going to be implemented in Webpack 2: http://www.2ality.com/2015/12/webpack-tree-shaking.html

[jdalton]

Tree shaking is going to be implemented in Webpack 2

Unfortunately it won't work with Lodash (webpack/rollup) have some problems which also casts shade on either's ability to tree-shake anything beyond a hello world example.

[chrisvfritz]

@justin808 Up to you ultimately. I'd definitely like Weback to do that for all my code, but I've never seen anything like it. Hence why I personally don't expect it. And technically, though this is the kind of responsibility we'd expect Webpack to handle, it has nothing to do with the magical trimming here. It's happening in a Babel plugin outside of Webpack. That's obviously caused some confusion already. And it splits bundling responsibilities between two projects, adding complexity. Choosing between convenience and simplicity, I'll personally choose simplicity every time.

[justin808]

Right now, the plan is to update the generator to use the babel https://github.com/lodash/babel-plugin-lodash. Any volunteers to help with this PR? I'd like to see us do this first on the tutorial: https://github.com/shakacode/react-webpack-rails-tutorial/issues/204

[justin808]

Here's a related issue: https://github.com/shakacode/react-webpack-rails-tutorial/pull/234

[justin808]

@chrisvfritz We're switching to the fat arrow syntax for binding. So the issue then becomes just efficiently including Lodash. @jbhatab I'm tempted to close this one, as it's more of a doc issue or an example issue for https://github.com/shakacode/react-webpack-rails-tutorial/.

[robwise]

@justin808 we're removing lodash from the generator, this can be closed