feat!: fix types for exported entrypoints and simplify others #50
Conversation
- JWT<C> its just a string and phantom to retain C - SignPayload removed, not really useful - Block removed not used anywhere - New `UCANBlock` type, if make the Capability generic an array we can merge this type with ucanto transport block - DID is now DIDString - Authority is now DIDVerifier - Some more tweaks and fixes for new TS version closes #49
hugomrdias
changed the title
There was a problem hiding this comment.
Thanks @hugomrdias I'm mostly onboard with all the changes here, even if I don't like some of the new names.
I do however disagree with changes in regards to CID / Link stuff. It took me a lot of time and effort to introduce changes that would move from CID class to Link interface in multiformats and changes here seem to go into opposite direction. Can we please not do that, I know it causes some pain right now but as soon as new multiformats is out it will be all nice & pretty. We would also be able to remove some of the local types here in favor of ones in multiformats.
package.json
Outdated
| ".": "./src/lib.js", | ||
| "./did": "./src/did.js", | ||
| "./codec/cbor": "./src/codec/cbor.js", | ||
| "./codec/raw": "./src/codec/raw.js" |
There was a problem hiding this comment.
TS is moving to export maps and I think it would be a good idea to follow
There was a problem hiding this comment.
give me links please, i would love to but what was there before didnt work.
There was a problem hiding this comment.
There was a problem hiding this comment.
tried to do this but we cant change module in tsconfig yet it breaks other dep imports ...
|
in the last commit i changed DID back to being a string a renamed DID to Agent. Identity is still there because removing it breaks ucanto. We can remove it later after upgraded ucanto |
There was a problem hiding this comment.
Pointed out few things, but I leave it up to you to decide what to do about them. Feel free to land as is or address some of those things and then land, I don't think we need another round of review here.
If you want to rename Agent to something else before landing that's cool with me as well.
src/ucan.ts
Outdated
| /** | ||
| * Entity that can sign UCANs with keys from a {@link Agent} using the signing algorithm A | ||
| */ | ||
| export interface Signer<A extends number = number> |
There was a problem hiding this comment.
Nit: I kind of liked Issuer name better a role was a lot more clear. Also idea was that we could add more things to the interface in the future if we need to.
src/ucan.ts
Outdated
| export type Link< | ||
| Cap extends Capability = Capability, | ||
| Alg extends number = number | ||
| > = IPLDLink<Model<Cap>, typeof CBOR_CODE, Alg, 1> | IPLDLink<JWT<Cap>,typeof RAW_CODE, Alg, 1> |
There was a problem hiding this comment.
Nit: You could leave out version as 1 is a default.
| > = IPLDLink<Model<Cap>, typeof CBOR_CODE, Alg, 1> | IPLDLink<JWT<Cap>,typeof RAW_CODE, Alg, 1> | |
| > = IPLDLink<Model<Cap>, typeof CBOR_CODE, Alg> | IPLDLink<JWT<Cap>,typeof RAW_CODE, Alg> |
|
|
||
| // readonly asCID: this |
There was a problem hiding this comment.
This was used in bunch of places & also a reason why I could not extend CID class because it's marked private there and that caused TS to complain.
I don't mind adding //@ts-expect-error in those places if this is really important, but just want to make sure you're aware of this.
UCANBlocktype, if we make theCapabilitygeneric an array we can merge this type with ucanto transport blockcloses #49