Skip to content
This repository has been archived by the owner on Aug 24, 2021. It is now read-only.

Support for plain array buffer #33

Open
Gozala opened this issue Nov 9, 2018 · 5 comments
Open

Support for plain array buffer #33

Gozala opened this issue Nov 9, 2018 · 5 comments

Comments

@Gozala
Copy link
Contributor

Gozala commented Nov 9, 2018

It would be very convenient to not have to convert everything into node style Buffer but rather allow use of ArrayBuffer

@Gozala
Copy link
Contributor Author

Gozala commented Aug 8, 2019

@hugomrdias I'm willing to work on a pull request that removes dependency on buffer in favor of Uint8Array but I'd like to make sure that changes would be accepted before I invest time into it.

@mikeal
Copy link
Contributor

mikeal commented Aug 8, 2019

IMO, the APIs should accept any binary type or view and just convert to a Buffer internally if necessary.

I’d love it if we also never returned Buffer instances, but that would be a much larger breaking change :( The problem is that one of the most common things people do with the multihash buffer is call .toString(‘base64’) which of course will break if we started returning anything but a Buffer instance.

@Gozala
Copy link
Contributor Author

Gozala commented Aug 9, 2019

IMO, the APIs should accept any binary type or view and just convert to a Buffer internally if necessary

I actually would like to get rid off Buffer dependency which is annoying chunk of code in browser.

I'm also reluctant to accept any binary type proposition, it seems like a good idea but in practice it comes with mental overhead (requiring side trips) to know what's being passed around. I'd much rather choose canonical representation and point users to adapters if they happen to need different one.

It is also not obvious to me what the desired behavior is if e.g. Float64Array instance has being provided. Sure we can just read underlying bytes from the corresponding buffer slice, but in practice I think it's more error prone than helpful.

I’d love it if we also never returned Buffer instances, but that would be a much larger breaking change :( The problem is that one of the most common things people do with the multihash buffer is call .toString(‘base64’) which of course will break if we started returning anything but a Buffer instance.

I actually implied getting rid of Buffer although I guess that is probably better set aside for next iteration. The way I was thinking to go about is to pull out Buffer free core into separate package and use this as a wrapper that just wraps returns into Buffer. Then we could go after each dependency and migrate them to Buffer free core.

For base64 conversion we could have separate module just like we do for base56

@mikeal
Copy link
Contributor

mikeal commented Aug 9, 2019

I actually implied getting rid of Buffer although I guess that is probably better set aside for next iteration. The way I was thinking to go about is to pull out Buffer free core into separate package and use this as a wrapper that just wraps returns into Buffer. Then we could go after each dependency and migrate them to Buffer free core.

I like this approach, it lets us maintain compatibility while opening up a better option for people that want it. It doesn’t even need to be another module, it could just be something like require(‘multihashing-async/core’).

@Gozala
Copy link
Contributor Author

Gozala commented Aug 9, 2019

It doesn’t even need to be another module, it could just be something like require(‘multihashing-async/core’).

The reason I thought it was better to go with separate package is so we can track how many packages still depending on buffer wrapped version (through npm stats). If it’s same package it would be a lot harder to tell / migrate.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants