-
Notifications
You must be signed in to change notification settings - Fork 216
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
Should we publish a version 2? #72
Comments
So long as we clearly document the change at the top of the npm page + provide an example of how users can easily switch back to the old behavior I don't see too much of a problem. obviously cloning would be a bad way of dealing with certain things because people may not expect it... for example a user using a object based module system probably does not want their modules cloned as calling methods from the cloned vs uncloned objects would have different internal state... |
Yeah, that intuitively seems like a change that would bite some people. I'd be more worried about it, except nobody seems to have ever been bothered by the root object always being freshly created every time, which seems like the case that would matter most. |
I could see a lot more people using the library on objects that contain classes vs on classes themselves, but I definitely agree that cloning everything is probably a better way to avoid unexpected behavior in the majority of cases. Along with clear documentation on how to reverse the behavior I see no problem with it. |
+1 |
Exactly this |
Agree strongly with all of your suggestions, they would make it much better |
I've opened a pull request - reviews of #77 would be appreciated! |
If there aren't any reviews, I'll merge/release next week. |
Version 2.0.0 has been published! Open new issues for anything you notice that seems off. |
I'll have the opportunity to put it through its paces pretty intensively soon, merging multiple nodes with complex models from a graph |
This module is pretty stable, but there are a few pieces of backwards-compatibility cruft that I would trim given the chance.
clone: true
Since you can pass in custom array merge functions, I would rather default to the super-simple method of just concatenating arrays and let people do more complicated things on their own if they need to.
I think
clone
should default totrue
- I think it's what people would most likely expect, and it's the safer option.We could even consider deprecating the
clone
option and eventually supporting only deep clones. This seems doable because nobody has apparently had any issue with the fact that a new object is always created for the output already.If we always cloned, it would also free us up to make
merge.all
a bit more forgiving, ala #71.So:
The text was updated successfully, but these errors were encountered: