search

denoland / std

The Deno Standard Library
https://jsr.io/@std
MIT License
3.18k stars 623 forks source link

Review the std/hash Hasher interface #652

Closed caspervonb closed 3 years ago

caspervonb commented 4 years ago

The Hasher interface defined in std/hash might warrant a review.

First thing I noticed was that Hasher.digest returns an ArrayBuffer, This is more or less the only interface we have that doesn't return an Uint8Array view of the generated ArrayBuffer.

Other concerns, @kitsonk ?

caspervonb commented 4 years ago

Actually, just noticed that the different "legacy" algorithms in std/hash dont even follow the same interface. Sha1.digest returns a number[] array, while Md5.digest returns an ArrayBuffer; the whole thing is quite the mess.

kitsonk commented 4 years ago

Yeah, looking at the WebAssembly implementation, that API is "right".

The reason why it was a mess is its legacy, and no one being specific about how we got there... SHA1 was there first, and it was a port. SHA256 I ported from the same author and kept the same APIs. SHA512 and variants was different but I called out we needed to align, so it mostly aligns. MD5 snuck in there, then along you came with the WebAssembly and we didn't specifically talk about the API there before we added it.

Seeing that it is factory function based, instead of class based, that .digest() returns an array buffer and .toString() returns a hex or base64 string, that is all the functionality that I think needs to be there. So I don't see any need to change the existing APIs.

I guess the debate is, is it worth taking the breaking change? Looking at it, once we have denoland/deno_std#653, I would say yeah, let's just get on with it. It will break people, but as you point out, the current "legacy" stuff is a mess and has APIs that are just "silly".

stale[bot] commented 3 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

caspervonb commented 3 years ago

Leaving this for std-wg in a couple of weeks; and since webcrypto is inbound std/hash this might just become deprecated.

jeremyBanks commented 3 years ago

I have proposed an expanded interface: