search

multiformats / js-cid

CID implementation in JavaScript
MIT License
97 stars 39 forks source link

fix: replace node buffers with uint8arrays #117

Closed achingbrain closed 4 years ago

achingbrain commented 4 years ago

Relaxes input from requiring node Buffers to being Uint8Arrays.

This also means that the .buffer (now .byte) and .prefix properties have changed to be Uint8Arrays.

Depends on:

BREAKING CHANGES:

vmx commented 4 years ago

The CI failure is real, though you could also use https://github.com/Gozala/web-encoding/ instead of ipfs-utils.

achingbrain commented 4 years ago

you could also use Gozala/web-encoding instead of ipfs-utils.

I will if using ipfs-utils would prevent this from getting merged, but ipfs-utils already has the TextEncoder browser/node polyfill in it so it seems redundant.

Plus once https://github.com/ipfs/js-ipfs-utils/pull/55 is merged I can remove some of the duplicated utility functions here.

Gozala commented 4 years ago

I will if using ipfs-utils would prevent this from getting merged, but ipfs-utils already has the TextEncoder browser/node polyfill in it so it seems redundant.

Plus once ipfs/js-ipfs-utils#55 is merged I can remove some of the duplicated utility functions here.

For what it's worth we have discussed option of using ipfs-utils vs creating a web-encodings library and chose later to avoid pulling all the extra stuff that api-utils has. I'm fine with either option.

achingbrain commented 4 years ago

I've pulled out ipfs-utils in favour of uint8arrays which uses web-encoding internally.

vmx commented 4 years ago

@mikeal @rvagg I'd really like to have your approval on that change. Once merged I want to release this as 1.0.0.

mikeal commented 4 years ago

So, +1 in general, great to see this happening.

One concern I have taking a breaking change is just wasting the opportunity to do a bit more. Knowing that we have a more substantial CID change coming in the future it would be great to get some code into this change that will make that transition a bit easier.

For instance, we could add the .code property to CID and we could allow for instantiation using integers for the codec rather than just strings. This would allow people to migrate to using these newer forms even with the old API and should make the future transition to js-multiformats a bit easier.

Basically, we know that we need to move to using integers rather than strings for codec identifiers, it would be nice if we had a transitional release that accepted both so people could upgrade gradually or at least have the opportunity to do some of that migration when they migrate off of Buffer.

achingbrain commented 4 years ago

One concern I have taking a breaking change is just wasting the opportunity to do a bit more

I can see the temptation but I really think to deliver this change in a reasonable timeframe we should limit the scope to what's currently in this PR.

Gozala commented 4 years ago

For what it's worth I would also like to slot in asCID into 1.0. That said, I do not think we should block this pull request on other things we want in 1.0. We should instead consider having a meeting to make some decision about how to best roll out all the breaking changes.