Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not reinvent the wheel. Outsource utility functions. #430

Closed

Conversation

gajus
Copy link
Contributor

@gajus gajus commented Aug 8, 2015

pick, mapValues and isPlainObject are standard utility functions available in lodash. Reduce bug surface by using ready made and well tested functions with minimum overhead.

@aaronjensen
Copy link
Contributor

Relevant: #306

@gaearon
Copy link
Contributor

gaearon commented Aug 8, 2015

We only need a small part of what they provide, and we really care about keeping the library tiny. What is the total library size before and after this change?

@gajus
Copy link
Contributor Author

gajus commented Aug 8, 2015

Notice that I am not including the entire lodash package. I am only including:

"lodash.isplainobject": "^3.2.0",
"lodash.mapvalues": "^3.0.1",
"lodash.pick": "^3.1.0"

@gajus
Copy link
Contributor Author

gajus commented Aug 8, 2015

Before:

curiosity:redux (gaearon) gajus$ ls -lah ./dist/
total 64
drwx------   4 gajus  staff   136B  8 Aug 23:54 .
drwx------  26 gajus  staff   884B  8 Aug 23:54 ..
-rw-------   1 gajus  staff    23K  8 Aug 23:54 redux.js
-rw-------   1 gajus  staff   5.4K  8 Aug 23:54 redux.min.js

After:

curiosity:redux gajus$ ls -lah ./dist/
total 328
drwx------   4 gajus  staff   136B  8 Aug 23:53 .
drwx------  26 gajus  staff   884B  8 Aug 23:53 ..
-rw-------   1 gajus  staff   138K  8 Aug 23:53 redux.js
-rw-------   1 gajus  staff    22K  8 Aug 23:53 redux.min.js

@gajus
Copy link
Contributor Author

gajus commented Aug 8, 2015

You are not using the DedupePlugin. Include it in the webpack config:

webpack.config.production.js:

config.plugins = [
    new webpack.optimize.DedupePlugin(),
    new webpack.optimize.OccurenceOrderPlugin(),
    new webpack.DefinePlugin({
        'process.env.NODE_ENV': JSON.stringify('production')
    }),
    new webpack.optimize.UglifyJsPlugin({
        compressor: {
            screw_ie8: true,
            warnings: false
        }
    })
];

With this in place, redux.min.js is 16.7 kB.

@gajus
Copy link
Contributor Author

gajus commented Aug 8, 2015

That said, it doesn't matter if redux.min.js is 5K, 20K or 100K.

@knowbody
Copy link
Contributor

knowbody commented Aug 8, 2015

@gajus but what's the point of replacing it? it does the same thing and it just makes redux bigger

@gajus
Copy link
Contributor Author

gajus commented Aug 8, 2015

  • Reducing size of the source code.
  • Separation of implementation and behavior.
  • Lowering barrier of entry to contribute to the library (as a result of people being already familiar to lodash API).
  • Free-rider effect: less tests to look after, less documentation to write.

On Aug 9, 2015, at 02:12, Mateusz Zatorski notifications@github.com wrote:

@gajus but what's the point of replacing it? it does the same thing and it just makes redux bigger


Reply to this email directly or view it on GitHub.

@tacone
Copy link

tacone commented Aug 8, 2015

The size trade-off does not seem worthy.

@aaronjensen
Copy link
Contributor

One thing to consider is that many people use lodash. By depending on lodash it may actually decrease total size on modern builds with dedupe. 

Aaron

On Sat, Aug 8, 2015 at 4:42 PM, tacone notifications@github.com wrote:

The size trade-off does not seem worthy.

Reply to this email directly or view it on GitHub:
#430 (comment)

@gaearon
Copy link
Contributor

gaearon commented Aug 9, 2015

That said, it doesn't matter if redux.min.js is 5K, 20K or 100K.

It may not matter to you, but it is one of the factors people consider choosing a library. It is also how they tell “frameworks” from “microlibraries”. Even for marketing purposes, we don't want to give an impression we're a framework, because we're just not.

@gaearon
Copy link
Contributor

gaearon commented Aug 9, 2015

Gzipped size before this change: 1957.
Gzipped size after this change: 5481.
(Weirdly, with dedupe plugin it's 5503.)

This is almost 3x size increase.

I'm pretty sure we're unlikely to discover edge cases in mapValues or pick (they're trivial). We have discovered an edge case in isPlainObject, but we have fixed it, and it's not a big hurdle to maintain it because we just use Lodash implementation.

I understand where your concern is coming from. In fact, if you look back in the history, depending on Lodash individual modules is how this library started. However, if we can be 3x smaller at the cost of maintaining three tiny utility functions, we'll do it.

Thank you for your input!

@gaearon gaearon closed this Aug 9, 2015
@mindjuice
Copy link
Contributor

I know this is closed and I completely agree with the size tradeoff decision.

What I don't understand though is how the difference can be so big!

Why does:

 import isPlainObject from 'lodash.isplainobject';

have so much more overhead than:

import isPlainObject from './utils/isplainobject';

(and similarly for the other 2 methods)

@alkhe
Copy link
Contributor

alkhe commented Aug 9, 2015

Same thing I encountered with my flux implementation. Using just flattenDeep and mapValues increased my min/gz size by five times :(

@alkhe
Copy link
Contributor

alkhe commented Aug 9, 2015

The thing is, lodash pushes all of these standardization utility functions that carry a lot of overhead. @gaearon's decision is natural for a scenario like this.

@mindjuice
Copy link
Contributor

So out of curiosity I looked into another way to include lodash with a custom lodash-cli build:

lodash modern include=isPlainObject,pick,mapValues

This resulted in the following two files:

-rw-r--r--    1 kencr2000  staff      67700  8 Aug 21:40 lodash.custom.js
-rw-r--r--    1 kencr2000  staff      10937  8 Aug 21:40 lodash.custom.min.js

These files include dozens of other utility functions that the included functions depend on in the lodash implementation, thus the size bloat (which looking back now is what @edge was referring to).

The functions include things like:

  • isObjectLike
  • arrayCopy
  • arrayEach
  • arrayPush
  • baseCopy
  • baseToString
  • baseCallback
  • etc.

So yeah, that's definitely not happening.

@kof
Copy link

kof commented Aug 20, 2015

Well, lodash does add the overhead and I am struggling too in my libs with that issue. But what we all do not consider is that lodash is the market leader and if not already, most project will use it. Overhead will be minimal if the main project uses those lodash functions anyways.

@kof
Copy link

kof commented Aug 20, 2015

In a contrast, if every lib will NOT use lodash and maintain own functions - they are not helping to make lodash more popular and though we are not helping in general.

Lets see the BIG PICTURE!

@gaearon
Copy link
Contributor

gaearon commented Aug 20, 2015

It's cool and great to use lodash from a larger library that actually depends on non-trivial functions, but it feels like a crime to go from being 2K to 5K for no other reason than pick.

@kof
Copy link

kof commented Aug 20, 2015

isPlainObject looks very compact by now

https://github.com/lodash/lodash/blob/master/lodash.js#L8352

@kof
Copy link

kof commented Aug 20, 2015

But it definitely looks like a crime when lib is so small like redux. It requires to be very idealistic and optimistic to use lodash functions in this cases.

I think though we need to push lodash forward to reach at some point a smaller overall footprint of the applications if they all rely on lodash.

@jdalton

@aaronjensen
Copy link
Contributor

I debated removing lodash from https://github.com/substantial/updeep but ended up putting it back in because of some performance gain. I didn't want to maintain those things, but it does add a pretty large file size overhead. I still ended up removing some (particularly curry, so I could use a faster, more ramda-like version)

@phated
Copy link

phated commented Aug 21, 2015

Lodash is working very hard to support people that want the consistency of the library but don't use all the sugar. This will allow people to have near-same file size without maintaining their own fork. You will also be able to reduce the size further using a couple of lines in the webpack config to do some module replacement. These changes should be landing soon, and I hope this can be revisited at that time.

@mindjuice
Copy link
Contributor

@kof Yes, isPlainObject is small, but it depends on other functions that depend on other functions, etc. See my post above on the resulting size of a custom lodash build including only the three functions Redux uses.

@jdalton
Copy link
Contributor

jdalton commented Aug 21, 2015

Yes, isPlainObject is small, but it depends on other functions that depend on other functions, etc. See my post above on the resulting size of a custom lodash build including only the three functions Redux uses.

As @phated wrote we're working on it. Once v4 lands the size should be comparable (within a kb or so) of what you're doing now.

@mindjuice Thanks for the help by the way :/

@mindjuice
Copy link
Contributor

@jdalton A passive aggressive class act as always! Don't ever change!

For me there is no "big issue". Redux doesn't use lodash. I don't use lodash, and the libraries I use don't use lodash, so why would I open an issue in lodash?

@jdalton
Copy link
Contributor

jdalton commented Aug 21, 2015

@mindjuice While you may not use lodash, redux has lodash in its dev-deps graph and until mid-June had lodash as a core dependency :)

@kof
Copy link

kof commented Aug 21, 2015

@mindjuice you need a bit more of idealistic thinking)

@jdalton We need to improve the size of each function in lodash. I don't think there is any project out there using even half of lodash. Increasing an overall size in favor of lower size of each function should be an acceptable tradeoff.

Also I would love to see just one way of including lodash functions. Right now there is a good chance to have the same function included from different build types more than once.

@jdalton
Copy link
Contributor

jdalton commented Aug 21, 2015

lodash v4 is modern first – dropping support for older enviros (use es5/6-shim for them). We've also ensured there's a path available for further reducing the size through webpack or browserify module aliases. For example going forward you'll be able to swap lodash/internal/baseIteratee with lodash/internal/toFunction to cut out the heavier callback shorthand code.

FWIW with the current WIP lodash v4 + redux is clocking in at 2.45 ~2.17 kB gzipped compared to the current redux size of 2.43 kB gzipped.

@kof
Copy link

kof commented Aug 21, 2015

@jdalton totally acceptable!

@gaearon
Copy link
Contributor

gaearon commented Aug 21, 2015

I'm glad to hear you folks are working on this!

@jdalton
Copy link
Contributor

jdalton commented Aug 22, 2015

Here's the WIP branch changes: https://github.com/jdalton/redux/tree/lodash

It turns out you could shave a couple bytes off by adding node: { process: false } to the webpack config which prevents it from loading the process shim.

@gaearon
Copy link
Contributor

gaearon commented Aug 22, 2015

Whoops! Good catch :-P

@jdalton
Copy link
Contributor

jdalton commented Aug 22, 2015

Digging a bit more by making the check in combineReducers for just __DEV__ and replacing it with webpack.DefinePlugin it skips the entire verifyStateShape function for production.

@gaearon
Copy link
Contributor

gaearon commented Aug 22, 2015

I think we'll drop __DEV__ in the next version and just use process.env.NODE_ENV. This will have the same effect. The reason we added it in the first place is because of React Native (#525), but React Native v0.10 now polyfills process.env.NODE_ENV so we can safely use it again.

When React Native 0.10 is out, I think we can bump major version and stop using __DEV__.

@aaronjensen
Copy link
Contributor

I think the point is that once React Native polyfills it, the process !== 'undefined' check is unnecessary, so it'd go back to something uglify can deal w/.

@jdalton
Copy link
Contributor

jdalton commented Aug 22, 2015

Got it working, unit tests pass, by replacing both __DEV__ and process.env.NODE_ENV with webpack.DefinePlugin for a size of ~2.17 kB with lodash v4. See https://github.com/jdalton/redux/commit/7c0f80bc81e45fbb00e977318dc5306d1f0284d4.

@gustavoguichard
Copy link

The only real reason I feel tempted to use Ramda.js is that they use functions first, then objects (e.g. _.map(x => x+1, array) ).
That is very helpful for currying. Any plans to have that sort of model in coming up versions of lodash?
Thanks for amazing work ;)

@aaronjensen
Copy link
Contributor

@gustavoguichard
Copy link

Sorry, this is the wrong thread for my comment... I thought it was a 4.0 discussion :/

@gaearon
Copy link
Contributor

gaearon commented Aug 24, 2015

Superseded by #611.

@gustavoguichard
Copy link

Wow! Didn't know that @aaronjensen ... ty, cheers!

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

Successfully merging this pull request may close these issues.

None yet