Should we publish a version 2?

Question

Should we publish a version 2?

TehShrike opened this issue 7 years ago · 10 comments

This module is pretty stable, but there are a few pieces of backwards-compatibility cruft that I would trim given the chance.

The default array merging algorithm is complicated and I don't think anyone could guess what it was without reading the code
By default, a new object is created for the output, but any child objects are moved right over instead of being cloned, unless you pass in 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 to true - 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:

Is cleaning up this cruft worth publishing a breaking change?
Is there any other cruft you can think of that could be cleaned up in a breaking change?

Answer 1 · 2017-08-31T15:58:21.000Z

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...

Answer 2 · 2017-08-31T16:09:19.000Z

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. ¯\_(ツ)_/¯

Answer 3 · 2017-09-04T02:55:10.000Z

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.

Answer 4 · 2017-09-13T12:30:05.000Z

+1
But, we should of course document the changes

Answer 5 · 2017-09-28T15:09:09.000Z

I think clone should default to true

Exactly this

Answer 6 · 2017-10-03T22:20:24.000Z

Agree strongly with all of your suggestions, they would make it much better

Answer 7 · 2017-10-04T03:35:13.000Z

I've opened a pull request - reviews of #77 would be appreciated!

Answer 8 · 2017-10-05T22:31:33.000Z

If there aren't any reviews, I'll merge/release next week.

Answer 9 · 2017-10-09T15:13:29.000Z

Version 2.0.0 has been published! Open new issues for anything you notice that seems off.

Answer 10 · 2017-10-10T21:47:58.000Z

I'll have the opportunity to put it through its paces pretty intensively soon, merging multiple nodes with complex models from a graph