Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

block.bytes can be a nodejs Buffer #191

Open
tabcat opened this issue Jul 23, 2022 · 2 comments
Open

block.bytes can be a nodejs Buffer #191

tabcat opened this issue Jul 23, 2022 · 2 comments
Assignees
Labels
P2 Medium: Good to have, but can wait until someone steps up

Comments

@tabcat
Copy link
Contributor

tabcat commented Jul 23, 2022

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
Copy link
Member

rvagg commented Jul 26, 2022

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.

@rvagg rvagg moved this to 🥞 Todo in IPLD team's weekly tracker Jul 26, 2022
@rvagg rvagg self-assigned this Jul 26, 2022
@tabcat
Copy link
Contributor Author

tabcat commented Jul 26, 2022

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?

@rvagg rvagg added the P2 Medium: Good to have, but can wait until someone steps up label Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 Medium: Good to have, but can wait until someone steps up
Projects
None yet
Development

No branches or pull requests

2 participants