-
Notifications
You must be signed in to change notification settings - Fork 12
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
Comments
Immutable |
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).
One instinct I have is 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 I dunno, naming things is hard. :) Like @rtorr said, when we initially wanted something like |
The name As for naming, lodash has deep forms of methods like |
I think Some other ideas (and could add
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. |
I was going to update some libraries using
amp-merge
in favor of the suggestedlodash.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 ofamp-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#L22I 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?
The text was updated successfully, but these errors were encountered: