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

Replace the term attr by prop #2

Closed
MangelMaxime opened this issue Jul 21, 2019 · 9 comments
Closed

Replace the term attr by prop #2

MangelMaxime opened this issue Jul 21, 2019 · 9 comments

Comments

@MangelMaxime
Copy link
Contributor

On twitter, I mentioned that I would prefer using prop instead of attr so I would like to continue the discussion here.

My point is that Feliz seems to come closer to the react philosophy where everything is a component and everything accepts properties. I don't know if that voluntary or not.

The original React DSL, is based on JSX (and so HTML) syntax which is a layer on top of react. In JSX, we are passing attributes/properties and have a separate way to pass children.

While in fact for react children are passed using children property. So for react, everything is a property.

I think that using prop instead of attr will favorize the adoption of Feliz. The other benefit is that if someone uses Feliz syntax for writing/binding components the term prop is correct in that case too.

And because HTML element (div, span, etc.) should be considered the lowest-level components possible for me it makes sense to use prop term.

IHMO asking the user to create an alias in each of their module/projects is not a good alternative because they will be thinking in term of prop while the documentation could mention attr. Or for newcomers, it's not really friendly to use module alias, etc.

What do you think?

@Zaid-Ajaj
Copy link
Owner

My point is that Feliz seems to come closer to the react philosophy where everything is a component and everything accepts properties. I don't know if that voluntary or not.

It is not necessarily what I meant to use, in short: I chose this syntax because it is consistent and easy to work with from F# perspective and it is something that I will use in my code.

The original React DSL, is based on JSX (and so HTML) syntax which is a layer on top of react. In JSX, we are passing attributes/properties and have a separate way to pass children.

While in fact for react children are passed using children property. So for react, everything is a property.

Actually in React, children are not props, they are passed as the second argument of the createElement function, which is also what I am doing in the code

<div id="main">
   <h1>Content</h1>
</div>

// translates to

import { createElement } from 'react' 

createElement("div", { id: "main" }, [ createElement("h1", { }, "Content") ])

think that using prop instead of attr will favorize the adoption of Feliz. The other benefit is that if someone uses Feliz syntax for writing/binding components the term prop is correct in that case too.

And because HTML element (div, span, etc.) should be considered the lowest-level components possible for me it makes sense to use prop term.

I am not against prop but I don't want it either in my code either: just personal preference. I am sure someone else will come and say that prop should now be gone forever and going back to default DSL. I have given up trying to make something that fits everyone so for now I will stick with attr because I like them 😄 simple as that

@MangelMaxime
Copy link
Contributor Author

Actually in React, children are not props, they are passed as the second argument of the createElement function, which is also what I am doing in the code

Children can be passed as the third argument to createElement or via props which is what they end up being in the end for access from the view, diffing algorithm, etc.

The resolution strategy is

  1. Children are initialized from the children property
  2. If you provide the children via createElement third argument then it's replacing the children property with that value.

Here is an live example

I add to make my own binding for createElement because the one from Fable.React force the children property while it should be optional. And so force us to overwrite with null.

type TestProps =
    { 
        children : ReactElement 
    }

type IReactApi =
    abstract createElement: comp: obj * props: obj -> ReactElement

[<Global("React")>]
let reactApi : IReactApi = jsNative

div [] [
    reactApi.createElement("a", {
            children = 
                div [ ]
                    [ str "Hello" ]
        })
] |> mountById "elmish-app"

This means that we can avoid at least one iteration over the property list here to avoid finding the children value.

And we can even go further:

Here is an implementation of the current version of Feliz (compatible with the REPL):
// More info about Fulma at https://mangelmaxime.github.io/Fulma/
module Fulma.Container

open Fable.Core
open Fable.Core.JsInterop
open Fable.React
open Fable.React.Props
open Fulma


type IReactApi =
    abstract createElement: comp: obj * props: obj -> ReactElement
    abstract createElement: comp: obj * props: obj * [<ParamList>] children: ReactElement seq -> ReactElement

[<Global("React")>]
let reactApi : IReactApi = jsNative

type IReactAttribute = interface end

module Interop =

    let reactElement (name: string) (props: 'a) (children: 'b) : ReactElement = reactApi.createElement(name, props, children)

    let inline mkAttr (key: string) (value: obj) : IReactAttribute = unbox (key, value)

    [<Emit("$2[$0] = $1")>]
    let setProperty (key: string) (value: obj) (objectLiteral: obj) = jsNative

    let createObj (properties: obj list) =
        let propsObj = obj()
        let propsList = unbox<(string * obj) list> properties
        for (key, value) in propsList do
            if key <> "children" then
                setProperty key value propsObj
        propsObj

    let extract (pred: 't -> bool) (xs: 't list) : 't option * 't list =
        let rec extract' pred xs elem acc =
            match xs, elem with
            | [ ], Some _ -> elem, acc
            | [ ], None -> None, acc
            | values, Some _ -> elem, List.append values acc
            | x :: rest, None when pred x -> Some x, List.append acc rest
            | x :: rest, None -> extract' pred rest None (List.append [x] acc)

        extract' pred xs None [ ]

    let createElement name (properties: IReactAttribute list) : ReactElement =
        let props = unbox<(string * obj) list> properties
        match extract (fun (key, value) -> key = "children") props with
        | None, rest ->
            let restProps = createObj (unbox rest)
            reactElement name restProps null
        | Some (key, value), rest ->
            let restProps = createObj (unbox rest)
            reactElement name restProps (unbox value)

// [<Erase>]
type attr() =
    static member inline className(value: string) = Interop.mkAttr "className" value
    /// Alias for inline `attr.children [ Html.content value ]`
    static member inline content(value: string) = attr.children [ unbox value ]
    /// Alias for inline `attr.children [ Html.content value ]`
    static member inline content(value: int) = attr.children [ unbox value ]
    /// Alias for inline `attr.children [ Html.content value ]`
    static member inline content(value: ReactElement) = attr.children [ value ]
    static member children (elems: ReactElement list) = Interop.mkAttr "children" (Array.ofList elems)

[<Erase>]
type Html =
    /// The `<div>` tag defines a division or a section in an HTML document
    static member inline div xs = Interop.createElement "div" xs

let view =
    Html.div [
        attr.className "button"
        attr.content "Click me"
    ]

mountById "elmish-app" view

This generates 109 lines of JavaScript

Generated JavaScript
import { ofList } from "fable-library/Array.js";
import { ofArray, append } from "fable-library/List.js";
import { some } from "fable-library/Option.js";
import { type } from "fable-library/Reflection.js";
import { iterate } from "fable-library/Seq.js";
import { declare, List } from "fable-library/Types.js";
import { Helpers$$$mountById as Helpers$0024$0024$0024mountById } from "fable-repl-lib/src/Fable.React.Helpers";
export function Interop$$$reactElement(name, props, children) {
  return React.createElement(name, props, ...children);
}
export function Interop$$$createObj(properties) {
  const propsObj = {};
  const propsList = properties;
  iterate(function (forLoopVar) {
    if (forLoopVar[0] !== "children") {
      propsObj[forLoopVar[0]] = forLoopVar[1];
    }
  }, propsList);
  return propsObj;
}
export function Interop$$$extract(pred, xs) {
  const extract$0027 = function extract$0027($arg$$4, $arg$$5, $arg$$6, $arg$$7) {
    var x, rest;

    extract$0027: while (true) {
      const pred$$1 = $arg$$4,
            xs$$1 = $arg$$5,
            elem = $arg$$6,
            acc = $arg$$7;
      const matchValue = [xs$$1, elem];

      if (matchValue[0].tail != null) {
        if (matchValue[1] == null) {
          if (x = matchValue[0].head, (rest = matchValue[0].tail, pred$$1(x))) {
            return [some(matchValue[0].head), append(acc, matchValue[0].tail)];
          } else {
            var $target$$8, rest$$2, x$$2;

            if (matchValue[0].tail != null) {
              if (matchValue[1] == null) {
                $target$$8 = 0;
                rest$$2 = matchValue[0].tail;
                x$$2 = matchValue[0].head;
              } else {
                $target$$8 = 1;
              }
            } else {
              $target$$8 = 1;
            }

            switch ($target$$8) {
              case 0:
                {
                  $arg$$4 = pred$$1;
                  $arg$$5 = rest$$2;
                  $arg$$6 = null;
                  $arg$$7 = append(new List(x$$2, new List()), acc);
                  continue extract$0027;
                }

              case 1:
                {
                  throw new Error("The match cases were incomplete against type of 'List' at test.fs");
                }
            }
          }
        } else {
          return [elem, append(matchValue[0], acc)];
        }
      } else if (matchValue[1] == null) {
        return [null, acc];
      } else {
        return [elem, acc];
      }

      break;
    }
  };

  return extract$0027(pred, xs, null, new List());
}
export function Interop$$$createElement(name$$1, properties$$1) {
  const props$$1 = properties$$1;
  const matchValue$$1 = Interop$$$extract(function (tupledArg) {
    return tupledArg[0] === "children";
  }, props$$1);

  if (matchValue$$1[0] != null) {
    const value$$2 = matchValue$$1[0][1];
    const key$$2 = matchValue$$1[0][0];
    const restProps$$1 = Interop$$$createObj(matchValue$$1[1]);
    return Interop$$$reactElement(name$$1, restProps$$1, value$$2);
  } else {
    const restProps = Interop$$$createObj(matchValue$$1[1]);
    return Interop$$$reactElement(name$$1, restProps, null);
  }
}
export const attr = declare(function Fulma_Container_attr() {});
export function attr$reflection() {
  return type("Fulma.Container.attr");
}
export function attr$$$$002Ector() {
  return this instanceof attr ? attr.call(this) : new attr();
}
export function attr$$$children$$6E3A73D(elems) {
  return ["children", ofList(elems, Array)];
}
export const view = Interop$$$createElement("div", ofArray([["className", "button"], attr$$$children$$6E3A73D(new List("Click me", new List()))]));
Helpers$0024$0024$0024mountById("elmish-app", view);

Live version

If we use the children property then we can use the native keyValueList function and reduce the generated JavaScript.

// More info about Fulma at https://mangelmaxime.github.io/Fulma/
module Fulma.Container

open Fable.Core
open Fable.Core.JsInterop
open Fable.React
open Fable.React.Props
open Fulma


type IReactApi =
    abstract createElement: comp: obj * props: obj -> ReactElement
    abstract createElement: comp: obj * props: obj * [<ParamList>] children: ReactElement seq -> ReactElement

[<Global("React")>]
let reactApi : IReactApi = jsNative

type IReactAttribute = interface end

module Interop =

    let reactElement (name: string) (props: IReactAttribute seq) : ReactElement = reactApi.createElement(name, keyValueList CaseRules.LowerFirst props)

    let inline mkAttr (key: string) (value: obj) : IReactAttribute = unbox (key, value)

// [<Erase>]
type attr() =
    static member inline className(value: string) = Interop.mkAttr "className" value
    /// Alias for inline `attr.children [ Html.content value ]`
    static member inline content(value: string) = attr.children [ unbox value ]
    /// Alias for inline `attr.children [ Html.content value ]`
    static member inline content(value: int) = attr.children [ unbox value ]
    /// Alias for inline `attr.children [ Html.content value ]`
    static member inline content(value: ReactElement) = attr.children [ value ]
    static member children (elems: ReactElement list) = Interop.mkAttr "children" (Array.ofList elems)

[<Erase>]
type Html =
    /// The `<div>` tag defines a division or a section in an HTML document
    static member inline div xs = Interop.reactElement "div" xs

let view =
    Html.div [
        attr.className "button"
        attr.content "Click me"
    ]

mountById "elmish-app" view

It generates 20 lines of JavaScript (+ 41 lines coming from keyValueList usage).

import { ofList } from "fable-library/Array.js";
import { type } from "fable-library/Reflection.js";
import { List, declare } from "fable-library/Types.js";
import { createObj } from "fable-library/Util.js";
import { Helpers$$$mountById as Helpers$0024$0024$0024mountById } from "fable-repl-lib/src/Fable.React.Helpers";
export function Interop$$$reactElement(name, props) {
  return React.createElement(name, createObj(props, 1));
}
export const attr = declare(function Fulma_Container_attr() {});
export function attr$reflection() {
  return type("Fulma.Container.attr");
}
export function attr$$$$002Ector() {
  return this instanceof attr ? attr.call(this) : new attr();
}
export function attr$$$children$$6E3A73D(elems) {
  return ["children", ofList(elems, Array)];
}
export const view = Interop$$$reactElement("div", [["className", "button"], attr$$$children$$6E3A73D(new List("Click me", new List()))]);
Helpers$0024$0024$0024mountById("elmish-app", view);

Live version

And if we try to "optimize"/adapt the attr class to be erased, we can reach 9 lines of generated JavaScript (+ 41 lines coming from keyValueList usage).

// Adapted type for using `inline children`
[<Erase>]
type attr =
    static member inline className(value: string) = Interop.mkAttr "className" value
    /// Alias for inline `attr.children [ Html.content value ]`
    static member inline content(value: string) = attr.children [ unbox value ]
    /// Alias for inline `attr.children [ Html.content value ]`
    static member inline content(value: int) = attr.children [ unbox value ]
    /// Alias for inline `attr.children [ Html.content value ]`
    static member inline content(value: ReactElement) = attr.children [ value ]
    static member inline children (elems: ReactElement list) = Interop.mkAttr "children" (Array.ofList elems)

Generated JavaScript

import { ofList } from "fable-library/Array.js";
import { List } from "fable-library/Types.js";
import { createObj } from "fable-library/Util.js";
import { Helpers$$$mountById as Helpers$0024$0024$0024mountById } from "fable-repl-lib/src/Fable.React.Helpers";
export function Interop$$$reactElement(name, props) {
  return React.createElement(name, createObj(props, 1));
}
export const view = Interop$$$reactElement("div", [["className", "button"], ["children", ofList(new List("Click me", new List()), Array)]]);
Helpers$0024$0024$0024mountById("elmish-app", view);

I added mention about (+ 41 lines coming from keyValueList usage) to try make a fair comparison. Please note that it's possible that the user already includes keyValueList in his project as it's used by others libraries or bindings.

@MangelMaxime
Copy link
Contributor Author

I forgot to say something in my last message.

I think that implementation proposition makes sense no matter if we choose attr or prop term :)

Also, to speak go back about the attr and prop term, in react bindings they mentioned both terms Attributes and Props. But when looking in the react documentation or in libraries/tutorials made by the community I found the property term being used more commonly. (I don't have the number I am just speaking by from memory here)

With that said, I will trust the decision you made or will make :)

@Zaid-Ajaj
Copy link
Owner

Thank you so much @MangelMaxime for checking the generated bundle sizes and the docs, I didn't know about children being a prop as well and I will definitely incorporate it before the stable release, or maybe as soon as I get access to my computer :)

I think that implementation proposition makes sense no matter if we choose attr or prop term :)

That's exactly what I think: it doesn't really matter 😄 maybe I could add a type alias called prop in this library and users can choose which to use but I still favor attr. The good news is that extending this type (whatever it is called) with more static functions is really easy for third party components.

There is also something I want to check: I think in F# you could open a static class the same you do for F# modules so it could be as simple as open Feliz.attr but I am not sure it works

@MangelMaxime
Copy link
Contributor Author

Thank you so much @MangelMaxime for checking the generated bundle sizes and the docs, I didn't know about children being a prop as well and I will definitely incorporate it before the stable release, or maybe as soon as I get access to my computer :)

Happy to share what I know and help improve this library :)

That's exactly what I think: it doesn't really matter smile maybe I could add a type alias called prop in this library and users can choose which to use but I still favor attr.

I am all for providing both in the library. Like that everyone can use what they prefer as long as we don't have 10 alias 😉

The good news is that extending this type (whatever it is called) with more static functions is really easy for third party components.

Indeed, and this can help people prototype their own typed CSS properties. Like using unit of measure, or specialized DSL for composing computation for SVG elements etc.

There is also something I want to check: I think in F# you could open a static class the same you do for F# modules so it could be as simple as open Feliz.attr but I am not sure it works

That actually reminds me of something but I do not think I have used it either

@MangelMaxime
Copy link
Contributor Author

After a quick search, I think that feature open static class is planned for F# 4.7 so we don't have it yet.

@Zaid-Ajaj
Copy link
Owner

Bundle size reduction implemented and simplified the interop story, however I kept the constructor of attr() to re-use the function styleList, otherwise inlining it would make the bundle size bigger if one used it multiple times.

As for attr vs. prop I am still convinced to keep attr without anything else because less options is more, we will see how users handle it ;)

@MangelMaxime
Copy link
Contributor Author

Bundle size reduction implemented and simplified the interop story, however I kept the constructor of attr() to re-use the function styleList, otherwise inlining it would make the bundle size bigger if one used it multiple times.

At minima, you can remove the attr() constructor which is needed neither.

You can also create a module attr with the function styleList and classList in it. This is common in F# to have a module and class with the same name to handle stuff like that.

Like that they are still accessible via attr.styleList or attr.classList and the bundle size is reduce to the minimum.

I mentioned classList because it's having more or less the same impact as styleList but has been inlined in the current version of Feliz.

@Zaid-Ajaj
Copy link
Owner

Done and worked nicely! fixed in v0.8.0

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

2 participants