Provide non-generic constructor methods #83
Comments
+1 on a
We don’t have a canonical JSON representation of a CID anymore, so I’m not quite sure what this means. CID’s are serialized differently in different codecs.
These seem to be more motivated by type system tooling preference than JS ergonomics. I’ve used this library quite a bit and being able to think about a serialized CID as either a string or buffer and not really caring which has been quite useful. —- Somewhat related, I’d like a method that I can pass a string/buffer and it will tell me if it’s a valid CID or not. |
Type checker can actually figure this out much better than when I do read the code, as I mentioned when I'm reading a code and see e.g. That is why I'm biased toward monophonic functions, in fact if in my editor with type-checker I usually can hover the variable to know the type. However I really wish just reading source on github could provide enough context. |
Hrm... I wonder what this is there for, cause this isn’t even the old canonical serialization :( |
|
Just noticed there's open issue on |
I'm also unsure. I still think types have huge benefits, but I also don't want to scare the JS crowd away. Though the usage of Anyway. The current constructor code is indeed quite strange. I currently lean towards having those typed methods, but also the As |
|
i dont have strong feeling regarding typed methods or a buffer like CID.from but i think we should only have one or the other not both. Regarding type checkers im in favour of typescript with jsdoc (maybe some type definitions), full ts or flow (flow doesn't feel right after seing FB react and jest teams drop flow for ts) are too much of an overhead. |
|
The js-cid constructor is gross, but it is useful when used against an existing codebase that deals with the string form, buffer form, and CID object form interchangeably. I'm with @vmx here that we should convert input to a CID instance at the boundaries, and then deal only with CID instances after that. That being the case, a CID becomes how we normalise the users input... so then it'd be rad if we can make that nice for both vanilla JS and the type fans. |
|
I'm very much a fan of the Robustness Principal here - we should continue to provide an API that accepts strings, buffers, etc (either the constructor or the proposed Re: adding loads of typed methods: @mikeal is right - expanding the API surface area just to make type-related tooling happier seems like the tail wagging the dog. |
I'm not a fan of JSDocs (I thought I would, but I'm not). flow is a more powerful type-checker than TypeSCript is (as this is what flow was specicially built for). Hence I prefer flow. I'm after proper types, not IDE/auto-completion support. For supporting IDEs TypeScript JSDocs would make more sense. |
|
Although I know little about the internals of IPFS/IPLD/CID/Multiformats, I'll just second @achingbrain on making things as simple as possible for the user. (Also, keeping conventions consistent between these various projects would be helpful, if that's a consideration.) The ProtoSchool lessons got much easier when we stopped having to use |
|
I think there are lot of assumptions here made that this proposal is to make types easier or that it would make people’s life harder - Truth is, it’s quite the opposite when I have type-checker at hand it can infer types and tell me so I don’t have to guess what is This is not a made-up story, that is my personal experience and a reason I end up doing first iteration on typedefs so I did not had to do these side-trips to make sense of the lot of new code. So while I agree for the writer it’s easier to write I also agree with @vmx ideally CID instances should be passed around & user should rarely if ever have to construct them this way, more like |
|
Not sure I agree - if |
|
@achingbrain you may disagree, but as I said it has being my struggle with IPFS - my brain might be wired differently from yours, but I often need to understand what are the things being passed around to make changes to them. |
|
We should re-focus what @Gozala is specifically proposing. I've nothing against the propsal to add extra static factory methods for each of the ways you can construct a CID. I was going to decompose the constructor that way in #77, but decided against it to keep the size ot the PR down. The idea of removing the generic constructor that takes anything is proposed as a "ideally" which i read as "nice to have", and that it "may be too much of a breaking change.". |
|
Can we just call it |
I just wanted to make method names correspond to their opposites in this case |
|
Yes please! |
|
We actually alias I didn’t know this until I looked through the code, we should probably document this better and make it the recommended API. |
|
I would like to reboot this conversation! In the light of Proposed CID interfacedeclare class CID {
// Just like all other ArrayBuffer views it can take underlying buffer and optional range within it
// to construct a CID view.
constructor(buffer:ArrayBuffer, byteOffset:number=0, byteLength:number=buffer.byteLength)
// Just like other typed arrays you can create CID view from bytes in the typed array.
constructor(ArrayBufferView)
// Just like with other typed arrays (and some other JS built-ins) you can pass CID in any other representation
// to construct an instance.
static from(string|ArrayBuffer|ArrayBufferView|CID):CID
// You can also create CID out of underlying data. Think of it as `Object.create` providing lower level API instead of overloading `Object` constructor.
static create(version:number, code:number, hash:Uint8Array):CID
}Proposed API tries to disambiguate between:
It may make more sense to have |
|
I don't buy the arguments you have for retaining constructors, making this thing feel like an ArrayBuffer would probably mean we've failed to make it a nice abstraction. Just because it might be built as an abstraction over ArrayBuffer doesn't mean we need to expose that to users, there's not a good reason to leak that upward. So if you put that aside, you run out of reasons to have constructors and static factory functions. So I'd just ditch constructors entirely, or make a single-use constructor of some kind that's justifiable. I'd be more than happy with static factory methods that were explicit about what they do. I'm a big +1 wherever we can reduce argument overloading. It's one of the biggest curses that jQuery gave JavaScript culture that we still haven't shaken and it keeps on leading to APIs that cause far more harm than the convenience they supposedly offer. Would this work? declare class CID {
static from(string|ArrayBuffer|ArrayBufferView|CID):CID
static from(buffer:ArrayBuffer, byteOffset:number=0, byteLength:number=buffer.byteLength):CID
static create(version:number, code:number, hash:Uint8Array):CID
}It's a big jump from js-cid, a simple one for js-multiformats in its early stage though.
|
That is not the argument I was making. I was advocating to have a single purpose for a constructor. And I imagine that to be mostly used internally or something very low level. I imagine mostly uses through factory functions.
I think where we might be disagreeing is that I would like to portrait CID as a view over some byte range (it just happens to be the case with all the typed arrays). I find to be a better model than presenting CID as something you decode from bytes. With such a viewpoint it's not a leaky abstraction, but a deliberate emphasis. If you have an argument against that view point, I'd like to consider that.
I am not sure what exactly you are suggesting. I don't think you can ditch constructors entirely. You could probably throw and use
I could not agree more!
My primary concern with
I thing that would be a bad idea because:
More broadly, having similar API signatures (blame TS for overlooking exceptions) alone is not a good reason to combine things that play different roles. |
|
This has me thinking a bit. I’d like to unwind this a little and just consider what I’d want the ideal interface to be. This may not work here but may be what we want to move to in When you have multiple ways of instantiating the CID I think it is best to just not use the constructor and instead use class methods designed for each case. We do this already in In that case, is something more like this the best way to design CID? const encode = (version, code, multihash) => concat(varint.encode(version), varint.encode(code), multihash)
class CID {
constructor({code, version, bytes, multihash}) {
if (typeof code !== ‘undefined’) this._code = code
// and so on for version, bytes, multihash
}
get bytes {
if (!this._bytes) {
this._bytes = encode(this.version, this.code, this.multihash))
}
else return this._bytes
}
get version {
if (!this._version) {
if (this._bytes) this._version = varint.decode(this._bytes)
else this._version = 1
}
return this._version
}
}Basically, you want the constructor to just take whatever the If you look at how these get used, there’s a lot of cases where you either never ask for the full byte representation or only ask for the fully byte representation, so we could avoid a lot of extra work by just deferring that processing until it is requested. |
That is exactly how I think about constructors so 👍 That being said, there is a caveat.
I think above trade-offs aren't worth the benefit you'll get from some lazy computation.
For all the above reasons I settled on something that I think is in the same spirit as you've described in
Both factories are a also smart in that they will cache And if you do have CID byte range already you don't need to decode or encode, you can create a view via constructor. I think this is a reasonable compromise that provides deterministic performance profile for each construction operation. If my assumption is incorrect and lazy encode / decode (from / create) does provide significant value that outweighs tradeoffs, I would argue that they should not be represented via the same CID class, but via different class that |
|
We have discussed this on the call today and tentatively reached an agreement on the following plan:
|
I would very much like to make construction of
CID's less generic as it really reduces cognitive overhead (saves you from side-trips) when reading code that doesnew CID(x)because from the contextxcan be many different things.Which that goal in mind I'd like to propose adding distinct distinct static methods:
CID.fromCID(cid)- Does that even makes sense ? Should there maybe aclone()method instead ?CID.fromBaseEncodedString(BaseEncodedString)or maybeCID.parse(BaseEncodedString)CID.fromBuffer(Buffer)or better yetCID.decode(Buffer)CID.fromJSON(SerializedCID)Ideally
CID()then could be made non genericCID(Version, Codec, Multihash, name:MultibaseName='base58btc')I would even go and make last param mandatory. If really desired generic version could also be added:CID.from(CID|BaseEncodedString|Buffer|SerializedCID):CID<a>(although I'd prefer not to)I do understand that maybe changing constructor API might be too big of a breaking change so I propose to gradually fade it out. Maybe initially
CID.create(Version, Codec, Multihash, MultibaseName)could be added instead and slowly migrate users to new ways until it's safe to make a switch.The text was updated successfully, but these errors were encountered: