aantron / promise

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

Clarify "JS promises" vs. "Js.Promise" in the docs #33

Closed GrantJamesPowell closed 4 years ago

GrantJamesPowell commented 6 years ago

I'm confused by the top of the docs? Shouldn't Repromise work with js promises?

/* Aren't all js promises automatically repromises?
Interop — each Repromise is a JS promise, so JS libraries that return promises already return Repromises, and existing JS tooling should work. */

Js.Promise.resolve(1)
|> Repromise.map(id)

/* [merlin]
Error: This expression has type
         Repromise.promise('a) => Repromise.promise('a)
       but an expression was expected of type Js.Promise.t(int) => 'b
       Type Repromise.promise('a) = Repromise.rejectable('a, Repromise.never)
       is not compatible with type Js.Promise.t(int) = Js_promise.t(int) */

Fetch.fetch("https://www.reddit.com/r/programming.json")
|> Repromise.map(id)

/* [merlin]
Error: This expression has type
         Repromise.promise('a) => Repromise.promise('a)
       but an expression was expected of type
         Js.Promise.t(Fetch.response) => 'b
       Type Repromise.promise('a) = Repromise.rejectable('a, Repromise.never)
       is not compatible with type
         Js.Promise.t(Fetch.response) = Js_promise.t(Fetch.response) */

Awesome library by the way! Thank you.

aantron commented 6 years ago

IIRC you need something like:

Js.Promise.resolve(1)
|> Repromise.fromJsPromise
|> Repromise.map(id)

See the docs on converting between Js.Promise and Repromise: https://aantron.github.io/repromise/docs/Interop#converting-between-repromise-and-jspromise. Looks like they need a patch or two.

Explanation: Repromise does work with JS promises, meaning the Promise library in JS. Js.Promise is something else, namely a module in Reason/OCaml, that is implemented as JS promises (the ones in JS).

The statements about interop are saying that Repromise is also implemented as JS promises. However, on the Reason/OCaml side, Js.Promise and Repromise are not compatible at the type level, even though they have almost the same implementation in JS. And it has to be that way, otherwise Repromise wouldn't be type-safe (Js.Promise is not).

That's why there are those two conversion functions, Repromise.fromJsPromise and Repromise.toJsPromise. The "JsPromise" they refer to is the Js.Promise module. No conversion is required between Repromise and the underlying JS promises.

This is pretty confusing, largely because of the mental overlap between "JS promises" and "Js.Promise." I'll give some thought to whether that can be clarified in the docs somehow, but it seems options are quite limited.

aantron commented 6 years ago

Also note the offending docs were talking about JS libraries and JS promises, not BuckleScript bindings that use Js.Promise (those require the one extra function call). I'm not sure how to make that clearer, short of adding a footnote :p

GrantJamesPowell commented 6 years ago

Thanks for the write up! That was helpful. I appreciate you taking the time to write it out for me.

This is a really cool library that is helping me out a ton.

Let me know if you need any help with writing PRs!

aantron commented 6 years ago

Great :)

I want to leave this issue open for a while, as a reminder to think about how to clarify the docs.

arecvlohe commented 5 years ago

So just to clarify, if I wanted to use this with something like bs-fetch I would do:

Js.Promise.(Fetch.fetch(...)) |> Repromise.fromJsPromise(...)

Thanks in advance!

aantron commented 5 years ago

That's right, though I made a mistake above. It's Repromise.Rejectable.fromJsPromise.

The resulting Repromise is actually a Repromise.Rejectable.t(Fetch.response, Js.Promise.error), and you then have to deal with this "opaque" error value.

The error type is this one: https://bucklescript.github.io/bucklescript/api/Js.Promise.html#TYPEerror. You would typically get rid of that using Repromise.Rejectable.catch, resulting in a "clean" Repromise.

So, something like

fetch(...)
|> Repromise.Rejectable.fromJsPromise
|> Repromise.Rejectable.catch(error => handle it)

/* Now you have a Repromise.t(Fetch.response) */
aantron commented 4 years ago

Repromise reached 1.0.0 and became reason-promise. Many of the functions have been renamed. The last code block would now be:

fetch(...)
->Promise.Js.fromBsPromise
->Promise.Js.catch(error => handle it)

There is also a new helper Promise.Js.toResult for quickly converting to a nested Result:

fetch(...)
->Promise.Js.fromBsPromise
->Promise.Js.toResult

I also considered adding Promise.Js.log which would call a callback on rejection, and not expect that callback to return a new promise, like Promise.Js.catch does:

Promise.Js.log: (rejectable('a, 'e), 'e => unit) => promise('a);

However, the resulting promise would be forever-pending in case a rejection does occur, and that seems like the sort of thing that could bite many users, so I hesitated to do it. I'd still add it based on feedback, probably with a comment about why it can be dangerous.

The new docs for this are in the Rejection section of the README, and there are additional docs in Bindings that could be relevant, especially the second example.

I removed the old docs site for now, so closing this issue.