Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: support decoding ArrayBuffers #287

Merged
merged 1 commit into from
Feb 15, 2024

Conversation

achingbrain
Copy link
Member

@achingbrain achingbrain commented Feb 14, 2024

When using fetch to download something over HTTP, it only makes an ArrayBuffer available, not a Uint8Array.

TextDecoder supports decoding ArrayBuffers, and the raw codec already turns passed ArrayBuffers into Uint8Arrays so this is just a type change.

Instead of:

const res = await fetch('...')
const obj = json.decode(new Uint8Array(await res.arrayBuffer()))

we can do:

const res = await fetch('...')
const obj = json.decode(await res.arrayBuffer())

When using `fetch` to download something over HTTP, it only makes
an `ArrayBuffer` available, not a `Uint8Array`.

`TextDecoder` supports decoding `ArrayBuffer`s, and the `raw` codec
turns passed `ArrayBuffer`s into `Uint8Array`s so this is just a
type change.

Instead of:

```js
const res = await fetch('...')
const obj = json.decode(new Uint8Array(await res.arrayBuffer()))
```

we can do:

```js
const res = await fetch('...')
const obj = json.decode(await res.arrayBuffer())
```
/**
* Similar to ByteView but extends ArrayBuffer.
*/
export interface ArrayBufferView<Data> extends ArrayBuffer, Phantom<Data> {}

Choose a reason for hiding this comment

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

just confirming that this is exported from index.ts:

export * from './interface.js'

and

export * from './bases/interface.js'
export * from './hashes/interface.js'
export * from './codecs/interface.js'
export * from './link/interface.js'
export * from './block/interface.js'

}

/**
* An IPLD codec is a combination of both encoder and decoder.
*/
export interface BlockCodec<Code extends number, T> extends BlockEncoder<Code, T>, BlockDecoder<Code, T> {}

export type { ByteView }
export type { ArrayBufferView, ByteView }

Choose a reason for hiding this comment

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

We are exporting this type twice from two separate files.

  1. src/blocks/interface -> src/codecs/interface -> src/interface
  2. src/blocks/interface -> src/interface

Do we need a central location or will TS handle that well?

Copy link
Member Author

Choose a reason for hiding this comment

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

TS handles it fine, but it makes linking to a canonical documentation location in generated TypeDocs a little more fiddly.

Ref: TypeStrong/typedoc#2125 and https://github.com/ipfs/aegir/blob/master/src/docs/type-indexer-plugin.js#L121-L128.

@SgtPooki
Copy link

do we want to update https://github.com/ipld/js-dag-json/blob/1e797391539237ab1738dbf60fe9d0a92278d88d/src/index.js#L249 and others as well?

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.

seems ok to me, the | makes it less elegant but I can't think of how you'd reduce that without making it really gnarly

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.

I personally prefer to keep interface as simple as possible and add all the sugar in the layers above that way not each implementation has to deal with the variability it’s just becomes only concern of the layer above which will handle across all codecs.

That said I don’t want to stand in the way of this if there’s a consensus around it.

P.S.: uint8array was chosen over array buffer because you can reference slice within buffer and you can’t do same with later without copying

@achingbrain
Copy link
Member Author

the | makes it less elegant

You could make ByteView a union type, but that'd be a breaking change that doesn't seem worth the pain to me.

I personally prefer to keep interface as simple as possible and add all the sugar in the layers above

I agree with the design principle but here there is no layer above. The IPLD specs say to use these modules, yet we then force users to create extra Uint8Arrays which are superfluous since raw coerces the input to Uint8Array anyway, and json passes the input off to JSON.parse which supports both ArrayBuffers and Uint8Arrays.

uint8array was chosen over array buffer because you can reference slice within buffer and you can’t do same with later without copying

Not all codecs will need this but if they do it's simple enough to wrap the ArrayBuffer in a Uint8Array and use .subarray internally. The DX to require all users to do this seems arbitrary when it's potentially unnecessary.

That said I don’t want to stand in the way of this if there’s a consensus around it.

🙏

@achingbrain achingbrain merged commit e7f3272 into master Feb 15, 2024
20 checks passed
@achingbrain achingbrain deleted the feat/support-decoding-arraybuffer branch February 15, 2024 14:08
github-actions bot pushed a commit that referenced this pull request Feb 15, 2024
## [13.1.0](v13.0.1...v13.1.0) (2024-02-15)

### Features

* support decoding ArrayBuffers ([#287](#287)) ([e7f3272](e7f3272))

### Trivial Changes

* Update .github/dependabot.yml [skip ci] ([aa9c730](aa9c730))
Copy link

🎉 This PR is included in version 13.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants