Idea for decoder type

[andywhite37]

This is purely for discussion/passing consideration, but I was poking around with the code, and I noticed it might be interesting to define a type for the decoder function Js.Json.t => M.t('a)

One way to do it would be like this (the type annotations were for me - you can leave them out):

  // inside DecodeBase
  type t('a) =
    | Decoder(Js.Json.t => M.t('a));

  // run function to apply the json input and produce the M result
  let run: (Js.Json.t, t('a)) => M.t('a) =
    (json, Decoder(decode)) => decode(json);

  module Functor: FUNCTOR with type t('a) = t('a) = {
    type nonrec t('a) = t('a);
    let map = (f, Decoder(decode)) => Decoder(decode >> M.map(f));
  };

  module Apply: APPLY with type t('a) = t('a) = {
    include Functor;
    let apply: (t('a => 'b), t('a)) => t('b) =
      (Decoder(f), Decoder(decode)) =>
        Decoder(json => M.apply(f(json), decode(json)));
  };

  module Applicative: APPLICATIVE with type t('a) = t('a) = {
    include Apply;
    let pure = a => Decoder(_ => M.pure(a));
  };

  module Monad: MONAD with type t('a) = t('a) = {
    include Applicative;
    let flat_map: (t('a), 'a => t('b)) => t('b) =
      (Decoder(decode), f) =>
        Decoder(json => M.flat_map(decode(json), a => f(a) |> run(json)));
  };
  
  // ... etc

Another way would be to leave out the data constructor Decoder and just make it an alias. I don't think you'd have to change much at all if you just did this.

type t('a) = Js.Json.t => M.t('a);

Doing that kind of cleans up the signatures for map/apply/bind/etc, and makes the implementations maybe a little more clear in that the Js.Json.t => M.t('a) bit would be hidden inside a type, rather than repeated. It might also make it more obvious what your core decoder type is for those new to the library - at its core it's just a Js.Json.t => M.t('a) function.

I'd also point out that the decoder function 'a => M.t('b) is basically the same as the definition of ReaderT. https://github.com/reazen/relude/blob/master/src/Relude_ReaderT.re#L10 - there might be some things to reuse from Relude or some general ideas that might come out of that. ReaderT is an abstraction where you can compose functions that will eventually be supplied some "environment" value in order to run and produce the result. In this case the "environment" is the json value.

[mlms13]

I'm definitely not opposed to this. I'm pretty sure Elm's decoders have a newtype wrapper like that, and I honestly don't remember why I chose to go the plain function route.

If you want to take a stab at this, go for it. Personally, I probably won't try any major refactors until:

• tests are organized and test coverage has improved (Reorganize tests and improve coverage #39)
• more DecodeBase functions are rewritten in terms of Decode.map, flatMap, alt (Add Alt for decoder functions #36, Don't use eager/parallel alt function #37), etc, instead of the current situation where most internal functions actually run the decoder, then work with the output monad

The second point in particular should make it easier to try out a different type for the decoders without touching as much code.

Anyway, I don't feel super strongly about this either way. It'd be fun to play with it and see how much it changes downstream code. I'm slightly inclined to try to get bs-decode to a stable 1.0 (#38) with very few changes to the current API, then maybe start looking at this (and other bigger changes that we've talked about, like abstracting out the Js.Json part, adding encoders (#31), and anything else that comes up).

[mlms13]

Ah, I vaguely remember why we didn't do this. Because it's common to locally-open Decode.AsResult.OfParseError.(...), adding a type t('a) to the decoder itself will shadow any other type t that's in scope, which you may have been referencing inside your decoder.

For example:

type t = {
  firstName: string,
  age: int,
};

let decode = Decode.AsResult.OfParseError.(
  (firstName, age) => ({ firstName, age }: t) // <-- this `t` is now a type error
  <$> field("first_name", string)
  <*> field("age", intFromNumber)
);

But still, that's more of a note for the changelog, not a reason to avoid this change, especially for a major v2 version bump. The easy fix is to define an explicit make function and put your : t annotation there, if it's really needed.