feat: integrate flow type-checker #82

Gozala · 2019-04-08T19:36:52Z

This change adds flow comment syntax & runs flow check on lint that will fail on type missmatches.

Additionally more type-friendly CID.matchCID alternative to CID.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 refining v to CID type before
being able to call cid.equals(v).

--

I'm using flow check right now but that should probably be moved to aegir ipfs/aegir#347. I suggest to address that separately.

Gozala · 2019-04-08T19:37:06Z

/cc @vmx

src/index.js

This change adds flow comment syntax & runs flow check on lint that will fail on type missmatches. Additionally more type-friendly `CID.matchCID` alternative to `CID.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 refining `v` to `CID` type before being able to call `cid.equals(v)`.

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.

vmx · 2019-04-09T10:34:44Z

package.json

@@ -5,7 +5,7 @@
  "leadMaintainer": "Volker Mische <[email protected]>",
  "main": "src/index.js",
  "scripts": {
-    "lint": "aegir lint",
+    "lint": "aegir lint && flow check --color=always",


I'd like to start slow. So type checking would be its own command type-check or so. This way it also won't run on CI yet. I'd like to address @mikeal concerns about new contributors. I'd like to add the type check as separate check so that it's also clear to contributors that their code basically works and that it's just the types.

Can we add this to at least "release" script ?

We could even add it to CI, I just want it to be a distinct test step.

@vmx so I've added "check": "flow check --color=always". I would like it to run before pull is being submitted, however I'm not exactly sure what the convention here is (I tend to have npm test setup to do all the checks) so please advice.

Never mind, I've figured it out 1bc1670

vmx · 2019-04-09T10:37:42Z

.prettierignore

@@ -0,0 +1,2 @@
+**/*.js


Can you please remove this file, I don't want to clutter the root directory too much.

Yeah sorry that was a mistake, just had to stop vscode from reformatting everything & forgot to omit it from commit

src/index.js

Gozala · 2019-04-09T16:42:41Z

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
}

Gozala · 2019-04-09T17:26:57Z

I would like to address @hugomrdias comment here #83 (comment) 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.

vmx · 2019-04-09T21:12:56Z

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).

hugomrdias · 2019-04-09T21:31:15Z

One can do the same type checking as normal ts, generated documentation and more.

vmx · 2019-04-09T21:40:23Z

@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.

Gozala · 2019-04-09T22:03:43Z

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 · 2019-04-09T22:07:22Z

@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.

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.

vmx · 2019-04-09T22:10:05Z

I might be biased towards flow as I find JSDoc syntaxt to be difficult to comprehend.

I also don't like the JSDoc syntax :)

hugomrdias · 2019-04-10T06:44:25Z

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 ..

hugomrdias · 2019-04-10T06:50:21Z

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.

Gozala · 2019-04-16T21:21:48Z

@vmx I have addressed all the comments. Please let me know what would you like to do with this.

vmx · 2019-04-23T10:36:05Z

@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 · 2019-04-23T16:07:04Z

@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.

vmx · 2020-06-18T16:39:00Z

IPFS is now using TypeScript in JSDocs, for more information see ipfs/js-ipfs#2945. Hence there's no point of introducing Flow here anymore.

Gozala commented Apr 8, 2019

View reviewed changes

src/index.js Show resolved Hide resolved

Gozala commented Apr 8, 2019

View reviewed changes

src/index.js Show resolved Hide resolved

Gozala force-pushed the type-checker branch from 7e9692c to 59fda51 Compare April 8, 2019 21:57

vmx reviewed Apr 9, 2019

View reviewed changes

Gozala added 3 commits April 16, 2019 14:09

chore: remove .prettierignore to address review comment

d5a7990

fix: move type checknig into own script (as per review)

19cd945

docs: explain CID type parameter

41f3f11

Gozala force-pushed the type-checker branch from f16acec to 41f3f11 Compare April 16, 2019 21:18

Gozala mentioned this pull request Apr 16, 2019

Clarify intent of type parameter in type defs #84

Open

fix: run type check on push & pull

1bc1670

Gozala force-pushed the type-checker branch from e52587f to 1bc1670 Compare April 17, 2019 18:37

vmx closed this Jun 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: integrate flow type-checker #82

feat: integrate flow type-checker #82

Gozala commented Apr 8, 2019

Gozala commented Apr 8, 2019

vmx left a comment

vmx Apr 9, 2019

Gozala Apr 16, 2019

vmx Apr 17, 2019

Gozala Apr 17, 2019

Gozala Apr 17, 2019

vmx Apr 9, 2019

Gozala Apr 9, 2019

Gozala commented Apr 9, 2019

Gozala commented Apr 9, 2019

vmx commented Apr 9, 2019

hugomrdias commented Apr 9, 2019

vmx commented Apr 9, 2019

Gozala commented Apr 9, 2019

Gozala commented Apr 9, 2019

vmx commented Apr 9, 2019

hugomrdias commented Apr 10, 2019

hugomrdias commented Apr 10, 2019

Gozala commented Apr 16, 2019

vmx commented Apr 23, 2019

Gozala commented Apr 23, 2019

vmx commented Jun 18, 2020

feat: integrate flow type-checker #82

feat: integrate flow type-checker #82

Conversation

Gozala commented Apr 8, 2019

Gozala commented Apr 8, 2019

vmx left a comment

Choose a reason for hiding this comment

vmx Apr 9, 2019

Choose a reason for hiding this comment

Gozala Apr 16, 2019

Choose a reason for hiding this comment

vmx Apr 17, 2019

Choose a reason for hiding this comment

Gozala Apr 17, 2019

Choose a reason for hiding this comment

Gozala Apr 17, 2019

Choose a reason for hiding this comment

vmx Apr 9, 2019

Choose a reason for hiding this comment

Gozala Apr 9, 2019

Choose a reason for hiding this comment

Gozala commented Apr 9, 2019

Gozala commented Apr 9, 2019

vmx commented Apr 9, 2019

hugomrdias commented Apr 9, 2019

vmx commented Apr 9, 2019

Gozala commented Apr 9, 2019

Gozala commented Apr 9, 2019

vmx commented Apr 9, 2019

hugomrdias commented Apr 10, 2019

hugomrdias commented Apr 10, 2019

Gozala commented Apr 16, 2019

vmx commented Apr 23, 2019

Gozala commented Apr 23, 2019

vmx commented Jun 18, 2020