adds decoder support to fragment definition

[mbirkegaard]

This adds support for @bsDecoder on fragments + tests. Previously you could add @bsDecoder(fn: "some_decoder"), but it would be removed from the printed query and have no effect.

@baransu We talked about this over in #20.

As far as I understand your problem is lack of @bsDecoder on fragments for example, as it would simplify your workflow when handling e.g Price fragment as in your example?

This PR fixes that.

AFAICT there shouldn't be a problem with adding decoder support to queries as well, but I'm new here so might not know what I'm talking about.

[mbirkegaard]

There should in principle be no problems abstracting this out and using it for fields, fragments and operations.

[baransu]

I'm ok with leaving it as is. A lot will change with #67 and we'll have to do major code cleanup after that.

[mbirkegaard]

Got it.

[mbirkegaard]

There are a couple of different ways of doing this, but to me this seemed the most legible to newcomers to the library.

[jfrolich]

Do I understand correctly this adds a decoder on the fragment spread? Why would you want to do that? You can add decoders on fields in fragments right? (or did that not work correctly and are you fixing that?).

It would be great btw if you can look into doing this PR on top of the records PR (#67). Because a lot of the code has changed, so it would bet tricky to merge this in later.

[mbirkegaard]

Do I understand correctly this adds a decoder on the fragment spread?

I named the PR poorly. It's for adding a decoder to a fragment definition.

You can add decoders on fields in fragments right? (or did that not work correctly and are you fixing that?).

You can add decoders on fields. Decoders on spreads (with or without @bsField) do nothing. For instance,

query {
  someField {
    ...SomeFragment @bsDecoder(fn: "someDecoder")
  }
}

is equivalent to

query {
  someField {
    ...SomeFragment
  }
}

That should probably also be fixed, but this PR a'int for that.

Why would you want to do that?

(I'm here answering the question assuming it's about decoders on fragment definitions.)

A couple of reasons. Firstly, I just think it should be possible to add @bsDecoder anywhere and have it work. There's currently a number of odd inconsistencies or unclear rules on where decoders work.
e.g

query {
  someField @bsRecord {
    ...SomeFragment
  }
}

gives an error message: @bsRecord can not be used with fragment spreads, place @bsRecord on the fragment definition instead.

(You're working on introducing records (which is great BTW!), so this exact point won't be relevant for long. My thoughts on @bsDecoder don't change though.)

Anyway... I can add @bsRecord to the definition of SomeFragment, but as fragment someFragment on someType @bsRecord @bsDecoder(fn: "myDecoder) is the same as fragment someFragment on someType @bsRecord any decoding that I want to do I have to add on someField:

query {
  someField @bsDecoder(fn: "myDecoder") {
    ...SomeFragment
  }
}

and I have to worry about whether someField is required, nullable, an array, etc.

A concrete use case from the app I'm currently working on (the one I also talked about in #20) is that we have a price concept. A price can be either a definite price or a price range. We're defining a module as follows:

module Price = {
   type t = 
     | Price(int)
     | PriceRange(int, int);

  include [%graphql {|
      fragment price on Item {
         price
         priceRange
         minPrice
         maxPrice
      } 
  |}];

  let decode = obj => {
    switch (obj##priceRange, obj##price, obj##minPrice, obj##maxPrice) {
      | (Some(true), Some(price), _, _) => Ok(Price(price))
      | (Some(false), _, Some(min), Some(max)) => Ok(PriceRange(min, max))
      | _ => Error({j|error decoding price: $obj|j})
    }
  };
};
}

In every query or other fragment that wants to use Price, we have to expose the inner workings of the price module so that the decoder can be added there. Having the fragment know how to decode itself is really powerful.

It's also convenient: If there's one query that uses the fragment with an optional field and another that uses it with a list these would have the correct types already. As it is now, each of those queries have to get the SomeFragment.decode function and wrap it in Option.map or Array.map.

It would be great btw if you can look into doing this PR on top of the records PR (#67). Because a lot of the code has changed, so it would bet tricky to merge this in later.

You might be right. I am worried however, by how long it'll take take to finish #67. This feature would be really useful to have in a minor version (for me at least). Unlike #67 it's non-breaking. I'd rather have this merged and released, maybe just as a release candidate, instead waiting for the records rewrite to be finished (even if it just takes another couple of weeks).

I'd be happy to re-implement this once #67 has been finished and merged.

[baransu]

I think this is good. As @jfrolich mentioned it would be great to have it on top of #67 but we're not sure when they will be released.

[baransu]

I'm ok with leaving it as is. A lot will change with #67 and we'll have to do major code cleanup after that.

[baransu]

@jfrolich It's much better to have automated steps for converting GraphQL raw data into business logic types.

In my example, I'm using ID module which hides internal string representation. This way I'm unable to mix different types of ids coming from different types of data. Of course they are on single string field which is the same as parsing Js.Json.t scalars into something useful but combining multiple fields into one data structure is also very useful.

[jfrolich]

Interesting... You example helped 😁 I think this would be pretty nice but wouldn’t it be better to collocate this with the fragment instead of where the fragment is being spread?

[baransu]

I think having it on the fragment is much better because you're just using the fragment and interact with it via public API of the Price module as in @mbirkegaard example. You don't have to care about internal representation, just use it 😉

[jfrolich]

Basically adding the decoder to the fragment definition. This means that it always has the same fields, and where the fragment is used, it is decoded automatically.

[jfrolich]

Lol sorry I am on my phone and looking at the other example. I see that it is on the fragment definition. That sound great. Forget my previous comments :)

[baransu]

LGTM 🎉