-
Notifications
You must be signed in to change notification settings - Fork 18
Conversation
In Node.js the underlying hashing functions have not changed, but the browser now uses `webcrypto` instead of JavaScript based methods for `SHA1`, `SHA2-256` and `SHA2-512`. Also `SHA3` support was added in both Node.js and the browser. BREAKING CHANGE: The api was changed to be callback based, as webcrypto only exposes async methods. Closes #10
Changes Unknown when pulling dffc590 on better-crypto into * on master*. |
Changes Unknown when pulling b012472 on better-crypto into * on master*. |
Cool! Any good reason why to break the interface? We can support a callback API and a sync API at the same time. Can you add benchmarking tests to justify/motivate this change PS: Can we get that coveralls comments off? |
The dependency size reduction is massive, I will post numbers later. Speed wise I haven't tested it, but I expect good things. We can't have a sync interface on the browser because of webcrypto as we have seen before with my work libp2p-crypto and it would be very confusing when exposing one interface in node and another in the browser. I don't have the rights to turn the off on coveralls for some reason. |
Another important reason for making the api async and moving to webcrypto is that those avoid blocking the main thread, the current sync implementations will block everthing else, while webcrypto always gets executed on a non render thread. |
@diasdavid benchmarks are there, plus full coverage is back |
Changes Unknown when pulling e6667fb on better-crypto into * on master*. |
@dignifiedquire this is really, really great :D having a faster js-ipfs and libp2p (specially for testing, which means developer produtivity is awesome). However, as I mentioned in IRC, I don't feel confident of breaking the multihashing API, instead, let's create a For those of you reading this PR "Why create two modules and not have the two API in one?", because having the sync version would mean that we have to keep all the dependencies for the sync code to work, keeping the module really big. It is true that now with things like rollup, you can eliminate dead branches, but also means everyone has to understand and buy into that. |
I still don't understand why it is not enough to just bump the semver major version. If we create a second module we have to maintain it + backport fixes to this module including things like sha3. So this means a lot more work in the long time. In addition if others are still using this module and we use the async module in our code base others would have to bundle both versions into their code even increasing their bundles in size. |
multihashing and the other multiformats implementations should not be seen as a 'internal' of IPFS as they are used by a lot of other groups, that being said, we can't just break user space like that.
multihashing is incredibly small and straight forward compared to other modules we maintain, checking up the contribution graph, we can see that most changes have been of code refactoring or dependencies update, it is fairly stable at this point.
That is why documentation (examples, notes on the README) is king, informing our users that we've moved to multihashing-async because of X, Y and Z is more valuable than just flipping the API and implementation under the carpet. |
Create a new repo https://github.com/multiformats/js-multihashing-async and published to npm: https://www.npmjs.com/package/multihashing-async |
In Node.js the underlying hashing functions have not changed, but the browser now uses
webcrypto
instead of JavaScript based methods forSHA1
,SHA2-256
andSHA2-512
.Also
SHA3
support was added in both Node.js and the browser.BREAKING CHANGE:
The api was changed to be callback based, as webcrypto only exposes async methods.
Closes #10
Size Impact
953K
620K
328K
208K
Speed Impact
Buffer(100)
Buffer(10 * 1024)
Buffer(100 * 1024)
Due to the overhead, on very small buffers the pure JavaScript performs better. When looking at larger inputs it comes apparent that WebCrypto is a good deal faster.