aantron / promise

Light and type-safe binding to JS promises
MIT License
340 stars 24 forks source link

Converting from Js.Promise #64

Closed idkjs closed 4 years ago

idkjs commented 4 years ago

Greetings, @aantron.

Would it be possible on providing some guidance on converting from Js.Promise to reason-promise? It seems like in many cases, that is what someone might be trying to do.

The library in question is reason-apollo-client. The branch has a bunch of other improvements, includes reason-promise as the default promise implementation. That branch is the next branch.

By way of example, found at reason-promise-question, I have a Js.Promise function that gets me the current token value. It has the following type:

unit => Js.Promise.t(
  Js.Nullable.t(string)
)

The function can be found in Tokens and is reproduced below. This is a dummy function, there is no binding. The point is to see how to get it to compile.

[@bs.val] [@bs.scope "localStorage"]
external setItem: (string, string) => unit = "setItem";
let setUserToken: string => Js.Promise.t(unit) =
  token => Js.Promise.resolve(setItem("auth", token));

[@bs.val] [@bs.scope "localStorage"]
external getItem: string => Js.Nullable.t(string) = "getItem";
let getUserToken: unit => Js.Promise.t(Js.Nullable.t(string)) =
  () => Js.Promise.resolve(getItem("auth"));

let setTempUserToken: string => Js.Promise.t(unit) =
  _ => Js.Promise.resolve();

let getTempUserToken: unit => Js.Promise.t(Js.Nullable.t(string)) =
  () => Js.Promise.resolve(Js.Nullable.undefined);

When I try use this with reason-promise when creating an apollo/client authlink, I get the following error:

unit => Js.Promise.t(
  Js.Nullable.t(string)
)
Error: This expression has type
         Js.Promise.t(Js.Json.t) = Js.Promise.t(Js.Json.t)
       but an expression was expected of type
         Promise.t(Js.Json.t) = Promise.rejectable(Js.Json.t, Promise.never)

Here is the authlink function:

let authLink =
  ApolloClient.Link.ContextLink.makeAsync((~operation as _, ~prevContext as ctx) => {
    Tokens.getUserToken()
    ->Js.Promise.then_(
        token => {
          switch (token->Js.Nullable.toOption) {
          | None =>
            Tokens.getTempUserToken()
            ->Js.Promise.then_(
                token => Js.Promise.resolve(Js.Nullable.toOption(token)),
                _,
              )
          | Some(token) => Js.Promise.resolve(Some(token))
          }
        },
        _,
      )
    ->Js.Promise.then_(
        fun
        | None => Js.Promise.resolve(Js.Json.null)
        | Some(token) => {
            Js.Promise.resolve(
              [%raw
                {| (context, token) => ({
                headers: {
                  ...ctx.headers,
                  authorization: `Bearer ${token}`
                }
              }) |}
              ](
                ctx,
                token,
              ),
            );
          },
        _,
      )
  });

How do we convert this to a reason-promise? Please feel free to be as diadactic as you want to be.

Thank you, in advance.

I have reposted on stackoverflow if you prefer to answer there. The link is https://stackoverflow.com/questions/64294223/converting-from-js-promise-to-reason-promise-in-reasonml

baransu commented 4 years ago

My rule of thumb is:

  1. when you know your promise won't fail (and the promise is just to express async) just use Promise.t('a) when binding.

    [@bs.module "my-awesome-lib"]
    external getStringAsync: unit => Promise.t(string) = "getStringAsync"
  2. when I have no idea about the internals of the function I'm binding to I would use Promise.Js.t('a, Js.Promise.error) and convert to Result.t as soon as as possible by wrapping Js.Promise.error into poly variant for better error composability. I believe this can be achieved with catch as well and I'm not 100% if its correct.

[@bs.module "my-awesome-lib"]
external _getStringAsync: unit => Promise.Js.t(string, Js.Promise.t) = "getStringAsync"
let getStringAsync = () => {
  _getStringAsync()
  ->Promise.Js.toResult()
  ->Promise.mapError(error => `PromiseException(error))
}

I also think in your case there is no need for Token module to be async as the JS API is sync.

I would implement authLink like this:

// Note: Refactored in GH comment, may not compile :P
let authLink =
  ApolloClient.Link.ContextLink.makeAsync((~operation as _, ~prevContext as ctx) => {
    let token = Tokens.getUserToken();
    switch(token) {
      | None => Js.Obj.empty()
      | Some(token) => 
          let headers = Js.Obj.assign({
            "authorization": "Bearer " ++ token
          }, ctx.headers)
          {"headers": headers}
    }
  });

This code may not express your logic in 100% but it's a simplified version. In my experience wrapping sync API into promises makes things worse 😉

aantron commented 4 years ago

@idkjs I believe @baransu has answered your question. In summary, your binding (ideally) wouldn't be written in terms of Js.Promise, but in terms of reason-promise to begin with. Js.Promise from BuckleScript and reason-promise are just two incompatible types (deliberately, as the first one is not type-safe), so you can't use one where the other is expected, which is what your code appears to have been trying to do.

There are, however, conversions, in the common case that you do have two sets of code using the different types, and the README provides instructions on writing bindings for those cases where you can edit the code to replace Js.Promise by reason-promise already at the binding level, thus avoiding introducing any Js.Promises at all. @baransu has pretty much perfectly explained how to use the binding support in his comment (thanks!).

idkjs commented 4 years ago

Thank you, gentelman. Very instructive. Will revert with results.