Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Converting from Js.Promise #64

Closed
idkjs opened this issue Oct 10, 2020 · 3 comments
Closed

Converting from Js.Promise #64

idkjs opened this issue Oct 10, 2020 · 3 comments

Comments

@idkjs
Copy link

idkjs commented Oct 10, 2020

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
Copy link

baransu commented Oct 11, 2020

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"
  1. 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
Copy link
Owner

aantron commented Oct 12, 2020

@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
Copy link
Author

idkjs commented Oct 12, 2020

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

@idkjs idkjs closed this as completed Oct 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants