Skip to content
This repository has been archived by the owner on Oct 29, 2024. It is now read-only.

js_exn: add brief explanations and links to MDN. #206

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jdeisenberg
Copy link
Contributor

Please check the documentation for asJsExn; I am not sure I read the code for this correctly (nor am i sure how to use it).

@jdeisenberg jdeisenberg changed the title Add brief explanations and links to MDN. js_exn: add brief explanations and links to MDN. May 25, 2020
@@ -26,35 +32,44 @@ Provide utilities for dealing with JS exceptions.
let asJsExn: exn => option(t);
```

Returns `Some(exn)` as a JavaScript exception if its argument is an OCaml `Error`, `None` otherwise. *Someone needs to check this; I am not really sure this is an accurate description.*
Copy link
Member

@ryyppy ryyppy May 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Returns `Some(exn)` as a JavaScript exception if its argument is an OCaml `Error`, `None` otherwise. *Someone needs to check this; I am not really sure this is an accurate description.*
Returns `Some(jsExn)` if the input argument `exn` is an actual JS exception, and returns `None` if `exn` is a native Reason exception.
```re example
let throwJsException: unit => 'a = [%raw {|function() { throw "hello"; }|}];
// This will log out "hello", since a JS exception was thrown
try (throwJsException()) {
| myExn => switch(myExn->Js.Exn.asJsExn) {
| Some(jsExn) => jsExn->Js.log
| None => Js.log("this is a Reason exception")
}
};
exception MyError;
// This will log out "this is a Reason exception", since our raised error is a native Reason exception
try (raise(MyError)) {
| myExn => switch(myExn->Js.Exn.asJsExn) {
| Some(jsExn) => jsExn->Js.log
| None => Js.log("this is a Reason exception")
}
};
```

(Thanks to @cristianoc for explaining this feature)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after further discussion, this function seems rather redundant, since the same functionality can be achieved via pattern matching:

try(throwJsException()) {
  | Js.Exn.Error(_) => Js.log("Js exception");
  | _ => Js.log("Some Reason exception")
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about combining these? Change the explanation to:

Returns Some(jsExn) if the input argument exn is an actual JS exception, and returns None if exn is a native Reason exception.

and then add:

Rather than using this function, you can get the same functionality with pattern matching:

with your example.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that sounds good!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants