feat: add flow annotations

[garrensmith]

This adds flow annotations to multihashing.

This is part of ipfs/js-ipfs#1260

[victorb]

Rather than having this file (.babelrc) and .flowconfig, could we have these in a separate repository and import here? Would be a lot better, otherwise this is gonna be duplicated everywhere.

[garrensmith]

I think ideally we should move these into AEgir

[Gozala]

Sadly .flowconfig isn't js or json so you can't really pull it from elsewhere (unless you're thinking of git submodules which aren't worth the hassle IMO).

As of .babelrc indeed this probably should end up in AEgir.

[garrensmith]

The tests will fail until multiformats/js-multihash#47 is merged in since I use some of the types based off of that PR.

[vmx]

I hope that it's still possible without too much hassle. Could you please use prettier-standard, it matches the current code style better. I would also suggest to make it two separate commits. First the prettier-standard run without any other changes and then the commit with the actual annotation and tooling changes?

[Gozala]

Sadly .flowconfig isn't js or json so you can't really pull it from elsewhere (unless you're thinking of git submodules which aren't worth the hassle IMO).

As of .babelrc indeed this probably should end up in AEgir.

[Gozala]

Is this init call intended here ? I don't see it in original code.

[garrensmith]

Sorry that was a mistake. I was testing the flow checking.

[Gozala]

One other way to do it would be to add another table in js-multihash which would be {[number]:Code} and then do const code = thatTable[minB + i]. That being said it's probably fine the way it is here.

[Gozala]

I would suggest export default Multihashing

[garrensmith]

Originally I had export default Multihashing but that changes the api of this library for commonjs because instead of doing const multihashing = require('multihashing') a user now have to do const mh = require('multihashing').default which I don't think we want to do with the initial flow conversion.

[Gozala]

@garrensmith In fact flow-node babel preset takes care of this as it includes add-module-exports plugin only thing to watch out for is that you can either single default export or multiple non default exports (as there is no way around it with common-js style).

[Gozala]

This is aside comment more on the API design. I would very much encourage to either have two different functions one that works with codes the other with names or better yet settle on canonical representation and make coercion a consumer's concern. The fact that IPFS libs tend to take unions of things made by far most difficult to figure out what's being passed around (although types would solve this) and also choose representation to use myself.

[garrensmith]

@vmx I think we should open up an issue around this. Something that could be added in another PR.

[vmx]

Yes. Please open issues if you find weirdness in the API. Adding types is surely a good reason to review the APIs.

[Gozala]

In general I would really recommend encoding such invariants in a type system as primary benefit of type checking is to eliminate possibility of runtime errors. I don't know exact specifics of how this is used so my specific suggestion may not necessarily be applicable here, but it might still clarify what I mean. Usually such invariants can be encoded at the type level by making invalid state unrepresentable. Here is an example:

interface HashUpdate {
  update(buf: Buffer): Hash;
}

interface Hash extends HashUpdate {
  digest(): Buffer;
}


class ShaHash implements Hash {
  // Assumbtion is that this static function is exposed and not the class so that consumer can only create
  // HashUpdate and can't create `Hash` without an input. Since our `HashUpdate` interface does not have
  // a `digest` attempt to call will be reported by a type checker.
  static new(hashFunc, arg?: number):HashUpdate {
    return new ShaHash(hashFunc, arg)
  }
  // Since `update` returns a `Hash` that has both `digest` and `update` methods
  // calling `digest` on returned value will be by fine by type checker. Please also note
  // that even though it's the same instance from type checker perspective former still
  // has not `update` and later does.
  update(buf: Buffer): Hash {
    this.input = buf
    return this
  }
  // It is necessary to check this as `input` can be `null` but either way it's useful to cover
  // consumers that don't use type-checker
  digest(): Sha3Hash {
    if (!this.input) {
      throw Error("Missing an input to hash")
    }
    //...
  }
}

[fahrradflucht]

Those 3 are devDependencies aren't they?

[fahrradflucht]

Same as multiformats/js-multihash#47 (review). No need for rollup I guess.

[fahrradflucht]

More out of curiosity - what was the problem? Shouldn't Code be just an alias for number?

[Gozala]

They were opaque type aliases which behave like nominal types. In that module context any number can be treated as Code but outside of that context Code is a subtype of number and only way to get hold of value of that tipe is by getting it from that module either from exposed constant annotated as Code or via function that returns Code. This gives you a way to say have parseCode : number -> ?Code function that can return passed value back but ensuring that it’s a valid Code. Kind of typically you’d have isCode function but such that type checker then enforce typings