feat: implement link interface & module #197
Conversation
TS confuses it with Alogrithm from crypto.subtle stuff
Co-authored-by: Rod Vagg <[email protected]>
| return new Block({ cid, bytes, value }) | ||
| return new Block({ | ||
| // eslint-disable-next-line object-shorthand | ||
| cid: /** @type {CID<T, Code, Alg, V>} */ (cid), |
There was a problem hiding this comment.
What would this look like if it was written in TypeScript? Can you do an unsafe cast like this, or would you need to pass it through CID.asCID(), and should we be doing that here anyway?
There was a problem hiding this comment.
What would this look like if it was written in TypeScript?
| cid: /** @type {CID<T, Code, Alg, V>} */ (cid), | |
| cid: <CID<T, Code, Alg, V>>cid |
Not sure if it would require cid: or casting on shorthand would have worked
There was a problem hiding this comment.
Can you do an unsafe cast like this, or would you need to pass it through CID.asCID(), and should we be doing that here anyway?
it is actually safe cast, if it wasn’t CID TS would not have allowed it. Here we just refine not fully known types unknown→ T, number → Code, etc..
I think it’s right thing to do if we want to capture type parameters otherwise it would come out unrefined and reduce TS ability to do inference.
|
|
||
| export type BlockCursorView<T extends unknown = unknown> = | ||
| | { value: T, remaining?: undefined } | ||
| | { value: CID, remaining: string } |
| A extends number = number, | ||
| V extends Version = 1 | ||
| > extends Block<T, C, A, V> { | ||
| cid: CID<T, C, A, V> |
There was a problem hiding this comment.
this extends Block and makes a this CID when it was just a Link, does it need this?
| * @template T - Logical type of the data being linked to. | ||
| * @template C - multicodec code corresponding to a codec linked data is encoded with | ||
| * @template A - multicodec code corresponding to the hashing algorithm of the CID |
There was a problem hiding this comment.
these 3 names don't match the template parameter names used below, doesn't that matter to the compiler? probably should make them match anyway
| equals(other: unknown): other is Link<Data, Format, Alg, Version> | ||
|
|
||
| toString<Prefix extends string>(base?: MultibaseEncoder<Prefix>): ToString<Link<Data, Format, Alg, Version>, Prefix> | ||
| toJSON(): { version: V, code: Format, hash: Uint8Array } |
There was a problem hiding this comment.
do we really need this here? maybe this could just be reserved for the concrete CID to leave the interface a little simpler.
There was a problem hiding this comment.
We could omit. Although I think it’s useful and is used in few places where you could accept Link type
|
|
||
| export type ToString<T, Prefix extends string = string> = Multibase<Prefix> & Phantom<T> | ||
|
|
||
| export type { ByteView } |
There was a problem hiding this comment.
Is this a necessary export? I think this interface is mostly used through the API forms of it in ../interface which already pulls in ../block/interface that has this.
There was a problem hiding this comment.
It’s just nice to have all the link related types in one place so you don’t have to import two modules later. I have bunch of places where I’d import just this module if I could access this type and Link
|
|
||
| toString<Prefix extends string>(base?: MultibaseEncoder<Prefix>): ToString<Link<Data, Format, Alg, Version>, Prefix> | ||
| toJSON(): { version: V, code: Format, hash: Uint8Array } | ||
| link(): Link<Data, Format, Alg, V> |
There was a problem hiding this comment.
can you talk about the choice of name here? would asLink be more consistent with what we've done to CID?
There was a problem hiding this comment.
this seems like it's a convenient way to downcast a concrete CID to a Link, but can't you just use a CID as if it's a Link wherever you need one, do you ever really need to cast it? what's the use-case here?
There was a problem hiding this comment.
Sorry I should have added some code comments here.
it is followup on our #185 discussion which I thought you wanted to include)
I did however since realized that I have some use cases where I’d want a class where link():Promise<Link> and it would be convenient to have equivalent on Link itself. I didn’t like toLink because method with such name returning a promise doesn’t seat well with me.
There was a problem hiding this comment.
oh yeah ... got popped from my memory stack, and seeing all of the additional functionality in context here makes me question this. I like the idea of thing.link(), but with so many other methods on this interface, it seems to diminish the value; we're pretty close to a CID already and I had in my head that a Link interface was going to be a minimal thing that you could just ask for the more full-featured thing from. But that probably doesn't make sense.
| assert.equal(CID.isCID(CID.parse(h1)), true) | ||
| assert.equal(CID.isCID(CID.parse(h1).toV0()), true) | ||
|
|
||
| assert.equal(CID.isCID(CID.parse(h1).toV1()), true) |
There was a problem hiding this comment.
| assert.equal(CID.isCID(CID.parse(h1)), true) | |
| assert.equal(CID.isCID(CID.parse(h1).toV0()), true) | |
| assert.equal(CID.isCID(CID.parse(h1).toV1()), true) | |
| assert.equal(Link.isLink(CID.parse(h1)), true) | |
| assert.equal(Link.isLink(CID.parse(h1).toV0()), true) | |
| assert.equal(Link.isLink(CID.parse(h1).toV1()), true) |
| * @param {unknown|L} value | ||
| * @returns {value is L & CID} | ||
| */ | ||
| export const isLink = value => |
There was a problem hiding this comment.
I thought you didn't like isX() but preferred an asX() pattern?
There was a problem hiding this comment.
I do like parse don’t validate approach. Yet here it seems different because it’s interface, rather than a concrete class, but I suppose asLink would be more consistent.
For what it’s worth, back when I made isCID → asCID case, typescript did not had type predicates or I didn’t know of it’s existence and with boolean return type no refinement took place on x inif (CID.isCID(x)) x. Withif (isLink(x)) x refinement occurs (thanks to return type been x is Link
it is also that isCID actually could return true, even if x was of different type (like CID from previous version) possibly with less methods, but return would have been true. With Link however version doesn’t matter so true seems more accurate.
I’m happy to change to asLink
There was a problem hiding this comment.
if (isLink(x)) xrefinement occurs
makes sense
From a JS consumer perspective though, we've encouraged the asCID pattern already so maybe we should continue with that. Under your influence I find myself being dragged in the parse don't validate direction, not even using TS so this feels like a backward step when not using TS.
| */ | ||
| class Block { | ||
| /** | ||
| * @param {Object} options | ||
| * @param {CID} options.cid | ||
| * @param {ByteView<T>} options.bytes | ||
| * @param {CID<T, C, A, V>} options.cid |
There was a problem hiding this comment.
can we fully Link the Block cass?
There was a problem hiding this comment.
We could but in that case it would be a breaking change & I don’t think it’s worth it. We could make this specific argument type into Link however. But then again, it is more of an internal function, user should use factory instead.
| */ | ||
|
|
||
| /** | ||
| * @template T | ||
| * @param {Object} options | ||
| * @param {CID} options.cid |
There was a problem hiding this comment.
if we're going to bump semver-major anyway (I think we should), then maybe we should also make CID go away in here and make this param a Link and the load param take a Link too?
There was a problem hiding this comment.
For internal stuff I don’t think it really matters & doesn’t seem worth breaking pplz code.
|
👍 looking good; lots of questions and minor suggestions inline. My main feedback focuses around (a) the |
Co-authored-by: Rod Vagg <[email protected]>
Co-authored-by: Rod Vagg <[email protected]>
|
"merged" in to #199 where we're going to get ready for a v10.0.0 which includes an esm-only push. I've applied some additional fixes from here, including some things that are needed for the linting rules from aegir. There's also a commit in here I had to drop, the first one, 553bdee, was overriding #163. It looks like it's an alternative form of the commit that made it in to #160; so nothing's lost by dropping that commit. Unfortunately I have failing browser tests in #199 now for some weird reason. @Gozala if you want to tinker more with this, PR against the |
Replaces #161 effort