search

cujojs / jiff

JSON Patch and diff based on rfc6902
Other
627 stars 41 forks source link

use JSON.stringify as default hash for all non-primitive array elements #33

Closed EyalAr closed 8 years ago

EyalAr commented 8 years ago

following discussion at #32

@briancavalier this was an easy enough change. Please feel free to reject if you don't agree with my suggestion.

briancavalier commented 8 years ago

@EyalAr, thanks for this! At first glance, it looks good. I'll take a closer look tonight.

briancavalier commented 8 years ago

Looks good, @EyalAr! I appreciate the additional unit test coverage too :) I'm debating whether this would be considered a breaking change, i.e. for semver implications. Any thoughts?

EyalAr commented 8 years ago

Good question... There are no API changes, and no new functionality. Non of the changes break old behavior of the public API. Even though generated diffs may have different structure from before, they still have the same effect on patched objects. That said, if someone depends on the specific structure of the diff for some reason (say, different logic if the patch is empty or not), then this would definitely be a breaking change for them. I think that since this library implements a standard, we don't have to guarantee a certain structure for diffs, but only guarantee the effect on patched objects. Nothing was broken in that regard. And since nothing changed in the public API, in my opinion this warrants only a patch version bump.

briancavalier commented 8 years ago

Sorry @EyalAr, I had a busy week and just now getting back to this. That logic makes sense to me. And as you've pointed out, this could be considered a bug. So, I agree: let's make it a patch version. I'll merge this and do the release.

Thanks again!

EyalAr commented 8 years ago

No worries. Thanks for the merge and for your time on this.