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

Invalid types are generated and published #24

Closed
Gozala opened this issue May 7, 2021 · 10 comments · Fixed by #25
Closed

Invalid types are generated and published #24

Gozala opened this issue May 7, 2021 · 10 comments · Fixed by #25

Comments

@Gozala
Copy link
Contributor

Gozala commented May 7, 2021

We had to disable lib check here nftstorage/nft.storage#56 (comment) 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 microsoft/TypeScript#41258

@Gozala
Copy link
Contributor Author

Gozala commented May 7, 2021

I'm suspecting this maybe causing the problem here

* @template T

Since generics for statics is not a thing

@rvagg
Copy link
Member

rvagg commented May 8, 2021

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

rvagg commented May 8, 2021

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

rvagg commented May 10, 2021

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

Gozala commented May 11, 2021

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

Gozala commented May 11, 2021

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

Gozala commented May 11, 2021

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.

  • Some codecs may not accept any and this provides a way to entype it and surface what you'd surface incompatibility at compile time instead of runtime.
  • Truth is data is not really any but rather a JSON + CIDs. That can actually be encoded in types which looks something along these lines
    image
    Note how hints show exactly what Uint8Array encodes. And then decoding that block also gives you exact structure of what comes out
    image
    And if you just use Uint8Array type checker is till helpful here in telling you what can go wrong
    image

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

Gozala commented May 11, 2021

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

Gozala commented May 11, 2021

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

Gozala commented May 11, 2021

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()

rvagg pushed a commit that referenced this issue May 12, 2021
@rvagg rvagg closed this as completed in #25 May 12, 2021
rvagg pushed a commit that referenced this issue May 12, 2021
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 a pull request may close this issue.

2 participants