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

Fixed #3658 #3838

Merged
merged 3 commits into from
Jun 13, 2024
Merged

Fixed #3658 #3838

merged 3 commits into from
Jun 13, 2024

Conversation

ncave
Copy link
Collaborator

@ncave ncave commented Jun 10, 2024

@ncave
Copy link
Collaborator Author

ncave commented Jun 10, 2024

@MangelMaxime I still think we should only suppress the testers for the erased union cases, and let bundlers eliminate the unused code (same as with other already generated methods like reflection etc.).

I think it makes more sense this way, as people might be expecting these testers to be available.
I've already made that change, let me know if you disagree.

@MangelMaxime
Copy link
Member

I still think we should only suppress the testers for the erased union cases, and let bundlers eliminate the unused code (same as with other already generated methods like reflection etc.).

@ncave I am ok with doing it this way I just have a suggestion regarding the new warning (see comment above).

@ncave
Copy link
Collaborator Author

ncave commented Jun 12, 2024

@MangelMaxime I updated it to inline all the union case testers instead, so it should work as it does today.

P.S. I still think it's not a good idea to model free-form open-ended POJOs with F# unions, there must be a better way we can suggest for new bindings, but I guess for now this should fix existing legacy bindings.

src/Fable.Transforms/FSharp2Fable.Util.fs Dismissed Show dismissed Hide dismissed
src/Fable.Transforms/Rust/Fable2Rust.fs Dismissed Show dismissed Hide dismissed
src/Fable.Transforms/Rust/Fable2Rust.fs Dismissed Show dismissed Hide dismissed
src/Fable.Transforms/Rust/Fable2Rust.fs Dismissed Show dismissed Hide dismissed
@MangelMaxime
Copy link
Member

MangelMaxime commented Jun 13, 2024

Awesome thank you @ncave

P.S. I still think it's not a good idea to model free-form open-ended POJOs with F# unions, there must be a better way we can suggest for new bindings, but I guess for now this should fix existing legacy bindings.

I do agree on that, we have better ways of doing it now days using how Feliz works.

open Fable.Core
open Fable.Core.JsInterop

// Create an interface to force the representation
type IHTMlAttr = interface end

module Interop = 
    
    let inline mkIHTMlAttr (key : string) (value : obj) = unbox<IHTMlAttr> (key, value)

// Use an erased interface with inlined static members to represent the different cases
// The [<Erase>] attribute is used to not generate the constructor or reference information
// Inlining the static members minimizes the bundle size and also allows Fable 
// to generate a Literal POJO later on
// 
// Benefit of using an interface with static member versus a module and functions
// is that we gain access to overload making it possible to avoid U2<string, float>
// and instead use two separate overloads
[<Erase>]
type HTMLAttr =
    
    static member inline Href (value : string) =
        Interop.mkIHTMlAttr "href" value

    static member inline Custom (key : string) (value : string) =
        unbox<IHTMlAttr> (key, value)

// Create an helper function to guide the user in creating the object
// This is inlined to, to give a chance to Fable to generate a Literal POJO
let inline createHtmlAttr (a : IHTMlAttr list) = 
    createObj (unbox a)

// Here a literal POJO is generated
let myInstance =
    createHtmlAttr [ 
        HTMLAttr.Href "http://www.google.com" 
        HTMLAttr.Custom "data-delay" "100ms"
    ]

generates

export const myInstance = {
    href: "http://www.google.com",
    "data-delay": "100ms",
};

I should probably add similar exemple to Fable documentation officially explaining this trick to people and when documenting the keyValueList mark it has obsolete compared to this code.

Thinking about it, it is perhaps possible to upgrade Fable.React to this new style while keeping the double list syntax. The difficulty however would be how to support SSR via this style. If someone think it is worth it and want to give it a try and think it feel free to send a PR.

@MangelMaxime MangelMaxime merged commit 7f6c193 into fable-compiler:main Jun 13, 2024
19 checks passed
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.

2 participants