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

Experimenting with error management #2

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jguyon
Copy link

@jguyon jguyon commented May 24, 2018

This PR allows to reject a future by calling Future.error or by raising in a callback.

After our small exchange yesterday on discord, I got interested and started experimenting with handling exceptions, and this is the result :)

One thing to consider before merging this is that catching exceptions doesn't seem to be sound in bucklescript, at least to my understanding, because js can throw whatever, not just Error objects.
I don't know if this is a showstopper for you, but I thought a PR could at least serve as a reference
if it is.

Otherwise, I was not sure what to name the equivalent of catch because naming it flatMapError was out of the question, so I named it... catch, but you might have a better idea?

@gilbert
Copy link
Member

gilbert commented May 24, 2018

Hi @jguyon, thank you for your interest in contributing to the project, I really appreciate it.

Exception handling is indeed a missing feature. However, adding reject to Future kind of makes it a promise, which is something I'd like to avoid if possible, mostly to avoid complicating the implementation and type signature. But there needs to be some solution for JavaScript exceptions, so I'm glad you're bringing the issue to light.

You or someone else might have a better idea, but I was thinking of an API to catch the error as soon as possible. Something like...

// Instead of...
Future.make(resolve => dangerousJs() |. resolve)

// You use...
FutureJs.make(
  resolve => dangerousJs() |. resolve,
  err => ...
)

where err is perhaps a Js.Exn.t. But like you said, JavaScript can throw anything... and a library might also throw different types of errors (SyntaxError, MyCustomError, etc.). But the main idea & feature is to force the err => ... function to return the same type as the resolve => function, unifying things from the very start.

I named this particular example FutureJs because it would only exist to catch JS-specific behavior. Normal Reason/OCaml exceptions already have a handy syntax that I think is sufficient for this use case:

Future.make( resolve => switch(dangerous()) {
  | result => resolve(result)
  | exception Not_found => resolve(...)
})

@jguyon
Copy link
Author

jguyon commented May 28, 2018

Thank you for the detailed answer!

After reading it I'm realizing that I was thinking too much in terms of js promises.
The use cases I had in mind would indeed be better solved by explictly handling exceptions on each callback like in your last example.

Otherwise, I like your idea for handling exceptions from js, I think it's simple and clear.

Feel free to close this PR if you think discussions around this would be better placed in issues, I don't think much of the changes I made could be reused to implement what you've detailed.

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

Successfully merging this pull request may close these issues.

2 participants