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

Use classes for the bindings? #77

Open
alfonsogarciacaro opened this issue Sep 11, 2021 · 7 comments
Open

Use classes for the bindings? #77

alfonsogarciacaro opened this issue Sep 11, 2021 · 7 comments

Comments

@alfonsogarciacaro
Copy link
Member

alfonsogarciacaro commented Sep 11, 2021

So far we (I) have been recommending interfaces to write bindings, but in recent discussions we are considering the possibility of using classes instead. Some links:

fsharp/fslang-suggestions#1054
fable-compiler/Fable#2353
fable-compiler/Fable#2492

So I'm toying now with the idea of writing a script to automatically convert some of the bindings in this repo to classes to see how far we can go, and if the ergonomics is good enough to start recommending writing classes instead of interfaces (and whether we should change ts2fable).

Merits:

  • We could use normal constructors (we would still leave the static Create method to avoid breaking changes)
  • We could do type testing

Demerits:

  • Namespaces could be a breaking change. We currently have Browser.Types and Browser for the values exposing the changes. But in many cases we wouldn't need
  • Multiple inheritance: many interfaces inherit from more than one interface. Given that JS doesn't allow multiple inheritance either, my hope is that those cases always correspond to actual interfaces. But this mean we will have to review manually multiple cases to see if they can become a class or not.
  • Virtual methods: I think there's no need to inherit and override a method of any type in this repo (mainly because it's not possible right now and we didn't have complaints, although writing raw web components do require this) but if it does become necessary we need to identify the methods and use the verbose F# syntax for virtual declaration (abstract signature plus default declaration with dummy implementation).

Thoughts? cc @MangelMaxime @Zaid-Ajaj @inosik @Booksbaum

@MangelMaxime
Copy link
Member

MangelMaxime commented Sep 11, 2021

We could use normal constructors (we would still leave the static Create method to avoid breaking changes)

TBH, I prefer the breaking changes as it would mean writing "standard" F# code instead of having to explain why there is 2 ways of doing it.

  • Namespaces could be a breaking change. We currently have Browser.Types and Browser for the values exposing the changes. But in many cases we wouldn't need

Less namespace magic seems good to me.

@MangelMaxime
Copy link
Member

MangelMaxime commented Sep 14, 2021

Today there has been a question on F# Slack about how to support multiple constructor in F#

Hello, what is the recommended way to make a Fable binding for a JS class with multiple constructors? Trying

and [<Import("Color","three")>] Color(r: float, g: float, b: float) =
        new(hex: int) = jsNative
        new(str: string) = jsNative

doesn't work. Do I just need to use the U3 multiple case paradigm?

Is this a Fable limitation when using constructor or did it wrote it the incorrect way?

Ok after trying the code into the REPL, this is not a Fable limitation but F# limitation:

This is not a valid object construction expression. Explicit object constructors must either call an alternate constructor or initialize all fields of the object and specify a call to a super class constructor.

@alfonsogarciacaro
Copy link
Member Author

Yes, this is tricky. First of all, there are no JS classes with multiple constructors, that's just a Typescript mechanism like the fake overloads. But the fake overloads are correctly called by Fable for classes decorated with Import/Global. I just tried it in the REPL and it seems multiple constructors work too. The only thing is, as you've already noticed, you cannot use jsNative (nor failwith) for the dummy implementation, you have to write a dummy call to the primary constructor. Example:

open Fable.Core

[<Import("Color","three")>] 
type Color(r: float, g: float, b: float) =
    new(hex: int) = Color(0.,0.,0.)
    new(str: string) = Color(0.,0.,0.)

let c = Color("foo")

Becomes:

import { Color } from "three";

export const c = new Color("foo");

@MangelMaxime
Copy link
Member

Thank you for the explanations @alfonsogarciacaro

@inosik
Copy link

inosik commented Sep 16, 2021

@alfonsogarciacaro I looked at the DOM bindings of Bridge.NET and they also have proper types and values of TType. For example, they have a XMLHttpRequest class, and a static property dom.XMLHttpRequestType of type XMLHttpRequestTypeConfig, which defines a New method that returns XMLHttpRequest values. But I think most of the time we don't need this, because we can just use constructors.

Do you have any examples of multiple inheritance in the DOM APIs?

@MangelMaxime this is what I do lately:

// Private parameterless constructor, can be public if necessary
type T private () =
  /// <summary>XML docs for the first constructor.</summary>
  /// <param name="arg">What is arg for?</param>
  new (arg: obj) =
    // Call the primary constructor. No need to pass any arguments
    T ()

  /// XML docs for the second constructor
  new (arg0: obj, arg1: obj) = T ()

  // "Minimal" binding for T.Prop. This syntax is not allowed without a primary constructor
  member val Prop: obj = jsNative with get, set

@alfonsogarciacaro
Copy link
Member Author

@inosik For example, Element inherits from multiple interfaces:

type [<AllowNullLiteral>] Element =
inherit Node
inherit GlobalEventHandlers
inherit NodeSelector
inherit ChildNode

I think in this case the "actual" parent is Node and the rest are interfaces. But these cases make an automatic conversion difficult because we have to identify

Unfortunately I don't have much time to work on this atm, but if someone wants to give it a try with a small package we can see how the ergonomics work and have a feeling of how much work would a full conversion entail.

@inosik
Copy link

inosik commented Sep 28, 2021

I think in this case the "actual" parent is Node and the rest are interfaces.

According to MDN, Element inherits Node inherits EventTarget. As far as I can tell, the other interfaces are defined by the authors of the typings for TypeScript in the generator tools repository. They have many interfaces like HTMLOrSVGElement, which isn't an actual thing.

I'll see if I can find a nice, machine-readable definition of the DOM/Browser APIs and see if we can go from there.

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

3 participants