Map field decoder

[dillonkearns]

Map field decoder

[xtian]

We've run into this as well. We are basically handling it like this:

import Api.Object
import Api.Scalar
import Graphqelm.Field as Field
import Graphqelm.SelectionSet as SS

type alias SomeRecord =
    { int : Int
    , string : String
    }

selection : SS.SelectionSet (Result String SomeRecord) Api.Object.SomeRecord
selection =
    Api.Object.SomeRecord.selection (Result.map2 SomeRecord)
        |> SS.with
            (Api.Object.SomeRecord.intScalarField
                |> Field.map (\(Api.Scalar.IntScalar string) -> String.toInt string)
            )
        |> SS.with (Api.Object.SomeRecord.stringField |> Field.map Ok)

However this is inconvenient for selection sets with a large number of fields since we have to define our own Result.map<N> functions for any number of fields > 5.

[dillonkearns]

Thanks for sharing the additional use case @xtian, that's helpful! And thanks Sebastian for starting the discussion.

I think I need to think this over some more, as there are a lot of implications and possible considerations here. But here is my initial gut feeling on it.

Based on the use cases you described, Sebastian, I'd be a little concerned about moving the library in this direction to introduce functions to chain decoders and allow the entire response decoder to fail. The goal of the library is to feel extremely safe, and "if it compiles it works," which would be in conflict with this approach. At least in these two use cases it would concern me because it feels like workarounds for problems that would be best served by server-side solutions.

Is there an issue filed for the relay code that generates a schema with edges: [User] (with no !s)? I feel that it's really a feature, not a problem, that this library makes it a little annoying to deal with these types because it causes you to fix the root issue instead of creating a workaround on the frontend. It's best to fix the schema. (Granted, sometimes you don't own the Schema, but hopefully people can report issues in these cases too).

And for the case of the createdAt field, it looks like it's coming back from the server as a String not a DateTime scalar. I haven't used the Rails GraphQL library, but with Elixir/Absinthe you would have a resolver for DateTimes that guarantees that the format is an ISO date.

scalar :datetime, name: "DateTime" do
  serialize &DateTime.to_iso8601/1
  parse &parse_datetime/1
end

(More details in the docs on custom scalar types). There would be no need to check if it's valid and fail the decoder because it's guaranteed to be in that format.

My gut feeling is that maybe there is some middle ground approach that allows you to have a SelectionSet with all Result types and map them into a single Result. But I don't think that would address your concern Sebastian.

I need to do some more thinking on this, let's continue the discussion. I'm very much open to hearing other perspectives on this! I'm still forming my thoughts on it.

[sporto]

@dillonkearns I will try this out hopefully today.

[itsgreggreg]

My take on it is that we have 2 options:

1. Assume that all data that comes in is always and forever valid, because graphql.

   • In sporto's example this would mean that the API could never return a "null" or "[null]" for UserEdge.node
   • In xtian's example this would mean that the API could never return a String that doesn't decode to an Int for SomeRecord.intScalarField

2. Concede that the API could possibly return invalid data at which point we would need a SelectionSetResult type much akin to regular Json.Decode

I've not yet formed a solid opinion on which of these is the reasonable assumption.

[sporto]

I don't quite follow the proposed approach with something like a DateTime scalars in Elm. We do have them in our Api. If understand well, in Elixir the library will parse that for you and give you a record with the date already parsed, is that right?

In ruby we do the same (if the date doesn't parse, ruby will throw an exception).

In the Elm side they are represented as type DateTime = DateTime String. I can think of these options:

Use the scalar as is in the record

So doing nothing resolves to:

type alias Thing = { createdAt: DateTime }

In this case we still need to unwrap and parse the contained string later if we want to do any operations with this.

Unwrap

In some places we unwrap immediatelly using Field.map so end up with {createdAt: String}. So we still need to parse this.

Unwrap and parse

We could parse using Field.map, but will end up with {createdAt: Result Time}. Doesn't seems great as you will need to handle the result for something that is supposed to be valid.

Wrap the whole GraphQL response in Result

Or wrap the whole response in a result like @xtian's example. Nice approach, never occurred to me. But this mean that you will need to deal with this extra Result later.

---

But what you might like is:

type alias Thing =
    { createdAt: Time }

I'm missing something about how something like a DateTime scalar is meant to be used?

[dillonkearns]

@sporto Yeah, my suggestion was just that you use the DateTime scalar rather than just a String to make it a little more clear semantically (and you know then that your backend is enforcing that contract so it will succeed). In that case you can of course use the Debug.crash trick since you're sure it won't fail. But that's not ideal since the whole page crashes if for some reason you're wrong.

I've done some experimentation with your proposed approach @sporto and I'm starting to like the simplicity of transforming values with it. I think I'd like to go with that approach despite the risks.

Ultimately it's a difficult balance because we want to maintain the library's guarantee that if your generated code is up-to-date (or even if it's not but the changes are backwards compatible), the decoders are guaranteed to succeed. I do worry about the idea of a single false assumption invalidating the entire API response because I want to give users something better than the experience of having to rerun your code to see if it's working! I want users to trust that "if it compiles it works," (such a liberating feeling, especially coming from JS!). I also worry about making it too easy to ignore poor schemas rather than fixing them at the source.

On the other hand, data is often unusable anyway if some of your assumptions are incorrect. And it's also common to have certain semantic ideas like DateTimes where we know that our backend will enforce a contract and guarantee that it will have certain properties (like being parseable as an ISO date-time)... and you want to be able to take advantage of those contracts easily. Or sometimes you don't control the API yourself so you're somewhat helpless.

Here's what I came up with for your suggested approach @sporto, I think it is quite nice in a lot of ways. Here's an example:

createdAt : Field Date Github.Object.Repository
createdAt =
    Repository.createdAt
        |> Field.mapOrFail
            (\(Github.Scalar.DateTime dateTime) ->
                Date.fromString dateTime
            )

And this use case of having scalars that should be transformed really does feel like it needs first-class support. It does make this library less pure in a sense, but at the same time much more powerful.

I also played around with some functions for assuming that fields are non-null:

Query.selection Response
    |> with (Query.human { id = Swapi.Scalar.Id "1001" } human |> Field.nonNullOrFail)

releases : SelectionSet ReleaseInfo Github.Object.ReleaseConnection
releases =
    Github.Object.ReleaseConnection.selection ReleaseInfo
        |> with Github.Object.ReleaseConnection.totalCount
        |> with (Github.Object.ReleaseConnection.nodes release |> Field.nonNullOrFail |> Field.nonNullElementsOrFail)

Any thoughts on this? I'm going to be on vacation for a couple of weeks so I won't be able to do any work for a bit. I'll try to wrap up some things before I leave tomorrow!

I think that @xtian's approach of mapping several fields into a single Result may also be something to explore in the future. It could potentially offer users an in-between approach that maintains the safety guarantees yet reduces the number of Maybes and Results. A safer alternative to the ...OrFail functions. I like this idea because it feels like it's inline with Elm's explicitness and the goals of this library. Interesting idea to dig into, but I'll leave that for another time!

Thanks @sporto and @xtian and @itsgreggreg for sharing your ideas and your use cases, it's incredibly helpful, and it really allows me move the library beyond what I could do with just my own perspective. I'm excited that this library is continuing to evolve and improve!

[sporto]

It depends on where do you want to handle possible failures. For me, having a decoder that is guaranted to succeed is not that important if I'm going to have a Debug.crash later or a parsing error. Personally, I rather see the fail earlier in the process.

I really like that approach |> Field.nonNullOrFail or |> Field.mapOrFail if that is doable. Folks can choose not to use this and have the current guarantee.

[dillonkearns]

I published a new package release with these Result mapping functions, let me know what you all think!

http://package.elm-lang.org/packages/dillonkearns/graphqelm/9.1.0/Graphqelm-Field

[dillonkearns]

@sporto or anyone else, any feedback or requests to add here before I close this issue?

[sporto]

I tried these, they work great. Thanks!

[xtian]

We switched to using `mapOrFail` and it’s working well for us. Thank you!

…

On Wed, Feb 7, 2018 at 9:41 PM Sebastian Porto ***@***.***> wrote: I tried these, they work great. Thanks! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <https://github.com/dillonkearns/graphqelm/issues/35#issuecomment-364000774>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAkyHhBrz_BgqTCptywgrh1QjDbdkII3ks5tSnsKgaJpZM4R6UZ9> .

[dillonkearns]

Great, thank you all for the great ideas and feedback. There are some tough tradeoffs here but I'm really happy we examined all the options thoroughly and I'm pleased with this addition to the library. Thank you so much for being so involved and helping improve the library!