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!: full type checks, generation and publishing #18

Merged
merged 1 commit into from
Apr 26, 2021

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Apr 7, 2021

Builds in #16

depends on ipld/js-ipld-garbage#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
Copy link
Member Author

rvagg commented Apr 7, 2021

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

@rvagg
Copy link
Member Author

rvagg commented Apr 7, 2021

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

Gozala commented Apr 7, 2021

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

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.

I've tried that as par of my unixfs effort and it passed the type checking as expected.

rvagg added a commit to multiformats/js-multiformats that referenced this pull request Apr 8, 2021
This is what we probably should have done to solve the conflict in cea9063
because we ended up with a `BlockCodec` that has too many properties and is
therefore not very useful.

This makes `BlockCodec` just an implementer of both `BlockEncoder` and
`BlockDecoder` but `codec()` returns you that, plus `encoder` and `decoder`
properties so you have all the things and can consume them however you like.

Ref: ipld/js-dag-cbor#18 (comment)
rvagg added a commit to multiformats/js-multiformats that referenced this pull request Apr 8, 2021
This is what we probably should have done to solve the conflict in cea9063
because we ended up with a `BlockCodec` that has too many properties and is
therefore not very useful.

This makes `BlockCodec` just an implementer of both `BlockEncoder` and
`BlockDecoder` but `codec()` returns you that, plus `encoder` and `decoder`
properties so you have all the things and can consume them however you like.

Ref: ipld/js-dag-cbor#18 (comment)
rvagg added a commit to multiformats/js-multiformats that referenced this pull request Apr 8, 2021
This is what we probably should have done to solve the conflict in cea9063
because we ended up with a `BlockCodec` that has too many properties and is
therefore not very useful.

This makes `BlockCodec` just an implementer of both `BlockEncoder` and
`BlockDecoder` but `codec()` returns you that, plus `encoder` and `decoder`
properties so you have all the things and can consume them however you like.

Ref: ipld/js-dag-cbor#18 (comment)
@rvagg rvagg force-pushed the rvagg/esm-ts-build-improvements branch from 42b163e to cc73645 Compare April 8, 2021 05:07
@rvagg
Copy link
Member Author

rvagg commented Apr 8, 2021

@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): multiformats/js-multiformats#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 multiformats/js-multiformats#75 branch's dist linked as multiformats but not with the current 6.0.0 version - they fail in the 4th case.)

@rvagg
Copy link
Member Author

rvagg commented Apr 8, 2021

The tests will pass with the multiformats/js-multiformats#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
Copy link
Contributor

Gozala commented Apr 8, 2021

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

rvagg commented Apr 22, 2021

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

matches ipld/js-dag-pb#6

@rvagg rvagg force-pushed the rvagg/esm-ts-build-improvements branch from 94aafed to e10cd26 Compare April 26, 2021 04:16
@rvagg rvagg changed the base branch from export-object to master April 26, 2021 04:24
BREAKING CHANGE:

* remove `default` export, see below
* check, generate and publish full TypeScript types
* use multiformats@7 and its non-default exports and updated types
* export types based on latest multiformats structure
* publish with "main"

Prior to this change this module was imported as

```js
import dagcbor from '@ipld/dag-cbor'
```

Now it is imported as

```js
import * as dagcbor from `@ipld/dag-cbor'
```
@rvagg rvagg force-pushed the rvagg/esm-ts-build-improvements branch from e10cd26 to b5fd787 Compare April 26, 2021 04:25
@rvagg rvagg merged commit 6513572 into master Apr 26, 2021
@rvagg rvagg deleted the rvagg/esm-ts-build-improvements branch April 26, 2021 04:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants