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

amp-merge and lodash.merge #94

Open
rtorr opened this issue May 13, 2015 · 4 comments
Open

amp-merge and lodash.merge #94

rtorr opened this issue May 13, 2015 · 4 comments

Comments

@rtorr
Copy link
Contributor

rtorr commented May 13, 2015

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
Copy link

jdalton commented May 13, 2015

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
Copy link

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
Copy link

jdalton commented May 14, 2015

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
Copy link
Contributor Author

rtorr commented May 15, 2015

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.

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

No branches or pull requests

3 participants