stampit-org / stamp-specification

The Stamp Specification: Composables
434 stars 16 forks source link

Use smaller assign instead of lodash.assign #95

Closed danielkcz closed 8 years ago

danielkcz commented 8 years ago

Another finding regarding bundle size.

At first it might seem that lodash.assign is winner here with 6.8K minified vs 7.6K for polyfilled Object.assign. However there are two main reasons why would I stick to Object.assign.

koresar commented 8 years ago

I would drop lodash.assign altogether for smaller polyfils like this: https://github.com/sindresorhus/object-assign/blob/master/index.js

danielkcz commented 8 years ago

Hm, yea I guess might be another option, however consider that someone wouldn't be able to make a build with just native Object.assign thus effectively removing any support code for it. For someone targeting only major browsers it might be another 1K down in size. Although I am not sure about performance of native implementations, heard some stories that polyfill is usually faster...

koresar commented 8 years ago

AFAIK Object.assign in the latest v8 versions (node v6 at least) is as fast as lodash or polyfills. The rest native implementation are slower though.

Premature optimizations is the root of all evil. I would prefer us to optimize

danielkcz commented 8 years ago

I think that general idea should be to give consumer ability to choose if he wants use polyfill or native solution depending on his target platform. If we just add there some external implementation we are taking any choice from him and adding another dependency to the package.

koresar commented 8 years ago

Yep. Agree. It's good we have the rollup now! :)

danielkcz commented 8 years ago

Alright, so based on investigation in #97 I don't longer thing that Object.assign is best approach. As much as it would be nice to use standardized functions, it's still very problematic in cases when it has to be polyfilled.

I think that using https://github.com/sindresorhus/object-assign/blob/master/index.js might be best solution after all. I was wrong that someone wouldn't be able to use native solution. This library does check if native one is available. With some minification and dead code elimination it shouldn't be present in result bundle.

Edit: We could actually use https://github.com/rollup/rollup-plugin-inject. That means there could Object.assign in the code but with this plugin it can be replaced with that package in relevant builds.

danielkcz commented 8 years ago

I suppose we can close this as well since it's not needed anymore to minimize bundle size.