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

Writable attributes #31

Open
JordanMarr opened this issue May 14, 2022 · 6 comments
Open

Writable attributes #31

JordanMarr opened this issue May 14, 2022 · 6 comments

Comments

@JordanMarr
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Collaborator

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

@AngelMunoz
Copy link
Collaborator

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

@AngelMunoz AngelMunoz reopened this May 14, 2022
@alfonsogarciacaro
Copy link
Member

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
Copy link
Collaborator

AngelMunoz commented May 17, 2022

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
Copy link
Member

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.

alfonsogarciacaro added a commit that referenced this issue May 27, 2022
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