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

Lit.Extensions #7

Closed
wants to merge 2 commits into from
Closed

Lit.Extensions #7

wants to merge 2 commits into from

Conversation

AngelMunoz
Copy link
Collaborator

This is an intial implementation of the Lit based (class components) in a separate package, these should give us a head start on "standard" Lit, we can refine this PR with extra things

For the LitElement class I didn't add every member since most of those are for internal/advanced use cases I don't think they're needed at least not right now

Also we can scrap this completely and do it on a separate repository

let mutable nameAttribute = "World"

static member properties =
{| counter = {| state = true |}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a complex one for me to better decide how to implement with type safety

those optional members are really annoying to deal with and anonymous objects just feel nicer IMO but I know not everyine likes this

I tried to define it like this

///Defines options for a property accessor.
type PropertyDeclaration =


    /// When set to `true`, indicates the property is internal private state. The
    /// property should not be set by users. When using TypeScript, this property
    /// should be marked as `private` or `protected`, and it is also a common
    /// practice to use a leading `_` in the name. The property is not added to
    /// `observedAttributes`.
    abstract member state : bool option

    ///
    /// Indicates how and whether the property becomes an observed attribute.
    /// If the value is `false`, the property is not added to `observedAttributes`.
    /// If true or absent, the lowercased property name is observed (e.g. `fooBar`
    /// becomes `foobar`). If a string, the string value is observed (e.g
    /// `attribute: 'foo-bar'`).
    ///
    abstract member attribute : string option

    /// Indicates the type of the property. This is used only as a hint for the
    /// `converter` to determine how to convert the attribute
    /// to/from a property.
    abstract member ``type`` : obj option

    /// Indicates how to convert the attribute to/from a property. If this value
    /// is a function, it is used to convert the attribute value a the property
    /// value. If it's an object, it can have keys for `fromAttribute` and
    /// `toAttribute`. If no `toAttribute` function is provided and
    /// `reflect` is set to `true`, the property value is set directly to the
    /// attribute. A default `converter` is used if none is provided; it supports
    /// `Boolean`, `String`, `Number`, `Object`, and `Array`. Note,
    /// when a property changes and the converter is used to update the attribute,
    /// the property is never updated again as a result of the attribute changing,
    /// and vice versa.
    abstract member converter : (string option -> obj option -> obj) option

    /// Indicates if the property should reflect to an attribute.
    /// If `true`, when the property is set, the attribute is set using the
    /// attribute name determined according to the rules for the `attribute`
    /// property option and the value of the property converted using the rules
    /// from the `converter` property option.
    abstract member reflect : bool option

    /// A function that indicates if a property should be considered changed when
    /// it is set. The function should take the `newValue` and `oldValue` and
    /// return `true` if an update should be requested.
    abstract member hasChanged : (obj -> obj -> bool) option

    /// Indicates whether an accessor will be created for this property. By
    /// default, an accessor will be generated for this property that requests an
    /// update when set. If this flag is `true`, no accessor will be created, and
    /// it will be the user's responsibility to call
    /// `this.requestUpdate(propertyName, oldValue)` to request an update when
    /// the property changes.
    abstract member noAccessor : bool option

/// Map of properties to PropertyDeclaration options.
/// For each property an accessor is made,
/// and the property is processed according to the PropertyDeclaration options.
type PropertyDeclarations = System.Collections.Generic.Dictionary<string, PropertyDeclaration>

I chose Dictionary because I think that's compiled as a plain object but using it is a little bit annoying as well

let defineComponent (name: string) (comp: obj) : unit = jsNative

[<Emit("String")>]
let JsString: obj = jsNative
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the constructors for Number, Object and similar ones but Didn't find String, should we add that in Fable.Core.JS?

@AngelMunoz
Copy link
Collaborator Author

Regarding the "Function" based components that rely on lit only check this stackblitz

https://stackblitz.com/edit/js-rh5qqq?file=index.js

I'm not sure if fable can treat classes as values, but one thing is for sure it has to be a class because you need to extend LitElement which also inherits from HTMLElement which... can only be inherited by classes AFAIK but if we can include that JS script/translate to fable we should be good to go and have around 80 - 90% Lit support besides class based bindings that might be missing

we could ditch haunted completely given that we already have hooks with directives (the only one missing would be useController)

@AngelMunoz
Copy link
Collaborator Author

I'll close this based the discussion we had on #8

@AngelMunoz AngelMunoz closed this Sep 2, 2021
@alfonsogarciacaro
Copy link
Member

alfonsogarciacaro commented Sep 2, 2021

Thanks a lot for this @AngelMunoz! Yes, if we're going to provide bindings for lit-html we should also provide bindings for lit-element (still unsure what are the differences between lit-element and @lit/reactive-element, maybe we just need the latter?). Still wondering if we should explore the function approach before trying to use the class. With decorators, we can just build the JS class on the fly as haunted does or have a class with the common functionality and inherit with the render function as the HookDirective does. The new decorators feature in fable 3.3 is going to allow reflection info on the method. But F#/.NET attributes only accept literal values (strings, numbers and booleans, basically), so for styles and properties initialization we would need something like a hook:

[<LitElement("my-component")>]
let MyComponent () =
    let props = LitElement.init initStyles (fun () ->
       {| name = Prop("default")
          age = Prop(5, attribute=true, hasChanged= fun x y -> x < y) |} // Sadly, you cannot be younger, so age only changes up

    html $"""<p>Name: {props.name.Value} ({props.age.Value} years old)</p>"""

This should appear on top of the function and we can make a first run of the function to get the styles and properties. Then we can attach them to the generated class. For lifecycle we can just use the effects hooks. What do you think?

I found the constructors for Number, Object and similar ones but Didn't find String, should we add that in Fable.Core.JS?

I think I was trying to make devs just use F# instead, but we've already added other constructors with F#/.NET equivalents, so probably we can add JS.String as well for completeness.

@AngelMunoz
Copy link
Collaborator Author

AngelMunoz commented Sep 2, 2021

still unsure what are the differences between lit-element and @lit/reactive-element, maybe we just need the latter?

ReactiveElement is the base class for LitElement I think for our purposes we can simply use the LitElement as our base class, as long as we are able to provide the static property properties we should be able to "react" to changes when consumers change properties or attributes (if the component autor allows it)

or have a class with the common functionality and inherit with the render function as the HookDirective does.

I think the link is pointing to this PR, I think that would be the ideal way to do it taking the example on the stackblitz link above this is the key behavior

const cls = class extends LitElement {
    // react to changes in the attributes

    render() {
      const fnArgs = Object.keys(opts?.props).reduce((curr, propName) => {
        // Lit will observe properties as long as we specify them in
        // the static properties geter
        curr[propName] = this[propName];
        // if the value is not set it's likely that the user specified
        if (opts?.props[propName]?.attribute && !curr[propName]) {
          // attribute property, the attribute property can be a string or a boolean
          if (typeof opts?.props[propName]?.attribute === 'string') {
            // if it's a string it means they used a different attributeNaeme
            curr[propName] = this.getAttribute(attribute);
          } else {
            // otherwise it should be the provided key
            curr[propName] = this.getAttribute(propName);
          }
        }
        return curr;
      }, {});
      // a the end simply invoke
      return viewFn(fnArgs);
    }
  };

then I add the static properties to cls we could also add the static styles there that can be an array of the tagged literal css or even a constructable stylesheet (the ones I added to Fable.ShadowStyles)

I like the args first approach used by haunted rather than the LitElement.init because it kind of lets the author focus on the function rather than the configuration, the configuration can come later when you register the component (because you can't skip that anyways either by decorator or by hand) one thing we should make very very clear to users is that attributes and properties are always optional (if we go for the args first approach) because there is no mechanism that can force a consumer to specify a particular attribute/property

for the configuration I've been thinking over the last two days how to make it more... F#'ish, I was thinking about using a DU somewhat like this

type AttributeConverter = 
    | Complex of {| fromAttribute: string option -> obj option -> obj; toAttribute: obj -> obj -> obj |}
    | Simple of string option -> obj option -> obj

type Property = 
    | Internal // {  state: true  }
    | Attribute // { attribute: true } -> 'myName' in html
    | CustomAttribute of string // { attribute: differentName } 
    | Type of  obj // {  type: Number }
    | HasChanged of  obj -> obj -> bool
    | Converter of AttributeConverter
    | Reflective // { reflect: true }
    | NoAccessor // { noAcessor: true}

let props = [
  "age", [ Internal ] // { age: { state: true } }
  "color",
     [ Type(JS.Object);
       Converter (Simple(fun color -> stringToColor color)) ]
     // { color: { type: Object, converter: (color) => stringToColor(color) } }
]

and then just create the static props for the user with the given definitions

defineComponent ("my-component", MyComponent, props)

I guess we can use the hook to do that or a mix of both approaches?

I'm fairly unfamilar still of how those work (kinda gave me the idea after looking at decompiled source but haven't been able to dive deep with them)

I think we can then re-open this one and add the missing things for the Function based Lit components

@AngelMunoz
Copy link
Collaborator Author

AngelMunoz commented Sep 2, 2021

defineComponent ("my-component", MyComponent, props)

Oh something that is missing here as well is the ability to turn on/off the shadow DOM for a particular component which is crucial until constructable stylesheets arrive in all browsers and to have interop with existing css frameworks like boostrap/bulma/etc

which I addresed it on the stackblitz by adding the props object into the options and then picking the props from the options... it's kind of weird

@AngelMunoz
Copy link
Collaborator Author

@AngelMunoz
Copy link
Collaborator Author

I'm doing some progress on the attributes as I thought they could be used but the functions for the HasChanged case get curried and that won't work with Lit, here's a Fable Repl is there something I can use to compile them uncurried?

@alfonsogarciacaro
Copy link
Member

Sorry, I've fixed the links in my previous comment :)

I think for our purposes we can simply use the LitElement as our base class,

Yes, it seems ReactiveElement is just LitElement minus the rendering with lit-html. I guess it's designed to be wrapper of components in other frameworks: React, Vue, Svelte.

I like the args first approach used by haunted rather than the LitElement.init because it kind of lets the author focus on the function rather than the configuration, the configuration can come later when you register the component

That's true, although I see a couple of problems with using the arguments for the properties in Web Components:

  • First, this way Fable devs would understand the function can be called directly (as we do with React components), but if I understand it correctly, web components must be instantiated in HTML.
  • Second, if we have the function arguments, and then the configuration of these later, it will be difficult to check at compile-time that the names and the types match.

one thing we should make very very clear to users is that attributes and properties are always optional

If I understand the Lit examples correctly, if you provide a default value you don't need to deal with undefined options. This would be another point in making the props available trough an init function.

@AngelMunoz
Copy link
Collaborator Author

  • First, this way Fable devs would understand the function can be called directly (as we do with React components), but if I understand it correctly, web components must be instantiated in HTML.

That's a fair point,I think this will require some guidance regarding on "how-to register components"
because even if they have unit as a parameter, some people might feel tempted to call functions from other modules into another function which might break stuff since some components will depend on the correct this value

that's the reason why on the Fable.Lit.Templates I declared such functions private and only exposed a register one precisely because components as they are must be created via html or document.create('my-component')

If I understand the Lit examples correctly, if you provide a default value you don't need to deal with undefined options.

that's true, but at runtime consumers can assign null/undefined to properties and remove the attributes as well even with the init approach we would need to remember users that the runtime doesn't play by the F# rules (which is somewhat similar to the C# <-> F# interop)

@alfonsogarciacaro
Copy link
Member

@AngelMunoz It still needs some work but I added a draft implementation of the LitElement decorator so we can convert render functions to web components with an init function and react-style hooks. This is what it looks like, I think it's a nice way of defining web components and it makes for a nice transition of an Elmish component to a web component, what do you think?

@AngelMunoz AngelMunoz mentioned this pull request Sep 10, 2021
@AngelMunoz
Copy link
Collaborator Author

Hey @alfonsogarciacaro the only thing that might be confusing here is the translation between camelCase and dash-case for attributes
in your example you used hourColor="red" this will always be converted to lowercase by browsers, for properties that's fine because lit uses .hourColor={color} for attributes it is expected to be something along hour-color={strColor}
But based on the comments of the PR, I guess with the Prop optional arguments this can be fixed Prop("red", customAttr = "hour-color") or somewhat along those lines

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