multiformats / js-multiformats

Multiformats interface (multihash, multicodec, multibase and CID)
Other
225 stars 51 forks source link

block.bytes can be a nodejs Buffer #191

Open tabcat opened 2 years ago

tabcat commented 2 years ago

Would like to know if this is the desired behavior:

Block = await import('multiformats/block')
codec = await import('@ipld/dag-cbor')
{ sha256: hasher } = await import('multiformats/hashes/sha2')

bytes = Buffer.from([ 100, 97, 115, 100, 102 ])
block = await Block.decode({ bytes, codec, hasher })

block.bytes instanceof Buffer // true

Noticed this while testing with the ipfs block api in node. Buffer extends Uint8Array which is great, but it seems to override some of the characteristics of Uint8Array.

rvagg commented 1 year ago

mm, I tend to not mind and just deal with them as if they are Uint8Array and in almost all cases it doesn't matter either way (except when you're optimising for perf .. where it matters quite a bit!). The contract says you should expect a Uint8Array, which is true, but proper is certainly is a worthy goal here, especially since these APIs can be implemented by a wide variety of implementations.

Would you like to submit a PR to address this? We can just extract the underlying .buffer and use that and not suffer a copy penalty.

tabcat commented 1 year ago

this line in the Block constructor seems like the best place to add it. i'm feeling wishy/washy, it might be good to make this change if ipfs ever starts only returning Uint8Array. if js-ipfs doesnt use Buffer instance methods currently (not sure if it does or not); why not return Uint8Array everywhere?