multiformats/js-multihash

More type friendly API

Gozala opened this issue · 0 comments

Give the en-typing effort (see #49) I think it might be a good moment to re-consider some API choices that could further increase benefits of using a type-checker. It also would not make sense to bundle any API changes with en-typing efforts so this issue is probably good place to outline current limitations along with proposed improvements.

I'll provide a list in no particular order (just things that catch my as I was taking a look at #49)

  • Exceptions aren't very type-friendly. Flow might eventually get a support for this, but as things stand now type-checker will not require you to try / catch functions that might throw. And even if you do use catch (e) {...} type checker won't know what the type of e is. It is worth pointing out that common languages with inferred static types primarily model errors via Result<x, a> type which is more or less following:

    type Result<x, a>
      | { value: a, isOk: true }
      | { error: x, isOk: false }

    I would recommend encode / decode functions to return such a Result<x, a> type. On the other hand argument could be made that such API would feel awkward for plain JS consumer. To address that concern I would suggest one or both of the following:

    1. Rust uses .unwrap() for these kind of stuff that would cause panic if result is an error. Our results could also have .unwrap() (or more javascripty .valueOf()) method that returns result.value or throw result.error. So API would be something like decode(buffer).unwrap() for untyped consumer and decode(buffer)//:Result<Error, Multihash> for typed. Please note that typed consumer would likely want to do if (result.isOk) { console.log(result.value) } else { console.error(result.error) } & type checker will be able to infer everything.

    2. Expose functions that return .unwrap()-ed results. In fact current API does not need to change, rather new functions could be exposed that return results and existing API could just call those and return unwrapped results. I personally think that making untyped js to .unwrap() is better option as it makes consumer acknowledge that function might throw, but then again I'm biased and migration to new AP may not be practical.

  1. Predicate functions like isValidCode aren't helpful with flow (ts actually has a some support for that). What that means in practice is that if (isValidCode(x)) { encode(buf, x, len) } will not type check if x is number as at the encode call-site x will still be inferred as number while encode expects Code.

To address this I would recommend replacing isValidCode with (or adding another function) parseCode(number):?Code which would allow type checker to infer everything by rewriting above example as:

const code = parseCode(x)
if (code) {
  encode(buf, code, len)
}

For untyped consumer things would thing won't need to change as if (parseCode(x)) { encode(buf, x, len) } would work just the same.

  1. Function validate is in fact a hybrid of above two, presumably intention is to describe for what reason validation failed. I would propose parseMultihash(Buffer):Result<Error, Multihash> instead which would just wrap buffer in result if it's valid multihash (but will have refined Multihash type) or error result carrying info what it's not valid just like with exception.