ipld / js-dag-cbor

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

Invalid types are generated and published #24

Closed Gozala closed 3 years ago

Gozala commented 3 years ago

We had to disable lib check here https://github.com/ipfs-shipyard/nft.storage/pull/56#discussion_r628082097 because js-dag-cbor seems to have invalid types generated. Specifically:

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

T generic type parameter is not declared for whatever reason. It maybe related to https://github.com/microsoft/TypeScript/issues/41258

Gozala commented 3 years ago

I'm suspecting this maybe causing the problem here

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

Since generics for statics is not a thing

rvagg commented 3 years ago

https://github.com/multiformats/js-multiformats/blob/96ff0043da2cfa158db966e4d22073141e9f9b3a/src/codecs/interface.ts#L26-L28

perhaps you could see a creative way to deal with this weird construct instead? I'd really like to not have this because it pushes the generic into the bytes portion of the encoder/decoder interface which is not needed cause it's always just Uint8Array

rvagg commented 3 years ago

Here's the other thing that bothers me about this - I have a TypeScript project in test/ts-use/ that tries to exercise the typing. What is it doing wrong that it's not picking up what you're experiencing?

rvagg commented 3 years ago

How about we just make ByteView<T> go away and stick with Uint8Array instead?

export interface BlockEncoder<Code extends number> {
  name: string
  code: Code
  encode(data: any): Uint8Array
}
export interface BlockDecoder<Code extends number> {
  code: Code
  decode(bytes: Uint8Array): any
}

The one thing T has been nice for is dag-pb where we want strong typing on the object: https://github.com/ipld/js-dag-pb/blob/1e5ab163a94416631aeec92a3e92d347f3017375/src/index.js#L276-L284

On dag-cbor and dag-json we can handle a subset of any (I've been considering an IPLD Data Model recursive type that would properly cover it, but I think that's probably going to be way too user-hostile).

Gozala commented 3 years ago

https://github.com/multiformats/js-multiformats/blob/96ff0043da2cfa158db966e4d22073141e9f9b3a/src/codecs/interface.ts#L26-L28

perhaps you could see a creative way to deal with this weird construct instead? I'd really like to not have this because it pushes the generic into the bytes portion of the encoder/decoder interface which is not needed cause it's always just Uint8Array

That was the whole purpose of that beast 🥺 Now it carries information of what is encoded. In practice it should not matter though because it would accept Uint8Array would qulaify asByteView<T>.

Gozala commented 3 years ago

Here's the other thing that bothers me about this - I have a TypeScript project in test/ts-use/ that tries to exercise the typing. What is it doing wrong that it's not picking up what you're experiencing?

I would guess that it's is the problem with how ipjs does things, but I'd have to check. I have this setup in bunch of places where it works.

Gozala commented 3 years ago

How about we just make ByteView<T> go away and stick with Uint8Array instead?

export interface BlockEncoder<Code extends number> {
  name: string
  code: Code
  encode(data: any): Uint8Array
}
export interface BlockDecoder<Code extends number> {
  code: Code
  decode(bytes: Uint8Array): any
}

I am not sure what problem are you trying to solve here, so it's hard for me to provide meaningful input. Generally though here is why I made those generic over T.

The one thing T has been nice for is dag-pb where we want strong typing on the object: https://github.com/ipld/js-dag-pb/blob/1e5ab163a94416631aeec92a3e92d347f3017375/src/index.js#L276-L284

Exactly! By having T there you could choose to make a codec like BlockCodec<112, any> or you could choose to provide more compile time value, illustrated in photos by doing something like BlockCodec<112, DAGNode> or what you do there for DAGPB

On dag-cbor and dag-json we can handle a subset of any (I've been considering an IPLD Data Model recursive type that would properly cover it, but I think that's probably going to be way too user-hostile).

I think it depends on the audience, I would suspect typed camp will be using this feature and they will appreciate all the compile time info they can get. People who do not care about TS are likely not going to care about any of this anyway.

Gozala commented 3 years ago

P.S. I wanted to thread through Code into the ByteView as well so it would be even more useful and catch things like these

image

In the end I did not do it because I did not wanted to make scare folks away

Gozala commented 3 years ago

This may also explain a bit more why I was so keen on the whole cbor.or(pb).or(jose) pipelines as the composition would carry typing information allowing it to encode / decode either of those but reject anything else at compile time.

Gozala commented 3 years ago

How about we just make ByteView<T> go away and stick with Uint8Array instead?

If the motivation here is to not have to type the T this might be a better alternative, that would reduce that burden while retaining all the benefits

interface CID {
  asCID: CID
}

type DAGValue =
  | boolean
  | number
  | string
  | null
  | CID

type DAGArray = DAGNode[]
type DAGObject = {[key:string]: DAGValue|DAGArray|DAGObject}
type DAGNode = DAGValue | DAGObject | DAGArray

interface ByteView<Code extends number, T> extends Uint8Array {
  code?: Code
  data?:T
}

interface BlockEncoder<Code extends number, Node extends unknown = unknown> {
  encode<T extends Node>(node:T):ByteView<Code, T>
}

interface BlockDecoder<Code extends number, Node extends unknown = unknown> {
  decode<T extends Node>(bytes:ByteView<Code, T>):T
}

declare var enigma:BlockEncoder<100> & BlockDecoder<100>
declare var cbor:BlockEncoder<112, DAGNode> & BlockDecoder<112, DAGNode>
declare var anything: BlockEncoder<0, any> & BlockDecoder<0, any>

const e1 = enigma.encode({ hello: "world" })
const out = enigma.decode(e1.slice(1))
out.hello /*
^^^ Object is of type 'unknown'.(2571) */

const out2 = enigma.decode(e1)
out2.hello // block view has type information so it knows hello is there

const c1 = cbor.encode({ my: 'suff' })
cbor.decode(c1).my // knows it has my becouse c1 carries info
const c2 = cbor.decode(new Uint8Array())
if (c2) {
  if (typeof c2 === 'object') {
    c2 // now it knowns it's CID | DAGObject | DAGArray
  } else {
    c2 // here it knowns it's boolean|string|number
  }
}

const c3 = cbor.encode({ items: new Set([1, 2]) })
//                     ^^^^^^^^^^^^^^^^^^^^^^^^^^
// Types of property 'items' are incompatible.
//   Type 'Set<number>' is not assignable to type 'DAGValue | DAGObject | DAGArray | undefined'.

const a = anything.encode({ method() { throw 'Boom!' }})
const a2 = anything.decode(new Uint8Array())
a2.just.like.that.go.nuts()