Closed Gozala closed 4 years ago
/cc @vmx
Can you please explain in which cases matchCID() is better? I'm a bit hesitant to add new methods to the public API that are similar to the existing API.
So when type-checker sees code like below where x
has non specific type say CID|Buffer
It can’t refine that type within if block
if (CID.isCID(x)) {
x //: Buffer|CID
}
If instead it was something like:
if (x instanceof CID) {
x//:CID
}
That is because calls to some functions can’t refine type of the bindings in the caller. However functions can refine type and return refined type, which is what this CID.matchCID does
var cid = CID.matchCID(x)
// now flow knows it’s type is `?CID`
if (cid) {
cid //:CID because it’s not void
}
I would like to address @hugomrdias comment here https://github.com/multiformats/js-cid/issues/83#issuecomment-481211795 because I don't want to further derail API change proposal into typing debate
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.
I agree that flow team has being really bad & at this point in time I would probably prefer TS myself even though type system is significantly less expressive. However adopting TS is far more costly because you have to buy-into whole tooling / syntax. With flow type comments bar is dramatically lower as you just add comments. As of JSDOC it's not even remotely the same, it better than nothing but I'd definitely won't call it a type checker. What we do in this pull request is actually both.
However adopting TS is far more costly because you have to buy-into whole tooling / syntax.
Not anymore. @hugomrdias talks about TypeScript via JSDoc Syntax. It's like flows inline comments (just not inline).
One can do the same type checking as normal ts, generated documentation and more.
@Gozala: FYI, the CID.isCID(x)
comes from the 'is-class' module, so it's only true if x
is really a CID
instance. But I guess that can't be expressed in flow.
However adopting TS is far more costly because you have to buy-into whole tooling / syntax.
Not anymore. @hugomrdias talks about TypeScript via JSDoc Syntax. It's like flows inline comments (just not inline).
One can do the same type checking as normal ts, generated documentation and more.
I'm aware, but that JSDoc syntax is even more limited e.g. you can't say define / expose interface. Nor I see how you would define generic methods.
I might be biased towards flow as I find JSDoc syntaxt to be difficult to comprehend. I will understand if choice is made in favor of TS.
@Gozala: FYI, the
CID.isCID(x)
comes from the 'is-class' module, so it's only true ifx
is really aCID
instance. But I guess that can't be expressed in flow.
I understand that even if it was plain static method as:
class CID {
isCID(x) {
return x instanceof CID
}
}
It still would refine x
with-in the isCID
scope but not in the caller scope.
I might be biased towards flow as I find JSDoc syntaxt to be difficult to comprehend.
I also don't like the JSDoc syntax :)
You can define interfaces and import them where ever you want and you can use ts syntax instead of jsdoc in the comments. Generics I never tried ..
I'm not trying to campaign for TS, just sharing what I learned about it. But it scares me a bit that everyone is moving to TS even internal FB teams. Doesn't seem wise for me to start using flow now.
@vmx I have addressed all the comments. Please let me know what would you like to do with this.
@Gozala I still want to give flow a try. Though I'd like to have the matchCID()
removed. I don't like to introduce new public APIs that are mainly there to make type-checkers happy. For new APIs we should make sure that the APIs work well with types.
@Gozala I still want to give flow a try. Though I'd like to have the matchCID() removed. I don't like to introduce new public APIs that are mainly there to make type-checkers happy. For new APIs we should make sure that the APIs work well with types.
Alright, I’ll make a change to remove it then.
I just wanted to avoid introducing a friction that could lead to a bad experience with a type checker. I do understand your argument.
IPFS is now using TypeScript in JSDocs, for more information see https://github.com/ipfs/js-ipfs/issues/2945. Hence there's no point of introducing Flow here anymore.
This change adds flow comment syntax & runs flow check on lint that will fail on type missmatches.
Additionally more type-friendly
CID.matchCID
alternative toCID.isCID
is added which returns CID if value is CID or void otherwise.Method
.equals
is updated so parametre can by an arbirtary value, not just CID. It makes working with CIDs & type-checker a lot easier as it avoids refiningv
toCID
type before being able to callcid.equals(v)
.--
I'm using
flow check
right now but that should probably be moved to aegir https://github.com/ipfs/aegir/issues/347. I suggest to address that separately.