search

stampit-org / stamp-specification

The Stamp Specification: Composables
434 stars 16 forks source link

[WIP] Use Object.assign instead of lodash #97

Closed danielkcz closed 8 years ago

danielkcz commented 8 years ago

Fixes #95

danielkcz commented 8 years ago

Hmm I see this is gonna be problematic for tests as there is no polyfill.

ericelliott commented 8 years ago

Which engine is this failing on?

danielkcz commented 8 years ago

I am clueless on that, Travis is golden compared to this, I don't see any information on what platform it was executing...

danielkcz commented 8 years ago

Ah I could have scrolled down bit more :) Yea it's failing on 0.10 obviously and it will fail on 0.12 too ... http://node.green/#Object-static-methods

koresar commented 8 years ago

Wow! Hold off with this PR please! I'm running code on production using node 0.10. Please, do not break my production!

I'm somewhat disappointed we are rushing that much. I tend to close such PRs.

Would you elaborate what is going on here?

danielkcz commented 8 years ago

I am not rushing anything, I should probably put there [WIP] marker. Essentially I just wanted to see if it will work, it was quick change.

I suppose we would need to add manual polyfill there by checking if Object.assign exists although that still would mean that polyfilled could would have to be included in the package. That's kinda sad story...

koresar commented 8 years ago

Feel free to play with polyfills. Like

Object.assign = Object.assign || require('lodash/assign');
ericelliott commented 8 years ago

Feel free to play with polyfills. Like

Object.assign = Object.assign || require('lodash/assign');

This only works if the system you build on and the system you run it on are the same. );

koresar commented 8 years ago

Dammit! :)

danielkcz commented 8 years ago

This only works if the system you build on and the system you run it on are the same. );

When are you bundling on Node 4+ I don't think that require call can be actually tree-shaken. It will get included in bundle in any case. And if you are just running Node, it will be installed there for you to use. So I think we are good here unless I am missing some scenario.

danielkcz commented 8 years ago

Ok tests are passing with that polyfill, however instead of lodash I would go with this...

https://github.com/zloirock/core-js/blob/master/library/fn/object/assign.js

That's essentially what Babel is including so with some fingers crossed there wouldn't be multiple implementations in a code for someone bundling with Babel.

danielkcz commented 8 years ago

@ericelliott Just tried to build stampit with this polyfill included in compose.js and disabled transform-runtime. Yet it was included in bundle. So I am convinced that your statement is not true. Even if you bundle on platform that has support, this will be included for any cases it wont be available on executing platform.

stampit.full.zip

However if I enabled transform-runtime then as much as I am finger crossing there will be double implementation of this. In overall that would mean that using Object.assign is just bad idea. We should grab some implementation and use that directly. Closing this PR...