Add flow type annotations.

[Gozala]

Setup

• flow is used to typecheck, while babel package is used to read typed code from src into untyped code in lib. I do not know how this setup would fit here, but that's most common one I have seeing so I though it was most reasonable (although it seems to have some problems with current setup, more on that in issues section
• flow-copy-source package is used to move type annotations from src/*.js into lib/*.js.flow, it might be confusing but it actually provides some flexibility when working with many packages, because flow follows the same module resolutions algorithm as node except file.js.flow takes precedence over file.js so this setup enables node to ignore type annotations while flow use type annotations. While it is possible to tell flow to look into src instead it needs to be done per project directory containing .flowconfig which implies every user of library would need to do that to get type annotations so it's more convenient to use this approach instead.
• typed code uses module import / export syntax. Flow does have some limited support for export. / require but it's not idiomatic and those modules can't be imported which won't play well with rest of the flow ecosystem. Also since there is still a babel step to remove type annotations it did not seemed there was much value in sticking to require, but if needs to be this could be changed back to require / exports..
• npm start will track changes in src and will automatically reflec them in lib. This makes work with multiple linked packages comparable to working with liken packages that are in pure js, whith only diff that you need to run npm start in the packages you're making changes in.
• Miss-match in error file / line issue was mentioned as a pain point. I (or rather tools like https://quokkajs.com/) in node tend to use node-source-map-support library to provide stack traces mappen in node. While it's certainly possible to include and enable it per package bases I don't think it makes much sense, again because browsers do it for you and in node project user could manually enable that as well. It might make sense to mention it in readme but I thought we can discuss this here first.

Issues

• I failed to figure out how aegir is configured here. I did not find .aegir.js file so I assume default are used here, which unfortunately:
  • Seems to conflict with typed code in src. Not sure if moving typed code elsewhere and emitting untyped into src makes more sense or configuring aegir differently instead.
  • I wanted to setup npm test to do type checking as well. I tried to get aegir somehow run flow check but without success, so instead I've added flow check into .travis.yml but I don't think that makes much sense. I think running type checker before submitting pull is probably a good idea.
  • aegir lint this is unfortunately a problem I did not expect and according to @victorbjelkholm is very important. From what I understand you use http://standardjs.com/ which unfortunately does not support flow out of the box. So in the past after discussing it with a standard team I end up making standard-flow. Which kind of worked but I'm not sure how to plug it into aegir. I also tried to make argument towards https://prettier.io/ which can be configured to output code if not fully standard compliant at least one that I can't distinguish, it handles types much better than standard does and is also output is consistent regardless of input, in my experience standard often preserves input which can lead to not really standard output which still lints. It's also worth mentioning that standard provides bunch of lint rules that are extremely helpful but redundant and sometimes get in your way when working type checker, since later has much better understanding of code.
  • aegir docs seems to generate docs although it seems to ignore flow type annotations. Again I'm guessing it is a config problem somewhere, but I'm not sure where or how to change that. It is also worth pointing out that I have mixed success using http://documentation.js.org/ for doc generation, if I recall correctly imported types from other libraries were sort of opaque. That being said it's probably not going to be better than without annotations anyway.

Notes

• You'll notice exported types like export type Multihash = Buffer that does not really do anything other than allow us to add semantic info that it's not just Buffer but rather it is one that represents Multihash. In the future I would like to use nominal types which would actually type check that Multiphash is passed rather than arbitrary Buffer but after discussing this with @vmx on IRC we concluded it's probably best to do it in the next step.
• exported codes, names and defaultLength are now typed such that names.foo will be a compile time type error, property would have to match one of the names supported. In fact coerceCode does that too now. It kind of makes runtime checks obsolete, but disabling those would make only sense if users of this library also type-check.
• isAppCode (code:number):boolean and isValidCode(code:number):boolean are instances of the functions that aren't very type friendly. I had lengthy discussion about this with @vmx on IRC. Problem is that for type checker if (isValidCode(code)) { code } predicate does not really refine type of code in the if block. Type friendly version would be something like:

  declare toCode function(x:mixed):?Code
  
  const code = toCode(x)
  if (code != null) {
    // now type checker knows code is valid `Code`
  }

  Note: Obviously naming of toCode isn't really important, it's just from the return of that function type checker knows that code is either null or Code and since it's not null it is Code. I would like to expose toCode and toAppCode style functions along with predicates otherwise every use of predicate would be forced to do this to type-check (and you want to reduce usage of any as much as possible):

  if (isValidCode(x) {
    const code:Code = (x:any)
    // ...
  }

[vmx]

I'm new to flow, so my questions might be pretty basic.

Setup

While it is possible to tell flow to look into src instead it needs to be done per project directory containing .flowconfig which implies every user of library would need to do that to get type annotations so it's more convenient to use this approach instead.

So the lib/index.js.flow is identical to src/index.js, isn't it? I deleted the lib/index.js.flow, edited the src/index.js and introduced a type error. Then I ran npm run type-check the error was correctly reported. Why is the lib/index.js.flow needed?

typed code uses module import / export syntax.

As we use Babel anyway, I agree that using import/export syntax is a good idea.

This makes work with multiple linked packages comparable to working with liken packages that are in pure js, whith only diff that you need to run npm start in the packages you're making changes in.

So you run a watcher script on every package you've linked, right? That surely is an additional step and might have surprising consequences if you forget about/don't know that a module is a flow based one. But I think with some tooling it might be possible to run a single script from the repository you've linked from.

Miss-match in error file / line issue was mentioned as a pain point.

For me it's a must that things are as easy as possible for new-comers. If you get a stacktrace you should be able to look at exactly those files to fix the issue. I contribute a lot to random open source projects I happen to use. There it is very important that it's easy to get started, without knowing the exact setup for development.

Issues

I wanted to setup npm test to do type checking as well. I tried to get aegir somehow run flow check but without success

Once we agreed on things, I can help getting things work with Aegir.

aegir lint this is unfortunately a problem I did not expect and according to @victorbjelkholm is very important. From what I understand you use http://standardjs.com/ which unfortunately does not support flow out of the box.

I agree that it is important. According to a pull request on your repository Gozala/standard-flow#4 standard does now support flow. So this might be a non-issue.

aegir docs seems to generate docs although it seems to ignore flow type annotations. … That being said it's probably not going to be better than without annotations anyway.

I only know that documentation.js supports flow, which would make the JSDoc annotations less redundant. But I think that's a minor thing. As the current setup doesn't make the documentation worse, we can have a look later on if there are ways that it could improve the docs/make it simpler.

My comments

What I'd like to see is that someone who has a first look at the library ideally doesn't need to have any idea about the tooling to fix simple bugs. A developer would only care about the src directory, the rest will be done automatically. On most npm run commands it will run Babel beforehand. In the README there will be a note that it makes sense to run npm start to have things as smooth as possible.

To me it looks already pretty good with little pain points. Though I haven't worked in such a setup yet, so please chime in if I'm missing something.

[Gozala]

So the lib/index.js.flow is identical to src/index.js, isn't it? I deleted the lib/index.js.flow, edited the src/index.js and introduced a type error. Then I ran npm run type-check the error was correctly reported. Why is the lib/index.js.flow needed?

That is entirely for other packages depending on this and using flow. So when other package has import * from multihash flow will first look at multihash/lib/index.js.flow and then fallback to multihash/lib/index.js.

So you run a watcher script on every package you've linked, right? That surely is an additional step and might have surprising consequences if you forget about/don't know that a module is a flow based one. But I think with some tooling it might be possible to run a single script from the repository you've linked from.

Sure it's additional step and one point or the other you might forget to do it, but not on every single package you linked, just packages you are making changes in. And you yes you probably could automate this by making watches watch every linked package but in my experience this problem was so rare that it did not bothered me enough to make such tool.

For me it's a must that things are as easy as possible for new-comers. If you get a stacktrace you should be able to look at exactly those files to fix the issue. I contribute a lot to random open source projects I happen to use. There it is very important that it's easy to get started, without knowing the exact setup for development.

I am not sure which option do you feel is more appropriate here. I really only run into this if I run node repl and we could add npm strat repl that fixes that for instance, but again I think the best thing would be to point to node-source-map-support lib as users might be using arbitrary tools to do development.

Once we agreed on things, I can help getting things work with Aegir.
👍

I agree that it is important. According to a pull request on your repository Gozala/standard-flow#4 standard does now support flow. So this might be a non-issue.

It did used to have that support in the past as well but it was really bad it was reporting issues where there were none and was completely ignoring type definition blocks see

• standard flow standard/standard#601
• Allow configuring plugin rules standard/standard#600

It seems there are still open issues:

• https://github.com/standard/standard/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aopen+flow

In fact @feross suggested to do standard-flow in first place to cover all this see:
standard/standard#592 (comment)

With that said I am happy to just use standard with a flow config as mentioned there, or use standard-flow, but I would really recommend giving prettier a chance as it just works and makes any style discussions obsolete, but then again that's not what I'm trying to sell here so whatever works for you, I can till secretly use prettier in my editor.

What I'd like to see is that someone who has a first look at the library ideally doesn't need to have any idea about the tooling to fix simple bugs.

Yeah that's an intention

A developer would only care about the src directory, the rest will be done automatically. On most npm run commands it will run Babel beforehand.

I think that's probably should be part of aegir test but then I could use help there. I can change most scripts to aegir build & aegir test if you like but not sure if that would make sense.

I'm also unsure about aegir release I imagine that would have to be changed as well.

BTW aegir test should probably also setup node-source-map-support to include source mapped reports.

In the README there will be a note that it makes sense to run npm start to have things as smooth as possible.

Please note that it's only really necessary if you're working in package a and have package b linked where you decided to make change. In such case you need to either run npm start in package b or do npm run build. That not to imply we don't need that in Readme but rather to point out that tooling comes into play in cross-package changes.

[Gozala]

@vmx BTW I'm not sure what the actionable todo list here is, if we could make one that would allow me to push forward.

[vmx]

We (@Gozala, @victorbjelkholm, @vmx) had a discussion on IRC. I'll quickly post the most important outcomes:

• Having the *.js.flowin lib which are a copy of the src/*.j files isn't ideal, but the current way of doing it. It would be better to use flow gen-flow-files which just generates the types without copying the source. But that's still experimental and buggy.

• The node-source-map-support package should be included and automatically be "required" in the scripts, e.g. when running the tests.

• Standardjs doesn't seem to be 100% compatible with flow. So we should perhaps just drop/violate the parts where standardjs doesn't play well. @Gozala proposed using Prettier instead. You won't need linting as it automatically formats the code for you.

• @Gozala will adapt this repo to use the tools for testing, linting (resp. the replacement) and coverage reports. It won't use Aegir for now as we only want to see it work. It will be integrated into Aegir at a later stage.

[Gozala]

I apologize for not following up on this sooner, just had some major things come up on other fronts that prevented me from this. I plan on updating this pull to do what @vmx summarized above today.

[Gozala]

Overview / Notes of the followup changes

• Added test/node file which loads up source-map-support/register so that errors in test will be source-mapped and stack traces would point to right locations in src rather than to compiled ones in lib. Please not that ideally aegir test would pass --require source-map-support/register to mocha instead.
• I run prettier + flow check in place of lint. Note that prettier currently runs only over the staged changes that is to avoid reformatting existing code. IMO It would make more sense to run it over the whole thing instead, but I'd do that as a followup so that formatting changes aren't part of diff here. Also aegir lint should probably do that in case it's flow typed project, but that was out of scope here.
• I had to config aegir with .aegir.js so that webpack would use babel-loader when building. Probably aegir should just do it whenever .babelrc is present instead.
• Currently different babel config is used for UMD builds, I mostly needed to avoid inlining source maps, and chose to compile to a version that would support last two releases of browsers. I suspect decision on what browsers to support in builds belongs in an aegir instead and requires separate discussion.
• I have verified that errors thrown from src/index.js are reported with a proper file path and line numbers when test are run in node. I also noticed that in browser tests reports don't contain info on where the error was thrown from so I assume things are as is for browser tests. Please let me know if I'm mistaken.

[vmx]

Thanks @Gozala that looks really good. Could we use prettier-standard instead of prettier? This way we should be able to use the standardjs linting + adaption to make it work with flow.

[vmx]

I did run a prettier-standard and then followed the the instruction at https://standardjs.com/index.html#can-i-use-a-javascript-language-variant-like-flow-or-typescript:

npm install babel-eslint eslint-plugin-flowtype
npx standard --parser babel-eslint --plugin flowtype

The only complaints I got were:

/home/vmx/src/pl/js-multihash/src/constants.js:345:20: '$Keys' is not defined.
/home/vmx/src/pl/js-multihash/src/constants.js:346:20: '$Values' is not defined.

It don't understand those, but I'm sure @Gozala can make sense of those.

[Gozala]

Thanks @Gozala that looks really good. Could we use prettier-standard instead of prettier? This way we should be able to use the standardjs linting + adaption to make it work with flow.

I have not personally used it but everything I used that tries to adapt standard to use with flow runs into the same incompatibility issue between choices standard and flow project made which shows up in different form:

1. Standard rules need to be relaxed is it conflict with choices in flow.
2. Standard rules aren't taking flow into account and often times ignore or produce strange formatting for some definitions.
3. Standard mistakes some flow syntax for JS and reports an issue where is none.

I am sure with enough effort you could get a working setup, but then again as I described elsewhere fundamentally standard attempts to format and lint your code, where's prettier does just formatting and flow does type checking (which can be though of as linting) there for there are no overlap and no issues and outcome is mostly same.

The only complaints I got were:

/home/vmx/src/pl/js-multihash/src/constants.js:345:20: '$Keys' is not defined.
/home/vmx/src/pl/js-multihash/src/constants.js:346:20: '$Values' is not defined.

It don't understand those, but I'm sure @Gozala can make sense of those.

My guess is that standard is not aware of built-in flow utility types $Keys and $Values and mistakes them for JS values that are referred to but never defined or imported.

P.S.: There is unfortunately no way in flow to import built-in types like that.

[Gozala]

To be clear I have no interested in solving issues between flow / standard incompatibilities for several reasons:

• This is not what I'm after.
• I tried my best in that regard with standard-flow and it still not great (I have pointed out issues with standard's flow support on one of the threads).
• Fundamentally there is an overlap (and there for surface for conflicts) between standard and flow work. Every addition to flow would require standard to adapt.
• There is no interest from standard to get first class support for flow.

If you decide to stick to standard, so be it I'd rather just add eslint-disable statements where standard conflicts with flow and leave standard+flow to someone more interested to resolve.

[vmx]

@Gozala I've to explain why I was so keen on standardjs, it was due to a misunderstanding (sorry for that). I'm still new to modern JS toolchains. I though most of the linting came from the standardjs rules. But when I checked the code I saw that many of them come from AEgir (https://github.com/ipfs/eslint-config-aegir/blob/master/index.js).

Also when I was talking about standardjs I actually meant "standardjs conform formatting" and not the formatter.

Although I'm kind of interested being standardjs conform, surely being 100% conform shouldn't block this effort.

Edit: Though I think using prettier-standard might be a good idea, on my machine the difference between the current code and the prettified one was quite OK (way less different that with the normal prettier)

[vmx]

For me it looks good. Could someone with more experience have another looks to see if there's anything missing or if it is even good to go? /cc @diasdavid @victorbjelkholm

[Gozala]

@Gozala I've to explain why I was so keen on standardjs, it was due to a misunderstanding (sorry for that). I'm still new to modern JS toolchains. I though most of the linting came from the standardjs rules. But when I checked the code I saw that many of them come from AEgir (https://github.com/ipfs/eslint-config-aegir/blob/master/index.js).

You were not actually wrong as standardjs is essentially set of rules+tools on top of eslint. But then again linting is naive type checking and I really don't see much value in using linter along with type checker as they more likely that not going to disagree on something.

Also when I was talking about standardjs I actually meant "standardjs conform formatting" and not the formatter.

Although I'm kind of interested being standardjs conform, surely being 100% conform shouldn't block this effort.

I don't want to leave an impression that I'm against standardjs or pushing for prettier. Just trying to provide my viewpoint formed by prior experience, but happy to back off and use whatever you folks choose, as long as I'm not forced to work out standard / flow incompatibilities.

[vmx]

You were not actually wrong as standardjs is essentially set of rules+tools on top of eslint. But then again linting is naive type checking and I really don't see much value in using linter along with type checker as they more likely that not going to disagree on something.

Agreed about types. I just want the additional linting (which is mostly ESLint setting set by AEgir, not standardjs) like wrong spacing, left-over console statements etc. I also would like (though I don't know if that's really important) to be able to put the "standardjs compatible" badge on it, so people can code that way and the result won't be much different from the prettier-standard run.

I don't want to leave an impression that I'm against standardjs or pushing for prettier. Just trying to provide my viewpoint formed by prior experience, but happy to back off and use whatever you folks choose, as long as I'm not forced to work out standard / flow incompatibilities.

I really appreciate your experience with Flow. You spend already so much time on this, which is great. You don't have to care about small details like fighting Flow - standardjs incompatibilities. I'll take care of those issues. Though I might ask yout do adapt things when I find how ways to work around those incompatibilities (then I again feedback from you is again appreciated as I might find things that are not practical).

So what I think is left is (please let me know if you disagree and/or don't find it important enough so that you want to spend time on these):

• Switching from prettier to prettier-standard
• Get linting back that is not just type-checking. Probably that's using AEgirs rules and doing a plain ESlint run

[vmx]

Could others (@diasdavid, @victorbjelkholm, @pgte) please chime for:

• How far of are we from merging this. Is this a way we should pursue?
• Am I bikeshedding too much and put too much load on @Gozala and instead should help out more myself?

@pgte: If you've thought's about using Flow or the current state of things, feedback is appreciated. I don't think you need to read the full issue, just the current state of things. But please don't feel obliged, I just thought you might be interested in this topic.

[daviddias]

Reviewing this PR now and I've some questions, however, given the amount of information here I want to apologize in advance if these were already answered and I missed it. With this PR:

• Devs will always running a transpiled version, will other modules be able to pick up on the Flow types this way with a simple require('multihashes')?
• Scenario: A user is developing on module that depends on this one and needs to make a change on multihash. Will the user have to npm run build every time it makes a change? How fast is this build? Is there a way to make it automatic?
• The Webpack config has changed, seems that now there is no longer a built + bundled version on dist. Both should exist (dist and lib).

[Gozala]

Devs will always running a transpiled version, will other modules be able to pick up on the Flow types this way with a simple require('multihashes')?

Shortly Yes! That is the reason why *.js.flow are placed in the lib.

Scenario: A user is developing on module that depends on this one and needs to make a change on multihash. Will the user have to npm run build every time it makes a change? How fast is this build? Is there a way to make it automatic?

Currently user needs to run npm start for on packages it's editing and rest should be taken care of. So if user needs to modify multihash best way is to run npm start in the multihash local repo.

The Webpack config has changed, seems that now there is no longer a built + bundled version on dist. Both should exist (dist and lib).

I'm not sure I do understand what you're saying here. But npm run build does generate both dist and lib as it runs both builds:

https://github.com/multiformats/js-multihash/pull/47/files#diff-b9cfc7f2cdf78a7f4b91a753d10865a2R11

I also believe that browser test do run from dist bundle.

[Gozala]

Agreed about types. I just want the additional linting (which is mostly ESLint setting set by AEgir, not standardjs) like wrong spacing, left-over console statements etc.

prettier will take care of spacing during commit. I suppose no left-over console is valid argument for linting.

I also would like (though I don't know if that's really important) to be able to put the "standardjs compatible" badge on it, so people can code that way and the result won't be much different from the prettier-standard run.

I would just replace standard badge with prettier. Point of the prettier is really to get rid of formatting rules, don't even think about formatting prettier will make sure it's right. I'd frame it how much time / mental effort would you like your contributors to spend on formatting, prettier gives you none, with standard in my experience it more than none (flow makes it even worth).

I really appreciate your experience with Flow. You spend already so much time on this, which is great. You don't have to care about small details like fighting Flow - standardjs incompatibilities. I'll take care of those issues. Though I might ask yout do adapt things when I find how ways to work around those incompatibilities (then I again feedback from you is again appreciated as I might find things that are not practical).

Thanks for the kind words.

So what I think is left is (please let me know if you disagree and/or don't find it important enough so that you want to spend time on these):

• Switching from prettier to prettier-standard

I personally question the benefits and worry of the incompatibilities that would still arise. That being said, but give the devotion to standard it's probably best to do just that.

• Get linting back that is not just type-checking. Probably that's using AEgirs rules and doing a plain ESlint run

I would prefer to leave this and AEgir side of things to someone else, but if this going to be blocked by that I'm willing to take a stab at it.

It's just the whole thing started as I want to build something like Dat on top of IPFS and figuring out how to configure ESlint is so far from my primary objective that I have hard time incentivizing myself.

[daviddias]

I'm getting pretty convinced that this can indeed work for our entire module codebase. We will need an updated version of https://github.com/ipfs/community/blob/master/js-code-guidelines.md (it is about time anyway).

@pgte @olizilla You have spoken some thoughts and concerns about injecting a type system. Could you go through all the notes and share with us your feedback?

[vmx]

I would prefer to leave this and AEgir side of things to someone else, but if this going to be blocked by that I'm willing to take a stab at it.
It's just the whole thing started as I want to build something like Dat on top of IPFS and figuring out how to configure ESlint is so far from my primary objective that I have hard time incentivizing myself.

This is what I suspected :) I will take core of the "additional linting" part. I don't wont to over-stress your commitment for getting us into the typed world.

[daviddias]

My apologies for taking a while to come back here and thank you @Gozala and @vmx for keep pushing on this one :)

I'm fairly happy with the answers I got so far, I would just like to see this transformation happen to a module that uses multihash (i.e js-cid, as planned in https://github.com/ipfs/community/issues/278#issuecomment-361002950) so that we can have a shared understanding how it all works across modules.

This is also a significant change to push throughout the module codebase and so, I won't feel comfortable until I have heard at least from 2 or 3 more from the active JS contributors (@Stebalien, given your love for types, I would like your thoughts too :)).

[daviddias]

@Gozala @vmx given that this enables us to do dynamic imports, does this mean that we finally can also do treeshaking? (ipfs/js-ipfs#951)

[vmx]

I'm cross linking @alanshaw comment on the same topic: https://github.com/ipfs/community/issues/278#issuecomment-361219723

[Stebalien]

@diasdavid as long as this doesn't introduce a high barrier to entry or make it difficult to using external libraries, I don't really have much of an opinion.

[vmx]

@Gozala: I've some good news. After talking to several people. We want to give the Flow types a try. Thanks for all your work so far! I'll be leading this endeavour. I'll create a separate issue which will outline the plan and further discussions.

[vmx]

I just created the issue for the Flow Awesome Endeavour. Further discussions about the general approach should take place there. The discussions here will be related to this specific PR. ipfs/js-ipfs#1260

[garrensmith]

Would it be possible to also add the type checking to the npm start command? It would be nice to be warned if there are any type errors introduced.

[Gozala]

In practice there should be impossible to introduce type errors as that would fail In CI and there for won’t be merged.

Also given that initail run of type checking takes quite bit time I don’t think running that on start would be a good option.

[garrensmith]

I don't think this file is meant to be in this PR.

[garrensmith]

I'm working on converting js-multihashing and I think that code parameter here should be code: Code | Name

export function encode (digest:Buffer, code:Code | Number, length:number):Multihash {

[vmx]

This PR is close to be ready. One thing I'd like to see is having it use prettier-standard instead, so that people will still have the source in the format they are used it.

Ideally it's two separate commits. One doing one run of prettier-standard and the other one being this current PR.

I'm happy to do that work myself, though it might slip into tomorrow. If anyone else wants to help out (even if it's later that week), please leave a comment, it would be appreciated.

[fahrradflucht]

Why was rollup introduced / where does it get used?

[vmx]

I wondered the same. I'm in the process of pushing an updated version of this PR. There I don't use the rollup module and it still seems to work.

[fahrradflucht]

I had bad experiences with a lint-staged configuration like this in the past. The main problem is that when you only stage patches of a file which get changed by prettier --write afterwards the whole file is staged.

[vmx]

I didn't want to destroy this PR's history, hence I've created a new one, please review #49.