fable-compiler / Fable.Lit

Write Fable Elmish apps with Lit
https://fable.io/Fable.Lit/
MIT License
91 stars 13 forks source link

Writable attributes #31

Open JordanMarr opened 2 years ago

JordanMarr commented 2 years ago

I am trying to create a custom web component that takes attributes:

[<LitElement("fluent-icon")>]
let FluentIcon() = 
    let _, props =
        LitElement.init(fun init ->
            init.useShadowDom <- false
            init.props <- 
                {| 
                    name = Prop.Of(defaultValue = "Add", attribute = "name")
                    size = Prop.Of(defaultValue = "20px", attribute = "size")
                    color = Prop.Of(defaultValue = "red", attribute = "color")
                |}
        )
    let name = props.name.Value
    let color = props.color.Value
    let size = props.size.Value
    html $"""<i class="ms-Icon ms-Icon--{name}" style="color: {color}; font-size: {size};" aria-hidden="true"></i>"""

Usage:

<fluent-icon name="Add" color="Red" size="20px"></fluent-icon>

It works if I use static html with no inputs, but adding the the 3 input attributes: init.props <- {| ... |} results in the following error:

LitElement.fs:275 

       Uncaught TypeError: Cannot set property name of #<classExpr> which has only a getter
    at LitElement.fs:275:35
    at Seq.js:837:9
    at fold (Seq.js:691:19)
    at iterate (Seq.js:836:5)
    at initProps (LitElement.fs:274:40)
    at new LitHookElement$1 (LitElement.fs:185:8)
    at new classExpr (LitElement.fs:288:29)
    at TemplateInstance._clone (lit-html.ts:927:52)
    at ChildPart._commitTemplateResult (lit-html.ts:1270:33)
    at ChildPart._$setValue (lit-html.ts:1165:12)
JordanMarr commented 2 years ago

I think I was using reserved attribute names. Changing them fixed it!

[<LitElement("fluent-icon")>]
let FluentIcon() = 
    let _, props =
        LitElement.init(fun init ->
            init.useShadowDom <- false
            init.props <- 
                {| 
                    iconName = Prop.Of(defaultValue = "Add", attribute = "icon-name")
                    iconSize = Prop.Of(defaultValue = "20px", attribute = "icon-size")
                    iconColor = Prop.Of(defaultValue = "red", attribute = "icon-color")
                |}
        )

    html $"""
        <i class="ms-Icon ms-Icon--{props.iconName.Value}" style="color: {props.iconColor.Value}; font-size: {props.iconSize.Value};" aria-hidden="true"></i>
    """
<fluent-icon icon-name="Add" icon-color="Red" icon-size="20px"></fluent-icon>
AngelMunoz commented 2 years ago

This probably shouldn't throw in those cases though, because AFAIK those are valid attribute names

AngelMunoz commented 2 years ago

I'll try to investigate this if I have some bandwidth next week

alfonsogarciacaro commented 2 years ago

Maybe name is causing problems. If that's the case we can try to identify it at runtime to throw a more meaningful error.

AngelMunoz commented 2 years ago

I was able to track down the issue, @alfonsogarciacaro name is a valid property and it should not throw in that case

inside Hook.fs we use some class expressions to make classes extensible

module internal HookUtil =
    let [<Literal>] RENDER_FN_CLASS_EXPR =
        """class extends $0 {
            constructor() { super($2...) }
            get renderFn() { return $1 }
            // name is not defined here so no problem
        }"""

    let [<Literal>] HMR_CLASS_EXPR =
        """class extends $0 {
            constructor() { super($3...) }
            // this defines only a getter not a setter
            get name() { return $2; } // hence why it throws
            get renderFn() { return $1.value; }
            set renderFn(v) {
                $1.value = v;
                this.hooks.requestUpdate();
            }
        }"""

In LitElement.fs we're using a class expression for HMR

type LitElementAttribute(name: string) =
/// ... more code ...
        config.InitPromise
        |> Promise.iter (fun _ ->
            let config = config :> LitConfig<obj>

            let styles =
                if isNotNull config.styles then List.toArray config.styles |> Some
                else None

            let propsOptions, initProps =
                if isNotNull config.props then
                   // ... more code ...

                    let initProps (this: obj) =
                        // this is where it fails
                        // it tries to assign this['name'] = v
                        // but in the class expression we only define a getter
                        propsValues |> Seq.iter(fun (k, v) ->
                            this?(k) <- v)
                    Some propsOptions, initProps
                else
                    None, fun _ -> ()

            let classExpr =
                let baseClass = jsConstructor<LitHookElement<obj>>
#if !DEBUG
                // no `name` getter/setter defined hence it doesn't throw
                emitJsExpr (baseClass, renderFn, initProps) HookUtil.RENDER_FN_CLASS_EXPR
#else

                let renderRef = LitBindings.createRef()
                renderRef.value <- renderFn
                // in this case the JS expression used has a `name` getter only
                emitJsExpr (baseClass, renderRef, mi.Name, initProps) HookUtil.HMR_CLASS_EXPR
#endif
// ... more code

If I comment the else from the DEBUG portion the error goes away, If I use the HMR_CLASS_EXPR then the error comes back

I guess the ideal would be to use a different property like __name but I'm not sure if that is a breaking change for HMR

alfonsogarciacaro commented 2 years ago

Oh! Thanks a lot for investigating the issue @AngelMunoz! You're right, I remembered now that name is used to identify the component when cherry-picking the exports of the new module in HMR here and here.

I don't remember if there was a specific reason to pick name, __name as you suggest should work too and have less chances of conflict.