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

missing optional on interface function #135

Closed
joprice opened this issue Oct 24, 2024 · 6 comments
Closed

missing optional on interface function #135

joprice opened this issue Oct 24, 2024 · 6 comments

Comments

@joprice
Copy link

joprice commented Oct 24, 2024

Issue created from Glutinum Tool

Glutinum version - 0.11.0-preview

TypeScript

function alert(
    title: string,
    message?: string
  ): void;

interface AlertStatic {
  alert: (
    title: string,
    message?: string
  ) => void;
}

FSharp

module rec Glutinum

open Fable.Core
open Fable.Core.JsInterop
open System

[<AbstractClass>]
[<Erase>]
type Exports =
    [<Import("alert", "REPLACE_ME_WITH_MODULE_NAME")>]
    static member alert (title: string, ?message: string) : unit = nativeOnly

[<AllowNullLiteral>]
[<Interface>]
type AlertStatic =
    abstract member alert: (string -> string -> unit) with get, set

Problem description

The binding for the top-level function in the above example handles the optional argument message (and is tupled with labels), whereas interface functions are curried and optionality of the message argument is missing string -> string -> unit instead of string -> string option -> unit. (They also lack labels since they are curried).

@MangelMaxime
Copy link
Contributor

From the top of my head, here is the explanation why we generate a curried function and not a member.

We could have something like that:

interface Options {
    prefix: string;
    callback: (value: string) => void;
}

And in this case, Options is the description of a POJO so we need to generate F# properties and not members. Unfortunately, we can't know from the AST only if the interface describe a POJO or something similar to a class (with methods).

Will all that said, the missing string option is 100% wrong.

@MangelMaxime
Copy link
Contributor

I fixed the string option issue, if you think we can improve the generation for this code please feel free to re-open / comment below.

@joprice
Copy link
Author

joprice commented Oct 29, 2024

Thanks for the quick fix! As for the labeled arguments, I compared a few possible approaches. Not sure any of them are useful, but thought I'd share my thoughts.

One idea I had was generating a second interface, so you can either treat it as an object with properties or as an interface. Then, I wondered what would happen if I made one inherit the other. Not even sure why this works:

type Labeled = 
  abstract member alert: title:string * message:string option -> unit

type Inherited =
    inherit Labeled
    abstract member alert: (string -> string option -> unit) with get, set

let callInheritUnLabeled(a: Inherited) = a.alert "a" None
let callInheritLabeled(a: Inherited) = (a :> Labeled).alert(title = "a", message = None)

I also thought that having the property be a delegate gives the option for labels as well, but that would require generating
a delegate type per definition:

type Callback = delegate of title:string * message:string option -> unit

type Delegate =
    abstract member alert: Callback with get,set

let callDelegate(a: Delegate) = a.alert.Invoke (title = "a", message = None)

Then I was curiosu what would happen if properties are added to a tupled function, and I got the surprising result where the function being called is get_alert instead of alert.

type LabeledProperty = 
  abstract member alert: title:string * message:string option -> unit with get,set

let callLabeledProperty(a: LabeledProperty) = a.alert(title = "a", message = None)
export function callLabeledProperty(a) {
    a.get_alert("a", undefined);
}

Is this last one expected?

@MangelMaxime
Copy link
Contributor

MangelMaxime commented Oct 29, 2024

Thanks for the quick fix! As for the labeled arguments, I compared a few possible approaches. Not sure any of them are useful, but thought I'd share my thoughts.

I am always open to suggestions, I know a lot about Fable binding in general but I am sure there some tricks that I am not aware of.


Not exactly related but because you mentioned inheritance to provide different level of API shape. Below are some others situation where we consider doing something similar:


The issue with the proposed solution is that the DX is not really good.

type Labeled = 
  abstract member alert: title:string * message:string option -> unit

type Inherited =
    inherit Labeled
    abstract member alert: (string -> string option -> unit) with get, set

let callInheritUnLabeled(a: Inherited) = a.alert "a" None
let callInheritLabeled(a: Inherited) = (a :> Labeled).alert(title = "a", message = None)

Here, it is difficult to know from the consumer point of view that you can downcast to another type.


Delegate is not too bad, it just not so common to use in F#. We could perhaps, keep it in mind somewhere.


Regarding the last example, it is probably a F# compiler bug.

The code should probably not compile because the generated C# code is:

using System;
using System.Reflection;
using Microsoft.FSharp.Core;

[assembly: FSharpInterfaceDataVersion(2, 0, 0)]
[assembly: AssemblyVersion("0.0.0.0")]

[CompilationMapping(SourceConstructFlags.Module)]
public static class @_
{
    [Serializable]
    [CompilationMapping(SourceConstructFlags.ObjectType)]
    public interface LabeledProperty
    {
        override FSharpFunc<string, FSharpFunc<FSharpOption<string>, Unit>> alert1 { get; set; }

        override Unit alert2 { get; set; }
    }
}

namespace <StartupCode$_>
{
    internal static class $_
    {
        public static void main@()
        {
        }
    }
}

@MangelMaxime
Copy link
Contributor

F# team confirmed the bug regarding the third proposition

@joprice
Copy link
Author

joprice commented Oct 29, 2024

Another idea I had that doesn't affect current codegen is adding a synthetic docstring that includes the names for the function's parameters, since in typescript they are effectively documentation.

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

No branches or pull requests

2 participants