feat: add flow typedefs #79
Conversation
Add flow type definitions which would improve workflow for dependent libraries that happen to use flow type-checker.
|
I don’t know enough about flow to know if this is correct or not so I’ll just trust that you did it correctly and merge it :) |
|
whoops! my bad, I thought this was a different repo cause we were talking about this same thing there. I should not have merged this before others had reviewed it. can we get some +1’s here and if people have objections i can back it out. |
| export type BaseEncodedString = string | ||
|
|
||
| declare class CID<a> { | ||
| constructor(Version, Codec, Multihash): void; |
There was a problem hiding this comment.
CID now takes an optional multibaseName string as the 4th param as of #77
There was a problem hiding this comment.
Thanks for pointing that out @olizilla, I'll create a followup pull to add that param.
There was a problem hiding this comment.
Not sure if there's consensus, but I'm the lead maintainer and I'd like to keep them :)
| export type Multihash = Buffer | ||
| export type BaseEncodedString = string | ||
|
|
||
| declare class CID<a> { |
There was a problem hiding this comment.
@Gozala generics aren't a requirement for class defs are they? I don't see a being used in CID and am not sure how CID could be a generic container so does <a> mean something else here?
There was a problem hiding this comment.
@Gozala generics aren't a requirement for class defs are they?
Correct, they are not required
I don't see a being used in CID and am not sure how CID could be a generic container so does mean something else here?
On my phone so can’t provide links but this is related to our other discussions on decode / encode. CID<a> implies here that it’s address for node a which in turn would allow get(CID<a>):Promise<a> without type parameter a it wouldn’t be possible to express that type of relationship.
There was a problem hiding this comment.
To be more specific: indeed a type param is not used here, but it’s there because users of this lib are expected to make use of that parameter
There was a problem hiding this comment.
Oh, wow, that's pretty funky. So tools that use flow definitions would follow that all the way through such a call chain and enforce that, or is this simply for the sake of descriptive completeness?
There was a problem hiding this comment.
Flow type-checker will indeed catch and report any miss-matches.
There was a problem hiding this comment.
BTW that’s also a weak point of TS (at least it was) as it throws away unused type parameters not used in the class / function definitions while flow keeps them for inference at call-sites. It may not seem that important but in practice it makes a huge difference
|
This was planned over a year ago ipfs/js-ipfs#1260 but seems to have stalled until now. @vmx are you planning on working on it this quarter? |
|
No real plan to work on it, but somehow I got dragged into this and as I'm still in favour of it (though this time less intrusive with using the inline comment style) I think I'll spend some time on it. |
@vmx I’m sorry |
Add flow type definitions which would improve workflow for dependent libraries that happen to use flow type-checker.
This just
index.js.flowis what flow would resolve to when imported module resolves toindex.jsand there for dependencies of this library will be able to infer type info.