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

fix: replace node buffers with uint8arrays #117

Merged
merged 2 commits into from
Aug 4, 2020
Merged

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Jul 30, 2020

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:

  • node Buffers have been replaced with Uint8Arrays
  • the .buffer property has been renamed to .bytes and is now a Uint8Array
  • the .prefix property is now a Uint8Array

Copy link
Member

@vmx vmx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks the changes look good!

Would you mind updating the README to not use Buffer, to indicate the Uint8Arrays are the way to go? I think using the global TextEncoder/Decoder is fine in a README example.

@vmx
Copy link
Member

vmx commented Jul 30, 2020

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

@achingbrain
Copy link
Member Author

achingbrain commented Jul 30, 2020

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 ipfs/js-ipfs-utils#55 is merged I can remove some of the duplicated utility functions here.

Copy link
Contributor

@Gozala Gozala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome! I just would like to use this opportunity to rename .buffer property:

  1. So it better reflects what it containts
  2. So that code expecting older CID breaks early rather than later.
  3. So we could in the future make CIDs ArrayBufferViews in which case we'd need buffer property to point to the underlying ArrayBuffer instead.

README.md Outdated
@@ -142,19 +158,19 @@ Property containing the CID version integer.

#### cid.multihash

Property containing the multihash buffer.
Property containing the multihash `Uint8Array`.

#### cid.multibaseName

Property containing the default base to use when calling `.toString`

#### cid.buffer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should rename this property, so changes are more obvious and name isn't misleading.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It makes me wince a little, because it's going to be very disruptive, but I've renamed it to .bytes.

src/index.d.ts Outdated Show resolved Hide resolved
src/index.js Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
test/index.spec.js Outdated Show resolved Hide resolved
@Gozala
Copy link
Contributor

Gozala commented Jul 30, 2020

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
Copy link
Member Author

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

@achingbrain achingbrain requested review from vmx and Gozala July 31, 2020 13:28
@vmx vmx requested review from rvagg and mikeal July 31, 2020 14:01
@achingbrain achingbrain changed the title fix: support uint8arrays fix: replace node buffers with uint8arrays Jul 31, 2020
@achingbrain achingbrain force-pushed the fix/support-uint8arrays branch 2 times, most recently from af9dea1 to 5c00d18 Compare August 3, 2020 12:18
Relaxes input from requiring node `Buffer`s to being `Uint8Arrays`.

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

BREAKING CHANGES:

- node `Buffer`s have been replaced with `Uint8Array`s
- the `.buffer` property has been renamed to `.bytes` and is now a `Uint8Array`
- the `.prefix` property is now a `Uint8Array`
@vmx
Copy link
Member

vmx commented Aug 3, 2020

@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
Copy link
Contributor

mikeal commented Aug 3, 2020

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 added a commit to ipld/js-ipld-raw that referenced this pull request Aug 3, 2020
Depends on:

- [ ] multiformats/js-cid#117

BREAKING CHANGE:

- `util.serialize` now returns a Uint8Array
@achingbrain
Copy link
Member Author

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.

Copy link
Member

@rvagg rvagg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm +1 on this but also +1 on Mikeal's ask, so see #118 for that piece of the puzzle which would be nice to slot in prior to a release of this if we can get agreement on that addition.

@vmx vmx merged commit a7ae250 into master Aug 4, 2020
@vmx vmx deleted the fix/support-uint8arrays branch August 4, 2020 11:49
achingbrain added a commit to ipld/js-ipld-ethereum that referenced this pull request Aug 4, 2020
This module now accepts Uint8Arrays as well as node Buffers and
returns Uint8Arrays.  Internally it converts non-Buffers into Buffers
because the ethereum libs require that.

BREAKING CHANGES:

- `util.serialize` returns a `Uint8Array`
- `util.cid` returns `CID`s with a breaking API change - see multiformats/js-cid#117 for changes
achingbrain added a commit to ipld/js-ipld-git that referenced this pull request Aug 4, 2020
returns Uint8Arrays.  Internally it converts non-Buffers into Buffers
because the ethereum libs require that.

BREAKING CHANGES:

- `util.serialize` returns a `Uint8Array`
- `util.cid` returns `CID`s with a breaking API change - see multiformats/js-cid#117 for changes
achingbrain added a commit to ipld/js-ipld-git that referenced this pull request Aug 4, 2020
This module now accepts Uint8Arrays as well as node Buffers and
returns Uint8Arrays.  Internally it converts non-Buffers into Buffers
because the ethereum libs require that.

BREAKING CHANGES:

- `util.serialize` returns a `Uint8Array`
- `util.cid` returns `CID`s with a breaking API change - see multiformats/js-cid#117 for changes
achingbrain added a commit to ipld/js-ipld-zcash that referenced this pull request Aug 4, 2020
BREAKING CHANGES:

- `util.cid` returns `CID`s with a breaking API change - see multiformats/js-cid#117 for changes
achingbrain added a commit to ipld/js-ipld that referenced this pull request Aug 4, 2020
Removes use of node Buffer in favour of Uint8Arrays and updates
all dependencies to versions that do the same.

BREAKING CHANGES:

- CIDs returned from `put*` methods have breaking API changes
  See see multiformats/js-cid#117 for changes
vmx pushed a commit to ipld/js-ipld-git that referenced this pull request Aug 4, 2020
This module now accepts Uint8Arrays as well as node Buffers and
returns Uint8Arrays.  Internally it converts non-Buffers into Buffers
because the ethereum libs require that.

BREAKING CHANGES:

- `util.serialize` returns a `Uint8Array`
- `util.cid` returns `CID`s with a breaking API change - see multiformats/js-cid#117 for changes
vmx pushed a commit to ipld/js-ipld-zcash that referenced this pull request Aug 4, 2020
BREAKING CHANGES:

- `util.cid` returns `CID`s with a breaking API change - see multiformats/js-cid#117 for changes
vmx pushed a commit to ipld/js-ipld-bitcoin that referenced this pull request Aug 4, 2020
Accepts and emits `Uint8Array`s instead of node `Buffer`s but converts
them internally because `bitcoinjs-lib` only understands `Buffer`s.

BREAKING CHANGE:

- `util.serialize` returns `Uint8Array`s
- `util.cid` returns `CID`s with a breaking API change - see multiformats/js-cid#117 for changes
vmx pushed a commit to ipld/js-ipld-ethereum that referenced this pull request Aug 4, 2020
This module now accepts Uint8Arrays as well as node Buffers and
returns Uint8Arrays.  Internally it converts non-Buffers into Buffers
because the ethereum libs require that.

BREAKING CHANGES:

- `util.serialize` returns a `Uint8Array`
- `util.cid` returns `CID`s with a breaking API change - see multiformats/js-cid#117 for changes
@Gozala
Copy link
Contributor

Gozala commented Aug 4, 2020

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.

vmx pushed a commit to ipld/js-ipld that referenced this pull request Aug 5, 2020
Removes use of node Buffer in favour of Uint8Arrays and updates
all dependencies to versions that do the same.

BREAKING CHANGES:

- CIDs returned from `put*` methods have breaking API changes
  See see multiformats/js-cid#117 for changes
achingbrain added a commit to ipfs/js-ipfs-unixfs that referenced this pull request Aug 5, 2020
Follow up to #66 that removes node Buffers from the tests and uses
version of protons that returns uint8arrays.

BREAKING CHANGES:

- All use of node Buffers have been replaced with Uint8Arrays
- CIDs exported by this module have breaking API changes, see: multiformats/js-cid#117
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants