ipld / js-dag-cbor

dag-cbor codec for IPLD
Other
26 stars 17 forks source link

feat!: full type checks, generation and publishing #18

Closed rvagg closed 3 years ago

rvagg commented 3 years ago

Builds in #16

depends on https://github.com/rvagg/js-ipld-garbage/pull/1 but I can't pull it in as a git dependency because it's compiled into a dist/ before publishing (you can symlink the dist/ as node_modules/ipld-garbage for it to work here).

rvagg commented 3 years ago

resolved rvagg/js-ipld-garbage#1 as v3.0.0, all ✅ now

rvagg commented 3 years ago

@Gozala I'm trying to reconcile some of your comments in #16 with the interface we defined in https://github.com/multiformats/js-multiformats/blob/master/src/codecs/interface.ts in this PR, but the naming complicates nice export { ... } = codec({ ... }) so I've had to export a bunch of separate things. The typedef ends up like this:

/**
 * @template T
 * @param {T} node
 * @returns {Uint8Array}
 */
export function encode<T>(node: T): Uint8Array;
/**
 * @template T
 * @param {Uint8Array} data
 * @returns {T}
 */
export function decode<T>(data: Uint8Array): T;
/** @type {0x71} */
export const code: 0x71;
/** @type {'dag-cbor'} */
export const name: 'dag-cbor';
export const decoder: import("multiformats/codecs/interface").BlockDecoder<113, any>;
export const encoder: import("multiformats/codecs/interface").BlockEncoder<113, any>;
//# sourceMappingURL=index.d.ts.map

Is that going to be good enough for anything that says it wants a BlockCodec since it has all the pieces on the exports, they're just not nicely structured into a single object. If not, I'm wondering what the utility of BlockCodec ends up being?

Gozala commented 3 years ago

Is that going to be good enough for anything that says it wants a BlockCodec since it has all the pieces on the exports, they're just not nicely structured into a single object.

I would expect so, but might be worth double checking, just in case.

If not, I'm wondering what the utility of BlockCodec ends up being?

I’m not entirely sure anymore to be honest. Mainly BlokCodec was put for convenience so that function that needs to both encode and decode could write:

const foo<C, T>(codec: Codec<C, T>, ...

as opposed to

const foo<C, T>(codec: BlockEncoder<C, T> & BlockDecoder<C, T>...

But now that BlockCodec also has encoder and decoder properties two are no longer equivalent and less verbose one is inconvenient because you can no longer do

foo({ encode, decode }, ....)

as signature also requires encoder and decoder properties

rvagg commented 3 years ago

@Gozala here's a rewind of what we did for the latest js-multiformats changes (removed class Codec and in fixing types made BlockCodec too onerous to be useful): https://github.com/multiformats/js-multiformats/pull/75 - I think this gets us back to the place where we have everything we need and you can consume this package however you like.

I've also added to this PR a test/ts-use case that actually consumes the codec in 4 different ways to test the composability. I'd be grateful if you'd have a look (at test/ts-use/src/main.ts in particular). (Edit: they pass with the https://github.com/multiformats/js-multiformats/pull/75 branch's dist linked as multiformats but not with the current 6.0.0 version - they fail in the 4th case.)

rvagg commented 3 years ago

The tests will pass with the https://github.com/multiformats/js-multiformats/pull/75 branch's dist linked as multiformats but not with the current 6.0.0 version - they fail in the 4th case, which looks like this:

src/main.ts:20:17 - error TS2345: Argument of type 'BlockEncoder<113, any> & BlockDecoder<113, any>' is not assignable to parameter of type 'BlockCodec<113, string>'.
  Type 'BlockEncoder<113, any> & BlockDecoder<113, any>' is missing the following properties from type 'BlockCodec<113, string>': encoder, decoder

20   useBlockCodec(Object.assign({}, encoder, decoder))

so it should be catching that composaibility point of BlockCodec (although use-cases quite like this are dubious, but maybe there's something novel).

Gozala commented 3 years ago

so it should be catching that composaibility point of BlockCodec (although use-cases quite like this are dubious, but maybe there's something novel).

These are actually pretty useful in terms of object capabilities stuff, I might have a codec but I might want to call the function which just requires decoder. This allows you to avoid reconstructing pieces and just pass them around.

Argument could be made that maybe codec should be just { encoder, decoder } and I think there is a merit to that, but originally there was no separation of concerns / capabilities at all so that seemed like more difficult sell, which is why I thought three different interfaces and single object was a better compromise.

rvagg commented 3 years ago

updated to match latest, simplified form of the BlockCodec interface @ multiformats/js-multiformats#75

matches https://github.com/ipld/js-dag-pb/pull/6