fable-compiler / Fable

F# to JavaScript, TypeScript, Python, Rust and Dart Compiler
http://fable.io/
MIT License
2.93k stars 301 forks source link

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

Open MangelMaxime opened 3 years ago

MangelMaxime commented 3 years ago

Description

This issue a is a follow up from this discussion: https://github.com/fable-compiler/ts2fable/issues/351#issuecomment-884159575

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

alfonsogarciacaro commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 https://github.com/fable-compiler/Fable/issues/2437#issuecomment-833184372

MangelMaxime commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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:

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

MangelMaxime commented 3 years ago

.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 commented 9 months ago

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