RationalJS / future

A Js.Promise alternative for ReasonML
213 stars 15 forks source link

FutureJs.resultToPromise requires Result to contain exception #33

Closed briangorman closed 5 years ago

briangorman commented 5 years ago

Example: Try appending this to TestFutureJs.re

testAsync("resultToPromise (Error result)", done_ => {
+    let err = `TestError;
+    delay(5, () => Belt.Result.Error(err))
+    |> FutureJs.resultToPromise
+    |> Js.Promise.then_(_ => raise(TestError("shouldn't be possible")))
+    |> Js.Promise.catch(checkPromisedValue(done_, err));
+  });

Code does not compile because Js.Promise.make requires a reject function with an exception as its parameter. This is problematic because we use Polymorphic variants for error handling, not exceptions. Right now to use this code with polymorphic variants I have to convert our Polymorphic variant errors to an exception before calling resultToPromise.

Possible solution: in resultToPromise add a Future.mapError() step to convert the contents of the Belt.Result to an exception. This is a "lossy" step, but this function is only useful for javascript interop, so that information would be lost anyway. Let me know if you would be open to a PR.

briangorman commented 5 years ago

@gilbert any thoughts here?

gilbert commented 5 years ago

Sorry for the delay, I'm not working with reasonml at the moment. Would your proposed solution be backwards compatible? If so then definitely go for it.

briangorman commented 5 years ago

@gilbert - No problem. Yes, the proposed change is backwards compatible. I opened #34 to address this. Let me know what you think! Thanks again!

briangorman commented 5 years ago

Hey @gilbert, any chance you make a release with this feature sometime in the near future? Thanks in advance for the update!