js-multihashing-async icon indicating copy to clipboard operation
js-multihashing-async copied to clipboard

Support for plain array buffer

Open Gozala opened this issue 7 years ago • 5 comments

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

Gozala avatar Nov 09 '18 01:11 Gozala

@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.

Gozala avatar Aug 08 '19 23:08 Gozala

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.

mikeal avatar Aug 08 '19 23:08 mikeal

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

Gozala avatar Aug 09 '19 00:08 Gozala

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’).

mikeal avatar Aug 09 '19 00:08 mikeal

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.

Gozala avatar Aug 09 '19 02:08 Gozala