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

Try to replace U2 arguments with overloads or common interfaces #1

Open
alfonsogarciacaro opened this issue Apr 1, 2019 · 17 comments

Comments

@alfonsogarciacaro
Copy link
Member

No description provided.

@MangelMaxime
Copy link
Member

MangelMaxime commented Apr 1, 2019

Totally for this one :)

It's how I update my bindings now and it's so much easier/natural to use

@whitetigle
Copy link
Contributor

ok I will refine my current WIP

@whitetigle
Copy link
Contributor

Question how do I transform this?

static member fileURLToPath: U2<URL,string> -> string = jsNative

@MangelMaxime
Copy link
Member

The error from F# is indeed "funny" when you do:

type URL = int

type Test () =
    static member fileURLToPath: string -> string = jsNative
    static member fileURLToPath: URL -> string = jsNative

Error:

Duplicate method. The method 'get_fileURLToPath' has the same name and signature as another method in type 'Test'.

So it's comparing only the number of arguments and the types... I would vote for using overload on abstract because it works here.

And for static member, something like where we rename the member but it's will not always be good when you have U3, U4, U5, etc. or several arguments that use UX...

I don't really know when Fable is generating the static member but I guess we could use a module to mock the type name and use a type with abtract member on it.

This is actually what I did to mock a function from FontAwesome which I think is similarly resolve in term of constraints as static member from F# compiler.

export function icon(icon: IconName | IconLookup, params?: IconParams): Icon;
module Exports =
    open Fable.Core.JsInterop

    type IconModule =
        [<Emit("$0({iconName : $1, prefix: $2},Object.assign({}, $3))")>]
        abstract Find: iconName: string * prefix: string * ?parameters: IconParams -> Icon
        [<Emit("$0({iconName : $1, prefix: 'far'},Object.assign({}, $2))")>]
        abstract FindRegular: icon: IconName * ?parameters: IconParams -> Icon
        [<Emit("$0({iconName : $1, prefix: 'fas'},Object.assign({}, $2))")>]
        abstract FindSolid: icon: IconName * ?parameters: IconParams -> Icon
        [<Emit("$0({iconName : $1, prefix: 'fab'},Object.assign({}, $2))")>]
        abstract FindBrand: icon: IconName * ?parameters: IconParams -> Icon
        [<Emit("$0({iconName : $1, prefix: 'fal'},Object.assign({}, $2))")>]
        abstract FindLight: icon: IconName * ?parameters: IconParams -> Icon

    let icon : IconModule = importMember "@fortawesome/fontawesome-svg-core"

And so like that, I am able to kind of mock the JavaScript API in a nice way for F# code. The emit is complex here this is to make the API even nicer to consume.

@whitetigle
Copy link
Contributor

hmm. why not. I'm going to try this. Thanks @MangelMaxime!

@MangelMaxime
Copy link
Member

Also, if the code looks ugly or is not possible to be supported. I guess we could only support one of the methods with an annotation stating it.

For example, because we can pass an URL or a string we could explicitly say we support only string and so the user need to convert URL into a string before using the method.

That's not perfect, but at least the user experience is still free of "hacks" like U2.Case1 myUrlOb or U2.Case2 myUrlAsAString or !^ myUrlAsAString

@whitetigle
Copy link
Contributor

I agree with you!

@alfonsogarciacaro
Copy link
Member Author

Hmm, why you need static member? Ideally bindings should contain only interface definitions and then the module values to expose the constructor types.

In any case if you need to use static member you need to use an "implementation" signature. In the example above, the compiler thinks you're defining two getters with the same name that return two different types of functions, thus the error. Overloading would work like this:

type Test () =
    static member fileURLToPath(x: string): string = jsNative
    static member fileURLToPath(x: URL): string = jsNative

@MangelMaxime
Copy link
Member

Oh I didn't know.

If what @alfonsogarciacaro says is right then it's better than any hacks if we need static member :)

@whitetigle
Copy link
Contributor

whitetigle commented Apr 11, 2019

I currently did that:

module Static = 
    let domainToASCII: string -> string = importMember "url"
    let domainToUnicode: string -> string = importMember "url"
    let fileURLToPath: string -> string = importMember "url"
    let fileURLToPathFromURL: URL -> string = importMember "url"
    let pathToFileURL: string -> URL = importMember "url"

The question I was asking myself this morning, was how to map static members for the Url type? So a type with static members and a single import * would work. But I had the overloading problem for fileURLToPath function.

Honestly, I'm trying to make my way and there are a few things I quite don't grasp yet like the format problem I raised in the last PR. Here's the definition. url.js defines exports like this:

module.exports = {
  // Original API
  Url,
  parse: urlParse,
  resolve: urlResolve,
  resolveObject: urlResolveObject,
  format: urlFormat,

  // WHATWG API
  URL,
  URLSearchParams,
  domainToASCII,
  domainToUnicode,

  // Utilities
  pathToFileURL,
  fileURLToPath
};

How do I access the format member? Because there seems to be a trick there: there's actually the old format method which is available from the Url prototype. I guess that's the root of my current problem: format is found but the prototype I'm refering to in teh bindings is not what is really found in the .js source code.

Again These are the kind of things I'm patiently learning ;)

@MangelMaxime
Copy link
Member

@whitetigle Which format API are you speaking about?

For url.format(urlObject) the following code works:

// url.format({
//   protocol: 'https',
//   hostname: 'example.com',
//   pathname: '/some/path',
//   query: {
//     page: 1,
//     format: 'json'
//   }
// });
//
// => 'https://example.com/some/path?page=1&format=json'

module URL =
    type FormatOptions =
        abstract member protocol : string with get, set
        abstract member hostname : string with get, set
        abstract member pathname : string with get, set
        abstract member query : obj with get, set

    let format (options : FormatOptions) : string = importMember "url"

let options =
    jsOptions<URL.FormatOptions>(fun o ->
        o.protocol <- "https"
        o.hostname <- "example.com"
        o.pathname <- "/some/path"
        o.query <- createObj [
            "page" ==> 1
            "format" ==> "json"
        ]
    )

let formattedURL = URL.format(options)
JS.console.log formattedURL
// Print: https://example.com/some/path?page=1&format=json

@whitetigle
Copy link
Contributor

Nope, the relatively new one.

@MangelMaxime
Copy link
Member

Ok, I have a working version which supports both format method.

Please note, that I wrote the binding from scratch without thinking if this is breaking the existant API and if this is easily scalable.

// url.format({
//   protocol: 'https',
//   hostname: 'example.com',
//   pathname: '/some/path',
//   query: {
//     page: 1,
//     format: 'json'
//   }
// });
//
// => 'https://example.com/some/path?page=1&format=json'

module URL =
    type LegacyFormatOptions =
        abstract member protocol : string with get, set
        abstract member hostname : string with get, set
        abstract member pathname : string with get, set
        abstract member query : obj with get, set

    type FormatOptions =
        /// `true` if the serialized URL string should include the username and password, `false` otherwise. Default: `true`.
        abstract member auth : bool with get, set
        /// `true` if the serialized URL string should include the fragment, `false` otherwise. Default: `true`.
        abstract member fragment : bool with get, set
        /// `true` if the serialized URL string should include the search query, `false` otherwise. Default: `true`.
        abstract member search : bool with get, set
        /// `true` if Unicode characters appearing in the host component of the URL string should be encoded directly as opposed to being Punycode encoded. Default: `false`.
        abstract member unicode : bool with get, set

    type [<AllowNullLiteral>] URL =
        abstract hash: string  with get, set
        abstract host: string  with get, set
        abstract hostname: string  with get, set
        abstract href: string  with get, set
        abstract origin: string
        abstract password: string  with get, set
        abstract pathname: string  with get, set
        abstract port: string  with get, set
        abstract protocol: string with get, set
        abstract search: string with get, set
        // abstract searchParams: URLSearchParams // commented to make reproduction easier
        abstract username: string with get, set
        abstract toString: unit -> string
        abstract toJSON: unit -> string

    type IExports =
        abstract member format : LegacyFormatOptions -> string
        abstract member format : url : URL * ?options: FormatOptions -> string
        [<Emit("new URL($1...)")>]
        abstract Create: input:string -> URL

let url : URL.IExports = import "default" "url"

// Legacy code

let legacyOptions =
    jsOptions<URL.LegacyFormatOptions>(fun o ->
        o.protocol <- "https"
        o.hostname <- "example.com"
        o.pathname <- "/some/path"
        o.query <- createObj [
            "page" ==> 1
            "format" ==> "json"
        ]
    )

let formattedURL = url.format(legacyOptions)
JS.console.log formattedURL
// Print: https://example.com/some/path?page=1&format=json

// New code

// const myURL = new URL('https://a:b@測試?abc#foo');

// console.log(myURL.href);
// // Prints https://a:b@xn--g6w251d/?abc#foo

// console.log(myURL.toString());
// // Prints https://a:b@xn--g6w251d/?abc#foo

// console.log(url.format(myURL, { fragment: false, unicode: true, auth: false }));
// // Prints 'https://測試/?abc'

let myUrl = url.Create("https://a:b@測試?abc#foo")

JS.console.log myUrl.href
// Prints https://a:b@xn--g6w251d/?abc#foo

JS.console.log (myUrl.toString())
// Prints https://a:b@xn--g6w251d/?abc#foo


let options =
    jsOptions<URL.FormatOptions>(fun o ->
        o.fragment <- false
        o.unicode <- true
        o.auth <- false
    )

let formatted = url.format(myUrl, options)
JS.console.log formatted
// Prints: https://測試/?abc

@whitetigle
Copy link
Contributor

Great! I'm going to see how to merge this.

@whitetigle
Copy link
Contributor

Ok found why it would not work!

working:

[<Import("*", "url")>]
let URL: Url.URLType = jsNative

vs

not working:
let [<Global>] URL: Url.URLType = jsNative

  • put format into good place.

Thanks @MangelMaxime!

@MangelMaxime
Copy link
Member

Ah yes indeed:

let [<Global>] URL: Url.URLType = jsNative is compiled to something like URL

while

[<Import("*", "url")>]
let URL: Url.URLType = jsNative

compiles to

var url = require("url");

The first one is accessing a global variable while the second one is loading a module.

@whitetigle
Copy link
Contributor

Yes should have better read the doc:

Global Objects: These objects are available in all modules. The following variables may appear to be global but are not

whitetigle added a commit that referenced this issue Jul 15, 2019
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