js-multiformats icon indicating copy to clipboard operation
js-multiformats copied to clipboard

Remove `cid.buffer` in favor of `cid.bytes.buffer` to avoid issues with node libraries

Open Gozala opened this issue 4 years ago • 1 comments

https://github.com/multiformats/js-multiformats/pull/29 repurposed former buffer: Buffer property to buffer: ArrayBuffer property that as @mikeal points out can lead to issues in node land that would happily accept either and mutate it.

This means that it could lead to issues when cid.byteOffset > 0 || cid.byteLength !== cid.buffer.byteLength. To avoid these problems we can rename .buffer to .arrayBuffer instead.

Gozala avatar Aug 07 '20 18:08 Gozala

@mikeal I recall your primary argument was that lot of node utils would take either and would write into ArrayBuffer just as if it was Buffer. However now that I think about it, writing into a cid.buffer would create whole class of other issues, so I'm no longer sue this is important.

I think we have also considered alternative of keeping buffer: ArrayBuffer and throwing if cid represents a view into slice of the ArrayBuffer. This got me thinking about following:

  • We could emit warning and return a slice of this.arrayBuffer.slice(this.byteOffset, this.byteLength) in such case.
    • That would alleviate issues with node libraries.
    • It is also worth pointing out that CID will only be view into slice of ArrayBuffer when constructor is used instead of factory.

Gozala avatar Aug 07 '20 18:08 Gozala