- 
                Notifications
    
You must be signed in to change notification settings  - Fork 54
 
feat: support decoding ArrayBuffers #287
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
Conversation
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> {} | 
There was a problem hiding this comment.
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:
Line 210 in aa9c730
| export * from './interface.js' | 
and
js-multiformats/src/interface.ts
Lines 1 to 5 in aa9c730
| 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' | 
| export interface BlockCodec<Code extends number, T> extends BlockEncoder<Code, T>, BlockDecoder<Code, T> {} | ||
| 
               | 
          ||
| export type { ByteView } | ||
| export type { ArrayBufferView, ByteView } | 
There was a problem hiding this comment.
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.
src/blocks/interface -> src/codecs/interface -> src/interfacesrc/blocks/interface -> src/interface
Do we need a central location or will TS handle that well?
There was a problem hiding this comment.
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.
| 
           do we want to update https://github.com/ipld/js-dag-json/blob/1e797391539237ab1738dbf60fe9d0a92278d88d/src/index.js#L249 and others as well?  | 
    
There was a problem hiding this 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
There was a problem hiding this 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
          
 You could make  
 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  
 Not all codecs will need this but if they do it's simple enough to wrap the  
 🙏  | 
    
| 
           🎉 This PR is included in version 13.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀  | 
    
When using
fetchto download something over HTTP, it only makes anArrayBufferavailable, not aUint8Array.TextDecodersupports decodingArrayBuffers, and therawcodec already turns passedArrayBuffers intoUint8Arrays so this is just a type change.Instead of:
we can do: