AmpersandJS / amp

a collection of individual JS utility modules
http://amp.ampersandjs.com
MIT License
130 stars 12 forks source link

amp-merge and lodash.merge #94

Open rtorr opened 9 years ago

rtorr commented 9 years ago

I was going to update some libraries using amp-merge in favor of the suggested lodash.merge. Once finally looking at the two methods, they expect different inputs, and give different outputs. You can make them work the same, but the idea of amp-merge came out of laziness and not wanting to confuse using _.extend({}, whatever) in our code. This also replicates the now depreciated helper method that was included in react around version 0.10. You can see it's use here https://github.com/facebook/flux/blob/09309a4aa4284e43b20737c3b7cd88ff9439ce89/examples/flux-todomvc/js/stores/TodoStore.js#L22

I can continue to use amp-merge as it exists today, however the documentation suggests using lodash. Is @jdalton interested in a new method with this use case?

jdalton commented 9 years ago

Immutable assign is something I've heard more call for recently but is not on our roadmap at the moment. I'm not against adding an immutable assign and merge so bikeshedding a name would help.

mattwondra commented 9 years ago

I've been racking my brain on this a bit. Is there any prior art for this kind of function? Namely a non-destructive merging of objects? I looked through Immutable.js and didn't see anything that does quite what we wanted, and I can't find any standard JS functions either (though I'm admittedly not greatly familiar with the ES6/7 specs).

merge(a, b, c) makes sense in my mind as "Merge all the arguments together and return the result" (non-destructive). But I can see how others would think of it as "Merge everything into a" (destructive). And since lo-dash takes it as the latter, that ship has probably sailed.

One instinct I have is combine(a, b, c) — does that read as "Combine these objects together and return the result"? IMO it's not as clear as the term merge especially when it comes to how property conflicts will be handled, but maybe that's not a huge concern.

The other thought I had is that sticking to a more passive name (instead of an action-y name) could make it clearer that we don't intend to actually change any of the arguments. Something like union(a, b, c) maybe? "Return the union of these three objects."

I dunno, naming things is hard. :) Like @rtorr said, when we initially wanted something like amp-merge it was just as a convenience method to help prevent us from doing something stupid like extend(a, b, c) in a context where we actually didn't want to modify a. So any solution that helps solve that specific use case would be wonderful.

jdalton commented 9 years ago

The name assign aligns with Object.assign. So I see it as merge will attempt to merge values while assign/extend will assign values (overwriting previous values).

As for naming, lodash has deep forms of methods like cloneDeep and flattenDeep. Maybe something like assignImmutable (long gross name but you get the idea).

rtorr commented 9 years ago

I think assignImmutable sounds great (although long).

Some other ideas (and could add deep):

spawn mix

I think the term spawn creates the feeling that we produce a new object. That being said, the object it produces/returns is (unfortunately) not immutable, so that could be confusing if we introduce that name. That could be something I would love to have in the future though. Naming is hard.