Results and Promises

[jeddeloh]

This PR attempts to resolve the outstanding configuration needs: #22. It was originally thought some sort of configuration module and functors would be necessary, but with a few smart choices I think we can avoid it. I went down that path for a bit and we’d definitely like to avoid it!

As it currently stands, this experiment only covers query and useQuery and there are still some things I would like to change or clean up about the current implementation. That said, I'd like to get feedback on the approach before going much further.

Goals

Provide sane promise defaults

We use records a lot, so using a T-first promise library for better type inference would be very helpful. There should also be a way to opt out or provide their own promise transformation function. Here I've chosen to use reason-promise, but how do we deal with the promise opt-out/configuration problem? Well, I don’t think we have to. Ultimately, a Promise.t is just a Js.Promise.t under the hood that won’t reject. If you want to use your own promise, we’ll expose zero-cost conversion functions from Promise.t => Js.Promise.t and vice versa. You’re in no worse situation than you would be if we decided to stick with Js.Promise.t, IMO.

Don’t allow parse exceptions to escape the library

These should never happen if everything is working properly, but there are a bunch of reasons why things might not be working properly :) Ideally this change would be transparent to the developer experience (you just always have optional data) and as such there's no need for any kind of configuration. And we definitely don’t want hide a parse failure as None. Here I've wrapped parse to return a result rather than throwing an exception. We'll get to how we make the change transparent next.

Combine separate failures into one type that can encapsulate any type of failure

So Apollo already provides several types of "results" that allow you to express some sort of success (data) or failure (error or errors). In these cases I don't see why we should be dealing with other types of results. In the case of a promise, it's very easy to fall into this sort of trap:

apolloClient
->ApolloClient.mutation(~mutation=(module AddTodo), ())
->Promise.get(
    fun
    | Ok(_) => Js.log("We're good!") // You forgot to check for errors!
    | Error(_) => Js.log("Something went wrong!"),
  );

We also want to account for parse failures. Both of these failures fall outside of whatever Apollo "result" we're dealing with so in order to account for them we'll need diverge a bit from the official Apollo types. Now any Apollo "result" that contains errors: array(GraphQLError.t) will be lifted to contain an error: ApolloError.t so we can represent all types of failures.

Promise errors become an ApolloError.t with networkError: Some(FetchFailure(...)) on the Apollo "result". What used to be Js.Promise.t(somekindOfApolloResult) that could reject is converted to Promise.t(someKindOfApolloResultWithApolloError). Notice there is no Belt.Result.t in there because we’ve already combined the rejection into this new result. You can only ever resolve with an Apollo result, never reject.

Parse failures get unwrapped before exposing to the user by converting data to None and error to be an ApolloError.t with networkError: Some(ParseError(...)).

Define a type for context so it’s consistent across all operations

I'm explicitly giving up on this one. I don’t feel like it’s worth the effort and I kinda don’t trust that we can ensure context is always what we say it is anyway. I think leaving it up to the user to create a module to parse/serialize context is not a big deal. We could further enforce the conversion with an abstract type vs. Js.Json.t, but I’m not sure that’s worth explaining to new users either. So I’m abandoning the goal, at least for now.

[jeddeloh]

One thing I rather like about this solution is we can actually allow for some partial data scenarios (ones that actually conform to the type).

[jeddeloh]

I still need to do a bit of a sanity check on all the changes here, but this PR should now be complete. In addition to the initial goals, this fixes a few very stupid bugs, and does another pass at serializing incoming variables args.

Overall, I'm much happier with the state of things now that before. For the most part, I was able to completely hide the fact that we're parsing to a result now, but there are a few cases where it's necessary to expose this fact because there is no possibility to fail with an error. onComplete for example just fires off with a 'jsData => unit so there's nothing you can do. I don't think it's a big deal. Most stuff isn't used that commonly.

[jfrolich]

Maybe we can ship with a ApolloClient_Promise module that can be shadowed by the user (by creating a ApolloClient_Promise.re) file. With default being Js.Promise.t but if you want to use Promise.t just create:

file ApolloClient_Promise.re:
include Promise;

So we have a non-opinionated default, but easy to use another promise library. Promises are getting better in ReScript as well, so I am a bit hesitant to ship with a reason-promise dependency, also in our codebase we use vanilla promises, but they would be a lot better with t-first!

[jeddeloh]

So the trick with shadowing that was used for ApolloClient I think only works if the module that gets shadowed isn't used (or maybe just at the bsconfig level). In this case the compiler goes through and compiles this library with one definition, but then later encounters the user defined one and thinks the build is stale. So I guess this hack isn't possible.

If we consider just promises, maybe functors are workable? Let me try again.

When you say "I am a bit hesitant to ship with a reason-promise dependency" are you referring to your own app or reason-apollo-client. If your own app, you wouldn't need to introduce it. We can provide cast functions like ApolloClient.toJsPromise or something.

[jeddeloh]

On thing I think is superior about reason-promise is the fact that you can convert a promise to something that can't fail. We've already done this work internally for this (and would be too much to ask users to do) and it's much better ergonomics & safety as a general pattern. Even if rescript improves their promises, it will still technically be a promise that can fail. For this reason, whatever conversion/customization is available, I'd like whatever comes out of ApolloClient to be PseudoPromise('a, never) vs PseudoPromise('a, Js.Exn.t). You can convert back to the looser thing if that's your choice.

[jeddeloh]

I'm going to start merging these outstanding PRs into a next release branch, move to moving all the method functions back on the record, and then revisit promises (possibly ripping the reason-promise stuff out entirely) before release.

[jfrolich]

Great! My initial reaction is that it's better to not have a promise dependency if it's avoidable, but if the upsides outweigh the downsides I'm game! :)