ipld / js-dag-cbor

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

fix: type annotations #25

Closed Gozala closed 3 years ago

Gozala commented 3 years ago

Fixes #24

As per #24 issue generic declared on value caused invalid types to be generate

https://github.com/ipld/js-dag-cbor/blob/a49da7e8d4008c995c829c6cf04f2746ee39774b/index.js#L101-L116

Which looked like this:

export const encode: (data: T) => import("multiformats/codecs/interface").ByteView<T>;

TS does non really has equivalent of generic value in TS syntax and my guess is it just gets tripped over the by the thing in JSDoc.

I should also note that const { code, name ... } = ... also is not going to work, because of how TS infers types, if you create const { code: 2 } it is inferred as { code: number } but if you do const code = 2 code will be inferred as 2 as TS knows it can never change (unlike object). More details here https://www.typescriptlang.org/docs/handbook/release-notes/typescript-4-2.html#reverting-template-literal-inference

For those reason this change just dose export const to allow TS infer all as needed & I've verified that it addresses the problem.

rvagg commented 3 years ago

well that sucks, I really wanted to get the BlockCodec type strongly tied to the export which is why it's there. Now you've removed that use you'll also need to remove the import at the top of the file. But I'd love to have a way to get it back to assert that this is the thing we're exporting, that's the whole reason behind this pattern, which is repeated in dag-json and dag-pb too, and I think I may have even done the same thing in raw and json up in multiformats.

Gozala commented 3 years ago

to assert that this is the thing we're exporting, that's the whole reason behind this pattern,

We could do and probably should do it in tests. Ideally we would exercise this module with something that requires BlockCodec, BlockEncoder, BlockDecoder as its' argument. If implementation does not meet that expectation that would produce an error.

If you really want it here we could also have line like:

/** @type {BlockCoder<*, *>} */
const codec = { code, name, encode, decode }

Which will fail TS check if there is an incompatibility.

rvagg commented 3 years ago

I figured out why ts-use wasn't picking this up, there's a "skipLibCheck": true in its tsconfig.json.. Change that and it fails.

I've pushed two more commits to this branch.

rvagg commented 3 years ago

ok, so I just merged this but am going back over the discussion in #24 as I apply it to the other codecs but we've omitted ByteView<T> from here and just gone with Uint8Array so your ByteView is entirely omitted here .. should we have done it the other way because they don't quite match the BlockCodec pattern now.

rvagg commented 3 years ago
diff --git a/index.js b/index.js
index 44a2bfd..99be0a5 100644
--- a/index.js
+++ b/index.js
@@ -4,6 +4,11 @@ import { CID } from 'multiformats/cid'
 // https://github.com/ipfs/go-ipfs/issues/3570#issuecomment-273931692
 const CID_CBOR_TAG = 42

+/**
+ * @template T
+ * @typedef {import('multiformats/codecs/interface').ByteView<T>} ByteView
+*/
+
 /**
  * cidEncoder will receive all Objects during encode, it needs to filter out
  * anything that's not a CID and return `null` for that so it's encoded as
@@ -98,13 +103,13 @@ export const code = 0x71
 /**
  * @template T
  * @param {T} node
- * @returns {Uint8Array}
+ * @returns {ByteView<T>}
  */
 export const encode = (node) => cborg.encode(node, encodeOptions)

 /**
  * @template T
- * @param {Uint8Array} data
+ * @param {ByteView<T>} data
  * @returns {T}
  */
 export const decode = (data) => cborg.decode(data, decodeOptions)

yields:

export const name: "dag-cbor";
export const code: 113;
export function encode<T>(node: T): ByteView<T>;
export function decode<T>(data: ByteView<T>): T;
export type ByteView<T> = import('multiformats/codecs/interface').ByteView<T>;

and passes the ts-use test.