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

Add ParamObject attribute #2555

Merged
merged 2 commits into from
Oct 1, 2021
Merged

Add ParamObject attribute #2555

merged 2 commits into from
Oct 1, 2021

Conversation

alfonsogarciacaro
Copy link
Member

Very often we have to write bindings for JS methods accepting an object argument and the current solution (instantiate options with jsOptions or cast an anonymous record with !!) are not nice.

This allows you to add [<ParamObject>] attribute to the method with an optional index argument to say where the object arguments start. Optional arguments set to None are omitted. For example, this F# code:

[<Global>]
type MyJsClass [<ParamObject>] (?foo: int, ?bar: string, ?baz: float) =
    [<ParamObject(2)>]
    member _.Foo(foo: int, bar: string, ?baz: float) = jsNative

let test() =
    let c = MyJsClass(4, baz=56.)
    c.Foo(5, baz=4., bar="b")

Becomes:

function test() {
    const c = new MyJsClass({
        foo: 4,
        baz: 56,
    });
    return c.Foo(5, "b", {
        baz: 4,
    });
}

@MangelMaxime
Copy link
Member

Hello @alfonsogarciacaro,

I think the proposition is interesting, here are my thought on it.

The <ParamObject>] on the constructor looks goods as I think C#/F# already have a similar functionality and it is pretty straightforward to understand what's going on.

You can even write it like that:

let c = 
    MyJsClass(
        foo = 4, 
        baz= 56.
    )

Which looks similar to

let c = 
	{
		foo : 4,
		baz : 56.
	}

We just have the MyJsClass as a prefix.

ℹ️ We can't yet write this code in F# because there is a warning about the indentation, but I think the next version of F# will relax a lot of the indentation rules.

let c = MyJsClass(
	foo = 4, 
    baz= 56.
)

However, I don't like the [<ParamObject(2)>] because it is not a 1 to 1 mapping. What if the second object needs more than one property?

I think it is better to have a single way to do things which works all the time than one way which works all the time and another one which works only for a very specific case.

So using [<ParamObject>] only, this would give this code which is I think almost 1 to 1 with the JavaScript if we exclude the "required class prefix".

[<ParamObject>]
type Baz (?baz : float) =
    class end

[<Global>]
type MyJsClass [<ParamObject>] (?foo: int, ?bar: string, ?baz: float) =
    member _.Foo(foo: int, bar: string, ?baz: Baz) = jsNative  

let c = 
    MyJsClass(
        foo = 4, 
        bar = "b"
    )

c.Foo(
    5, 
    "b", 
    Baz(
        baz = 4.
    )
)

Also, is the [<Global>] required?

@MangelMaxime
Copy link
Member

MangelMaxime commented Sep 30, 2021

Another thing, that I forget to add is that I think in general this kind of objects are suffixed with Options or Parameters which will make it even more explicit.

Here is an example adapted from a "real npm package":

let databaseOptions =
    PgSubset.Pg.IConnectionParameters<_>>(
        max = 15., // The maximum number of connections to keep open in the pool
        idleTimeoutMillis = 10000., // The maximum amount of time a connection can sit idle in the pool before being removed
        host = Env.POSTGRES_HOST.Value,
        port = (float Env.POSTGRES_PORT.Value),
        database = Env.POSTGRES_DATABASE.Value,
        user = Env.POSTGRES_USER.Value,
        password = !^ Env.POSTGRES_PASSWORD.Value
    )

let db = pgp.Invoke(databaseOptions)

versus

let databaseOptions =
    jsOptions<PgSubset.Pg.IConnectionParameters<_>>(fun o ->
        o.max <- Some 15. // The maximum number of connections to keep open in the pool
        o.idleTimeoutMillis <- Some 10000. // The maximum amount of time a connection can sit idle in the pool before being removed
        o.host <- Some Env.POSTGRES_HOST.Value
        o.port <- Some (float Env.POSTGRES_PORT.Value)
        o.database <- Some Env.POSTGRES_DATABASE.Value
        o.user <- Some Env.POSTGRES_USER.Value
        o.password <- Some !^ Env.POSTGRES_PASSWORD.Value
    )

let db = pgp.Invoke(databaseOptions)

The main benefit that we can see directly is that we don't have the Some required on each line

@alfonsogarciacaro
Copy link
Member Author

alfonsogarciacaro commented Sep 30, 2021

@MangelMaxime Sorry, my explanation was not complete. The index indicate when the object arguments start. So if you have:

[<Global>]
type MyJsClass() =
    [<ParamObject(2)>]
    member _.Foo(foo: int, bar: string, baz: float, ?lol: int) = jsNative

let test() =
    let c = MyJsClass()
    c.Foo(5, baz=4., bar="b", lol=3)

Becomes:

function test() {
    const c = new MyJsClass();
    return c.Foo(5, "b", {
        baz: 4,
        lol: 3,
    });
}

That is, from argument with index 2, all the arguments are transformed into an object.

Also, is the [] required?

At the moment this only modifies the call not the receiving arguments. So yes, it should be used only with Global or Imported classes. We should allow it for interfaces too because interfaces are used a lot to represent JS types, but it would be problematic if users implement them in F#.

@alfonsogarciacaro
Copy link
Member Author

Also, note that atm this affects only methods/constructors, not classes. So it's not exactly the same as jsOptions. Note in the example in the first comment, we call new MyJsClass, we're not just creating a POJO. I did think to make ParamObject applicable to classes as well, but this added complication, and having to manually type all the members in case you want to access them is quite verbose.

// This is not possible in the current proposal, but
// imagine we could apply ParamObject to a class.
// We would have to define them like this and the compiler
// should deal with them as if they were an interface.

[<ParamObject>]
type MyJsOptions (?foo: int, ?bar: string, ?baz: float) =
    member _.foo = foo
    member _.bar = bar
    member _.baz = baz

@MangelMaxime
Copy link
Member

Sorry, my explanation was not complete. The index indicate when the object arguments start. So if you have:

I like it even less 🤣

Because, what if the JavaScript method ask for something like that:

myFunc(1, 2, { propA: "something" }, "b", { propB: "test", propC: "something"})

The [<ParamObject>] is really confusing because it generates something totally different than what you are reading from the F# code.

Also, note that atm this affects only methods/constructors, not classes

This is was I understand indeed, sorry I forgot the new in my snippets indeed. Personally, I think it should only apply to Object constructor as I think we should only have [<ParamObject>] added.

@Zaid-Ajaj
Copy link
Member

@alfonsogarciacaro I really like both proposals. @MangelMaxime there are many functions that an initial number of required parameters, then an options object at the end which [<ParamObject(n)>] would take care of even if the binding is not 1-to-1, at least it makes it relatively easy to do.

As for

myFunc(1, 2, { propA: "something" }, "b", { propB: "test", propC: "something"})

Even though [<ParamObject(n)>] won't be able to handle it, it is an edge case that rarely arises (afaik) as opposed to the options object that many JS APIs use

@Happypig375
Copy link
Contributor

Happypig375 commented Sep 30, 2021

@MangelMaxime Yes,

let c = MyJsClass(
    foo = 4, 
    baz = 56.
)

will work in F# 6.

@MangelMaxime
Copy link
Member

I know that I gave an edge case.

One of the greatest thing in Fable is that it has good interop story with JavaScript not because of magic stuff happening but because it is explicit about what's going on and what is the generated code from it.

I see people being confused all the time about magic stuff in Fable and having a hard time understanding when to use solution X versus solution Y.

We can also see it happening in the C# language decision where people don't know any more which feature to use to do what because there are too many ways of doing things.

Please let's keep the F# philosophy and keep things simple/straightforward :)


Also, note that atm this affects only methods/constructors, not classes. So it's not exactly the same as jsOptions. Note in the example in the first comment, we call new MyJsClass, we're not just creating a POJO. I did think to make ParamObject applicable to classes as well, but this added complication, and having to manually type all the members in case you want to access them is quite verbose.

// This is not possible in the current proposal, but
// imagine we could apply ParamObject to a class.
// We would have to define them like this and the compiler
// should deal with them as if they were an interface.

[<ParamObject>]
type MyJsOptions (?foo: int, ?bar: string, ?baz: float) =
    member _.foo = foo
    member _.bar = bar
    member _.baz = baz

According to my experience, this is not often that we need to access an object options/parameters properties but it can happen. So it would be nice to have it supported if possible.

It is indeed more verbose but we can have binding generation making the process easier.

@MangelMaxime
Copy link
Member

I just thought that I was focusing a lot on F# to JavaScript side but the inverse is true.

One benefit, of having a closer mapping to JavaScript is that when consuming or adapting a JavaScript library to F# the JavaScript documentation is still relevant. The farther we go from JavaScript the less relevant is the JavaScript documentation and so people need to write more documentation on how to consume the bindings.

@alfonsogarciacaro
Copy link
Member Author

I think we can merge the PR as is now and we can decide whether to allow ParamObject on classes later. The issue with having it only for classes is it makes more verbose both to write the bindings and call them because you have to instantiate the options object. Example:

// Binding
[<ParamObject>]
type SaveSnapshotOptions(name: string, content: string) =
    class end

type WebTestRunner =
    static member saveSnapshot(options: SaveSnapshotOptions): JS.Promise<unit> = jsNative

// Usage
WebTestRunner.saveSnapshot(SaveSnapshotOptions("foo", "bar"))

Versus:

// Binding
type WebTestRunner =
    [<ParamObject>]
    static member saveSnapshot(name: string, content: string): JS.Promise<unit> = jsNative

// Usage
WebTestRunner.saveSnapshot("foo", "bar")

Binding is not 1:1

In most cases, JS APIs mix positional and "object" arguments because of two reasons:

  • The positional arguments are mandatory.
  • The options argument was added later to the API.

In both cases I think it should be easy for users to mentally map the options with the optional arguments in the original JS api thanks to the argument names. Let's consider the Event constructor API new Event(typeArg[, eventInit]):

  • typeArg
  • eventInit:
    • bubbles
    • cancelable
    • composed

In F# this can become (suposing Event has a real constructor instead of Create method):

type Event
    [<ParamObject(fromIndex=1)>]
    new (typeArg: string, ?bubbles: bool, ?cancelable: bool, ?composed: bool) =

Fable becomes more complicated

This is a rightful concern. To avoid adding too much magic to Fable, we try to use standard F# solutions and avoid Fable hacks (e.g. removing Inject attribute). But bindings are a necessary evil, because F# doesn't really have mechanisms to do that so we need to explore ways that allow us to use JS code in a nice way from F#.

@alfonsogarciacaro alfonsogarciacaro merged commit 4362b08 into main Oct 1, 2021
@inosik
Copy link
Contributor

inosik commented Oct 6, 2021

Just to note that here, we can already leverage ParamObject and Emit to create proper types for POJOs:

[<Global>]
type Options [<ParamObject; Emit "$0">] (?a: int, ?b: int) =
    member val a: int = jsNative with get, set
    member val b: int = jsNative with get, set
let opts = Options (a = 1)

This snippet becomes:

export const opts = {
    a: 1
};

@alfonsogarciacaro
Copy link
Member Author

That's true, good one @inosik!

@MangelMaxime
Copy link
Member

I am playing with ParamObject attributes to create a binding and one thing that I discovered is that you can use constructor overload to offer an experience without U2, U3, etc. types.

    [<AllowNullLiteral>]
    [<Global>]
    type FuseOptionKeyObject [<ParamObject; Emit("$0")>]
        private (name: U2<string, ResizeArray<string>>, weight: float) =

        [<ParamObject; Emit("$0")>]
        new (name: string, weight: float) =
            FuseOptionKeyObject(U2.Case1 name, weight)

        [<ParamObject; Emit("$0")>]
        new (name: ResizeArray<string>, weight: float) =
            FuseOptionKeyObject(U2.Case2 name, weight)

        member val name: U2<string, ResizeArray<string>> = jsNative with get, set
        member val weight: float = jsNative with get, set

See how here, the main constructor is private and the public constructors use standard F# types :)

On the usage side it looks like that Fuse.FuseOptionKeyObject("title", 0.3).

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.

5 participants