flitbit / diff

Javascript utility for calculating deep difference, capturing changes, and applying changes across objects; for nodejs and the browser.
MIT License
3k stars 214 forks source link

ApplyChanges troubles #89

Closed suni-masuno closed 7 years ago

suni-masuno commented 7 years ago

I've encountered a few problems attempting to use deep.applyChanges() and I'm looking for reasons why/providing feedback.

1-why does it take a second argument that's unused? in the code I noticed this argument is called source, but it isn't used by this function at any time and in any way. It confused me for ages. 2-why does it take a single change, when changes are provided by diff as an array? 3-why does it take an argument that it mutates? I suppose this is reasonably common, I just would like to argue for taking in an origin argument that isn't modified and then producing a return value of the origin with the diff(s) applied instead. 4-could I get a few lines on how to call applyChanges in the documentation? took a fair bit of reading the source code to figure the above out.

Please don't take this as an attack, I dig the library just felt the responsibility to provide feedback describing my troubles.

flitbit commented 7 years ago

Your answers, for what its worth:

1) At some time I may have had a reason to pass both target and source, but nowadays it appears to be what is stuck on the wall, and I don't know why. 1) I wrote applyChange as a utility function, you'll notice in an example here and there that I use observableDiff; my own code elsewhere leverages this to do interesting things with individual changes... it is rather easy to adapt the one to many but not so the many to one. 1) I understand and support your argument since I prefer immutables. I couldn't find a fast way to do so without dependencies, which was was originally important because module loading was uncommon in browsers as well as non-existent in Riak's javascript map/reduce so I needed it to function standalone. 1) Updating the readme soon.

Cheers.