feat: support cloning buffers

[salmanm]

Fixes https://github.com/davidmarkclements/rfdc/issues/18

[salmanm]

Thanks @feugy.

I thought about clone(Buffer.from('whatever') but couldn't decide because while recursing, there is no way this function would be called with a buffer. So adding an extra check in clone (which is a recursive function) only to support this case could be an overhead while recursing.

I understand it's debatable, perhaps we need a third opinion. 🙂

[simoneb]

@salmanm @feugy I think they point is more about whether cloning something that is not an object, meaning the argument that's provided to the main exported clone function, is supported already. If it is, I believe the operation mentioned by @feugy should be supported, but I'm not sure if that's how it currently works or if you can only provide an object as input (and a few other types including dates).

@salmanm in this PR you'll also need to change README to mention that Buffers are now supported.

[salmanm]

whether cloning something that is not an object is already supported

As per README, the function signature says clone(obj) => obj2. Does that count as only objects can be cloned? 😉

[simoneb]

whether cloning something that is not an object is already supported

As per README, the function signature says clone(obj) => obj2. Does that count as only objects can be cloned? 😉

It does not in my opinion, because even though that's what the readme says, the code says otherwise.

[codecov[bot]]

Codecov Report

Merging #19 (f6f344f) into master (5e87a45) will not change coverage. The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master       #19   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines           97       112   +15     
=========================================
+ Hits            97       112   +15     

Impacted Files	Coverage Δ
index.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 5e87a45...f6f344f. Read the comment docs.

[salmanm]

Quick question for maintainers: The README shows how Uint8Array get's messed up while cloning. Do you see any reason why I should not fix that too as part of this PR?

[davidmarkclements]

Great efforts here everyone - @salmanm would you be okay to extend this for all binary types. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Typed_arrays

For best speed I’d check if there’s a buffer property that’s an instance of ArrayBuffer (o.buffer instanceof ArrayBuffer) and then use the slice method on the buffer property to clone the array buffer - then pass that into the constructor method of the thing being cloned (new o.constructor(o.buffer.slice())

Something like that with some tweaks should work for all typed array views but node Buffers still need to be special cased (new Buffer is deprecated) so I’m fine to keep the current Buffer.from code as is and then do the type array checks under that

[salmanm]

Updated the PR @davidmarkclements. Used ArrayBuffer.isView instead of checking o.buffer instanceof. Let me know if it may have any consequence I missed.

[salmanm]

Added a complex case of cloning ArrayBuffers in https://github.com/davidmarkclements/rfdc/pull/19/commits/20cab98a7b5a42dc4ca27c33a2cffcb2af149d16

[salmanm]

@davidmarkclements could you please take a look?

[davidmarkclements]

awesome work @salmanm I'm good with the approach, it looks good. One final thing: can you please update the readme to reflect this change

[salmanm]

@davidmarkclements updated the doc.

[davidmarkclements]

great job @salmanm - thank you!