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

How to write bindings for errors in Fable 3? #2492

Open
MangelMaxime opened this issue Jul 21, 2021 · 11 comments
Open

How to write bindings for errors in Fable 3? #2492

MangelMaxime opened this issue Jul 21, 2021 · 11 comments

Comments

@MangelMaxime
Copy link
Member

Description

This issue a is a follow up from this discussion: fable-compiler/ts2fable#351 (comment)

I am encountering a problem while working with bindings that contains Errors in JavaScript. In Fable 3 (or was it Fable 2?), a decision has been made to use System.Exception to represents JavaScript errors but this seems to cause a problem when working on bindings.

It seems like we can't inherit from System.Exception without adding AsbtractClass but adding this attributes makes Fable generates an invalid code in the case of the binding.

@alfonsogarciacaro What is the correct solution for writing bindings for a JavaScript error ?

REPL

Related information

  • Fable version: 3.2.9
  • Operating system: Windows / REPL
@alfonsogarciacaro
Copy link
Member

This is a good question... I don't really know 😅

My first thought is we could include an Error interface in Fable.Core so interfaces in bindings can inherit from it:

type Error =
    [<Emit("$0.message")>] abstract Message: string
    [<Emit("$0.stack")>] abstract StackTrace: string

However, this won't work if you want to capture the exception in a try catch. In that case, we do need to inherit from System.Exception using the AbstractClass attribute. We can also use the Erase attribute to prevent Fable from outputting any code:

type [<Erase; AllowNullLiteral; AbstractClass>] DatabaseError =
    inherit System.Exception
    abstract code: string option

let test() =
    try
        "foo"
    with
        | :? DatabaseError as e -> e.Message + (defaultArg e.code "0")
        | e -> e.Message

Now this compiles. The only issue is Fable won't do type testing with erased types, looking at the generated code we realize the with always goes through the first branch, which may be or may be not what the user intends.

export function test() {
    let e, e_1;
    try {
        return "foo";
    }
    catch (matchValue) {
        return (e = matchValue, e.message + defaultArg(e["Test.DatabaseError.get_code"](), "0"));
    }
}

To have actual type testing, instead of Erase we need to use the Import attribute properly:

type [<ImportMember("my_library"); AllowNullLiteral; AbstractClass>] DatabaseError =
    inherit System.Exception
    abstract code: string option

let test() =
    try
        "foo"
    with
        | :? DatabaseError as e -> e.Message + (defaultArg e.code "0")
        | e -> e.Message
// js
export function test() {
    let e, e_1;
    try {
        return "foo";
    }
    catch (matchValue) {
        return (matchValue instanceof DatabaseError) ? ((e = matchValue, e.message + defaultArg(e.code, "0"))) : ((e_1 = matchValue, e_1.message));
    }
}

@MangelMaxime
Copy link
Member Author

Hum, I didn't know of the last option.

It seems to tick all the features I needed. I will use it today and close this issues if everything seems to be working as expected.

Also, thank you for showing that | :? DatabaseError generates matchValue instanceof DatabaseError I add a ugly helpers in my code base.

let isDatabaseError (error : obj) : bool =
    emitJsExpr (error, PgProtocol.DatabaseError) """$0 instanceof $1"""

No I am not at all abusing emitJsExpr and emitJsStatement 😇

@alfonsogarciacaro
Copy link
Member

Well, type testing only works here because we use a concrete (imported) type, so your solution is indeed necessary when using interfaces as it's often the case in bindings. There was a proposal to add a special attribute for type testing of interfaces in bindings. I closed it because if lack of feedback but we could revisit it #2437 (comment)

@MangelMaxime
Copy link
Member Author

I think adding the special attributes could be a nice addition to Fable.

That's true that in Elmish/React application we don't seems to often need this kind of type testing. However, when writing back-end code and when you want to handle exception error coming from libraries it comes handy.

And it would improve the interop feature of Fable. 😉

It would also, makes case like the one I encountered with DatabaseError more straight forward because I suppose in that case we could use the attributes too instead of saying. It works only because we added ImportMember and XX conditions etc.

@inosik
Copy link
Contributor

inosik commented Jul 29, 2021

Seems like @alfonsogarciacaro's comment reiterates what we talked about in #2353.

IMHO, this approach makes it much easier to understand what the F# bindings actually represent on the JS side. For example, no more values of an XYStatic type, for what actually is a type in JS. And in consequence, no need for isInstanceOf helpers, but just a plain type check in F#.

@MangelMaxime
Copy link
Member Author

MangelMaxime commented Jul 29, 2021

@inosik Thank you for the head I forgot about that discussion.

I will go back to it when I have the time again for it.

I started a discussion on Glutinum project so I can keep track of the "most interesting" discussions that happened around this subject.

@inosik
Copy link
Contributor

inosik commented Jul 29, 2021

A helper like this one could make sense, though. In JS, errors don't necessarily inherit from Error, and can be of any type.

let (|Error|_|) (exn : exn) =
    match box exn with
    | :? 'a as err-> Some err
    | _ -> None

// Usage:
let f () =
  try
    JSLib.somethingThatMightThrowAString ()
  with
  | Error (err : string) ->
    printfn "%s" err

@inosik
Copy link
Contributor

inosik commented Jul 29, 2021

This works better:

open System
open Fable.Core

let (|Error|_|) (exn : exn) =
    Some (box exn)

// Usage:
let f () =
  try
    JSLib.somethingThatMightThrowAString ()
  with
  | Error (:? string as err) ->
    printfn "%s" err

@alfonsogarciacaro
Copy link
Member

Ups, I forgot about that discussion too. In fact, I think I originally missed this comment so I've posted a reply to it 😅 BTW, I didn't know you could use active patterns in try .. with either. I agree it's a great solution for the cases where you cannot inherit from Error/exn.

Following the discussion of how we can improve the bindings, I think I should restrain myself from adding more hacks like EmitTypeTest and we should try to improve things in the F# level by adding a language suggestion. I'm thinking of two possibilities:

  • Allow "empty" .fsi signature files (empty in the sense they don't need a corresponding .fs file), maybe through a special attribute or pragma, that can appear in the AST but whose calls will be resolved by transpilers.

  • Make extern more flexible. I just learned this keyword is already working in F# but in a very limited way, we could ask to use it the same as in C# so we can write empty class declarations.

Do you agree with any of them or have any other idea to make a language suggestion.

@MangelMaxime
Copy link
Member Author

.fsi files are something that not a lot of people know about/use in F# so I think the extern approach has the benefit of not introducing a too "complex"/foreign feature for the user.

@goswinr
Copy link
Contributor

goswinr commented Mar 1, 2024

This could be bypassed by creating a separate module that has classes of the same name as the interface. see:
glutinum-org/cli#16

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

No branches or pull requests

4 participants