feat: add flow annotations #27

garrensmith · 2018-04-03T14:38:02Z

This adds flow annotations to multihashing.

This adds flow annotations to multihashing

victorb · 2018-04-03T14:39:50Z

.babelrc

@@ -0,0 +1,34 @@
+{


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.

I think ideally we should move these into AEgir

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 · 2018-04-03T14:45:25Z

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 · 2018-04-03T14:48:32Z

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 · 2018-04-03T16:40:45Z

.babelrc

@@ -0,0 +1,34 @@
+{


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 · 2018-04-03T16:44:57Z

src/blake.js

  init: blake.blake2bInit,
  update: blake.blake2bUpdate,
  digest: blake.blake2bFinal
 }

-var blake2s = {
+blake2b.init(1, 1)


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

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

Gozala · 2018-04-03T17:05:45Z

src/blake.js

  }

-  var i
+  // I don't like using any here but the only way I could get the types to work here.


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 · 2018-04-03T17:09:17Z

src/index.js


-const mh = module.exports = Multihashing
+const mh = (module.exports = Multihashing)


I would suggest export default Multihashing

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.

@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 · 2018-04-03T17:17:46Z

src/index.js

@@ -26,38 +37,38 @@ mh.digest = function (buf, func, length) {
  return digest
 }

-mh.createHash = function (func, length) {
+mh.createHash = function(func: Name | Code): Hash {


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.

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

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

Gozala · 2018-04-03T17:43:51Z

src/sha3.js

+  digest(): Sha3Hash {
+    if (!this.input) {
+      throw Error("Missing an input to hash")
+    }


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 · 2018-04-08T07:51:34Z

package.json

@@ -33,16 +43,25 @@
    "url": "https://github.com/multiformats/js-multihashing/issues"
  },
  "dependencies": {
+    "babel-cli": "^6.26.0",
+    "babel-core": "^6.26.0",
+    "babel-loader": "^7.1.4",


Those 3 are devDependencies aren't they?

fahrradflucht · 2018-04-08T08:01:06Z

package.json

+    "flow-copy-source": "^1.3.0",
+    "lint-staged": "^7.0.2",
+    "pre-commit": "^1.2.2",
+    "rollup.config.flow": "^1.0.0",


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

fahrradflucht · 2018-04-08T08:11:18Z

src/blake.js

  }

-  var i
+  // I don't like using any here but the only way I could get the types to work here.


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

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

feat: add flow annotations

3f23a11

This adds flow annotations to multihashing

victorb reviewed Apr 3, 2018

View reviewed changes

vmx mentioned this pull request Apr 3, 2018

Proposal to move to move to Flow types ipfs/js-ipfs#1260

Closed

8 tasks

Gozala approved these changes Apr 3, 2018

View reviewed changes

garrensmith added 2 commits April 5, 2018 17:01

improvements on the types based on review

fe4c553

remove prettier and use standardjs for linting

e6f7f0e

fahrradflucht suggested changes Apr 8, 2018

View reviewed changes

daviddias added the status/deferred Conscious decision to pause or backlog label Aug 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add flow annotations #27

feat: add flow annotations #27

garrensmith commented Apr 3, 2018

victorb Apr 3, 2018

garrensmith Apr 3, 2018

Gozala Apr 3, 2018

garrensmith commented Apr 3, 2018

vmx commented Apr 3, 2018

Gozala Apr 3, 2018

Gozala Apr 3, 2018

garrensmith Apr 3, 2018

Gozala Apr 3, 2018

Gozala Apr 3, 2018

garrensmith Apr 3, 2018

Gozala Apr 3, 2018

Gozala Apr 3, 2018

garrensmith Apr 4, 2018

vmx Apr 4, 2018

Gozala Apr 3, 2018 •

edited

Loading

fahrradflucht Apr 8, 2018

fahrradflucht Apr 8, 2018

fahrradflucht Apr 8, 2018

Gozala Apr 8, 2018


		const mh = module.exports = Multihashing
		const mh = (module.exports = Multihashing)

feat: add flow annotations #27

Are you sure you want to change the base?

feat: add flow annotations #27

Conversation

garrensmith commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garrensmith commented Apr 3, 2018

vmx commented Apr 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gozala Apr 3, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Gozala Apr 3, 2018 •

edited

Loading