Next Release

[jeddeloh]

There's a lot of changes coming down the pipe so I'm going to create this branch so we can view the changes as a whole before committing to a new release. I'm making no commitments to anything in this branch until it is finally merged.

Current Status

Improved promise results

It's very common to forget to check for graphql errors when you're presented with an Ok/Error result. For this reason we're now a little smarter and we unwrap the case where you definitely have data, and everything else into error:

client.query(~query=(module TodosQuery), ())
|> Js.Promise.then_(result =>
    switch (result) {
    | Ok({data: {todos}, error: None}) =>
      // You can't be Ok with data: None
    | Ok({data: {todos}, error: Some(error)}) =>
      // You _can_ have data and error, but only if you change error policy from default to "all"
    | Error(error) =>
      // Any other case is an error:
      // - 200 response with graphqlErrors and no data (always if errorPolicy is "none", the default)
      // - fetch error, etc.
      // - no data and no error (impossible?)
    }
  );

Parsing will no longer raise exceptions (mostly transparent change)

All parse functions now safely converted to result returning functions. For the vast majority of cases, Apollo returns a "result" of some type anyway so we unwrap the result and convert to an appropriate error representation. There are a few cases where this is not possible (like parsing previous values from the cache) so you have to deal with a result but I think in the end this is a good tradeoff.

Methods are now called directly from records or destructured

This library can now be used almost 1:1 like Js. You can now just destructure and use methods like fetchMore:

  let queryResult = TodosQuery.use();

  <div>
    {switch (queryResult) {
     | {loading, data, error, fetchMore} =>
...

[jeddeloh]

I'm going to propose this go straight from 0.0.1-beta.8 to 1.0.0-beta.0 as much as to signify that this release includes some very big breaking changes as to signify with this next release, the major concepts will feel "complete" to me (1.0 version says that a little better than 0.0, imo).

[jeddeloh]

I'm going to explore allowing full promise config with functors since it might be small, but if that doesn't work, I will probably back off and use Js.Promise.t. For reason promise, I think it's actually very important to work with result type so you need to do further conversion as it stands anyway.

[jfrolich]

Great stuff. Functor customization would be great. But doesn't reason-promise work well with a Js.Promise.t (if it doesn't raise)?

Also when using the result type for data results, isn't there a case that there is an error AND some data returned? I am not sure, have to look it up in the GraphQL spec, but it worth looking into to see if a variant is the right data structure here.

I also have concerns about backwards compatibility of ditching {error, data} return type for the hook, not sure if it's a clear improvement (the other one is closer to the JS version and there are no obvious upsides for the result type). For promises I think we should definitely adopt the result type.

[jeddeloh]

Great stuff. Functor customization would be great.

I haven't totally given up yet. :)

But doesn't reason-promise work well with a Js.Promise.t (if it doesn't raise)?

Yes, they're effectively the same thing, and it's totally safe to cast from Promise.t to Js.Promise.t, but from a blind types perspective, not the reverse. Here, we know it's safe, but no one else would without reading docs or code. My proposal at this point is that we default to reason-promise and provide a cast function from Promise.t to Js.Promise.t and also provide T-first functions for working with Js promises:

module Promise = ApolloClient__Promise;

// Chaining with our T-first Js promise helpers if you don't like reason-promise
client.query(~query=(module TodosQuery), ())
->Promise.toJs
->Promise.Js.then_(result => {
    switch (result) {
    | Ok({data: todos}) => Js.Promise.resolve()
    | Error(error) => Js.Promise.resolve()
    }
  })
->Promise.Js.finalize;

You could also use Promise.resultToJs if you would rather not be given a result.

Also when using the result type for data results, isn't there a case that there is an error AND some data returned? I am not sure, have to look it up in the GraphQL spec, but it worth looking into to see if a variant is the right data structure here.

Yes, data and error can both be present. Maybe it's better to illustrate with code?

client.query(~query=(module TodosQuery), ())
->Promise.get(result =>
    switch (result) {
    | Ok({data: {todos}, error: None}) =>
      // You can't be Ok with data: None
    | Ok({data: {todos}, error: Some(error)}) =>
      // You _can_ have data and error, but only if you change error policy from default to "all"
    | Error(error) =>
      // Any other case is an error:
      // - 200 response with graphqlErrors and no data (always if errorPolicy is "none", the default)
      // - fetch error, etc.
      // - no data and no error (impossible?)
    }
  );

I think without this change it's quite dangerous and easy to forget to check for errors in the Ok state. For this reason I've made one change (made data non-optional in the Ok case) and the rest is purely pushing things that should be an error in to the Error branch.

I also have concerns about backwards compatibility of ditching {error, data} return type for the hook, not sure if it's a clear improvement (the other one is closer to the JS version and there are no obvious upsides for the result type). For promises I think we should definitely adopt the result type.

Nothing has changed about the result type in hooks yet (promise returning methods on them have) and I don't have any plans to change them. I do want to reassess one more time if we learn anything from the promises situation, but I haven't been able to think about it yet.

[jfrolich]

Thanks. Sounds great!

One more thought. Perhaps we can have two packages that you can install to get either Promise.t or Js.Promise.t (depending on which package you installed). Like reason-apollo-js-promise, that creates a module ApolloPromise with just a type t as an alias to Js.Promise.t (and perhaps some function aliases as well)..

[jeddeloh]

Thanks. Sounds great!

One more thought. Perhaps we can have two packages that you can install to get either Promise.t or Js.Promise.t (depending on which package you installed). Like reason-apollo-js-promise, that creates a module ApolloPromise with just a type t as an alias to Js.Promise.t (and perhaps some function aliases as well)..

Interesting! I hadn't considered this option. So we'd have different npm package names, but in bsconfig they would have the same name like reason-apollo-client--promise and provide a module with a consistent name as well. It's clunky, but provides for full customization without functors. 🤔

In the case of the js-promise one, I would expect the promises to be "normal" like so:

client.query()
|> Js.Promise.then_(okResult => ...)
|> Js.Promise.catch(untypedPromiseError => ...)

because Js promises that return a result with no need to catch is very unorthodox use of Js promises :) It feels like a weird middle ground. In my opinion, either go all-in on Js.Promise ergonomics or use a library meant for working with promise+result combo. Thoughts?

[jfrolich]

I was thinking more along the lines of an extra package that people have to install

• reason-apollo-client-reason-promise
• reason-apollo-client-js-promise

If we have a consistent interface (module type) a third party can facilitate another promise library (such as prometo).

because Js promises that return a result with no need to catch is very unorthodox use of Js promises :) It feels like a weird middle ground. In my opinion, either go all-in on Js.Promise ergonomics or use a library meant for working with promise+result combo. Thoughts?

In Reason you generally don't throw in a promise, but use something like a result type. So even with vanilla ReScript promises this is the best practice AFAK. So I don't see any conflict there. Just reason-promise having a slightly arguably nicer API (I think the API surface is a bit to large and unconventional though).

[jfrolich]

In the case of the js-promise one, I would expect the promises to be "normal" like so:

client.query()
|> Js.Promise.then_(okResult => ...)
|> Js.Promise.catch(untypedPromiseError => ...)

Isn't this nicer? (this is fairly idiomatic, at leat in our codebase):

client.query()
|> Js.Promise.then_(fun
| Ok(...) => ...
| Error(...) => ...
)

[jeddeloh]

I was thinking more along the lines of an extra package that people have to install

• reason-apollo-client-reason-promise
• reason-apollo-client-js-promise

If we have a consistent interface (module type) a third party can facilitate another promise library (such as prometo).

Yes, you would need to install reason-apollo-client and reason-apollo-client--js-promise or something. I initially didn't think this could work due to the fact that reason-apollo-client would need to declare the module as a dependency. But if you require that the bsconfig module name is the same in both, and add one of them in devDependencies of reason-apollo-client, perhaps it will work (still haven't tried it).

[jeddeloh]

Isn't this nicer? (this is fairly idiomatic, at least in our codebase):

Oh, it's definitely nicer, but something about it also drives me crazy. The types say this can fail (it's just he nature of Js.Promise.t), but by convention you don't catch because you resolve with a result and trust the people upstream have done the work to ensure this. It's perhaps irrational, but it just doesn't sit well with me. Anyway, if this works, it doesn't really matter if I like it or not, we can provide any style of promises!

[jfrolich]

But rejecting is also not the nicest solution because the errors are not typed. I think maybe an argument that would convince you is a reasoning when it should throw or when it should resolve. You can argue that it should only throw in exceptional scenarios. That the API returns errors is not really exceptional, it's to be expected. Of course where you draw the line is subjective, but I usually like to only raise exceptions in scenarios that shouldn't really happen in practice.

[jeddeloh]

I like to be able to 100% trust the types regardless of how rare or unlikely the failure might be. As a general pattern across an app, I just don't totally trust this promise style enough to not catch. Because of that, I advocate that people that want promises of Belt.Result.t use reason-promise instead. The pure Js style promises have disadvantages that you noted (which is why I would not use them), but also advantages that they are familiar Js style and they feel safer to me (I can see that I'm catching errors and trust it 100%).

But I don't think any of this matters? Are you advocating that Js.Promise.t(Belt.Result.t('a, ApolloError.t)) be the default that we use in the examples and docs? Because if not, it doesn't matter what I think if we can provide both styles of Js promises 😄

// Example: It doesn't happen often, but someone eventually does something like this
let importantOperation = () =>
  Axios.post(...)
  |> Js.Promise.then_(response =>
      response.data.message === "message received"
      ? Ok(data)
      : Error("Oh no")
   );

// And I'm going to consume it like this
importantOperation()
|> Js.Promise.then(result =>   
  switch (result) {
  | Ok(data) => doSomething(data)
  | Error(error) => doImportantThingIf(error)
  }
)
// And not realize the function above will only error on 200 range responses (a "success" that is converted to Error)
// somehow always when it's very important we not be missing those errors!
// Are you catching in addition for things that are important?

[jeddeloh]

Status update:

I was wrong that the proposed "extra module" solution would work. I must not have changed the directory name when testing it out.

That being the case, the longer I sit on it, the more I didn't want to force people into a dependency on reason-promise. So I went with @jfrolich 's suggestion of returning a Js.Promise.t(result('a, ApolloError.t)). But as soon as I did this, it became clear that this is very inconvenient representation to convert to other promise styles (like reason-promise) because there are no built-in helpers for it. At this point I think the best option is to just stick with vanilla promises and do your own conversion if you don't like them.

I did take one more pass at restructuring things to work a little better with a config functor, but same result. The complexity was not a worthwhile tradeoff, imo.

It's still possible we could publish totally separate versions or something, but I'd rather not be holding this next release up for that reason.

[jeddeloh]

Well I had to go with Js.Promise.t(result('a, ApolloError.t)) anyway because I actually do some work with ApolloError and it is too annoying to convert between reason to Js multiple times. PR #57. So demoralized, but hopefully can finally release this soon.

[jeddeloh]

I plan on merging the outstanding two PRs in here tomorrow and releasing. Apologies for taking so long on this, I've been swamped :)