search

chbrown / rfc6902

Complete implementation of RFC6902 in TypeScript
https://chbrown.github.io/rfc6902/
330 stars 39 forks source link

Copy operation removes the property at `from` #37

Closed enobufs closed 6 years ago

enobufs commented 6 years ago

I see no difference in copy operation from move. I believe, this line should be removed.

enobufs commented 6 years ago

I noticed this bug when I was trying to understand what your implementation of copy actually offers. Specifically, I wanted to figure out if the operation does a copy of reference or a deep copy. (then came across this bug)

If you intend to copy just a reference, then you would introduce a bunch of other (nasty) problems when from points to an object. (Imagine, modifying one of the properties of the object copied to/from elsewhere earlier.) The RFC 6902 does not state this issue clearly, but I would assume, deep-copy is the way to go. (removing the this line won't be good enough)

chbrown commented 6 years ago

Wow, pretty egregious copy & paste error there! Thanks for catching that; I (and other users) have certainly neglected the functionality that createPatch/diff never invokes, like the copy operation.

This makes a good argument for code coverage, too, since the whole patch.copy function lacked coverage!

Fixed in 8bb5334, by removing that line.

Your second point, about copying by value rather than reference, is a much deeper change. I should certainly be more systematic in the documentation about which functions treat data as mutable, or operate on references rather than values. In general, I agree that a deep copy would be preferable. Practically speaking, though, this library tends toward mutable over immutable, and reference rather than value; going immutable would require some pretty major API changes, and might (?) incur some heavy performance penalties (or at least require depending on Immutable.js or similar).