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

Alternative fluent syntax #54

Closed
cmeeren opened this issue Oct 9, 2019 · 13 comments
Closed

Alternative fluent syntax #54

cmeeren opened this issue Oct 9, 2019 · 13 comments

Comments

@cmeeren
Copy link
Contributor

cmeeren commented Oct 9, 2019

How do you feel about a syntax like this? (I drafted this quickly; there may be lots of room for improvement - for example, there may be a way to eliminate the .reactElement at the end of each component when more careful thought is given to this.)

AppBar()
  .position.absolute
  .children([
    Typography()
      .variant.h6
      .color.primary
      .text("Foo")
      .reactElement
  ])
  .reactElement

Benefits: Supports inheritance, and puts a final end to any discoverability issues still left in Feliz: No more hunting for the correct prop type; you're just dotting through and seeing what's available. Also supports overloaded props and enum props just like Feliz; we have basically just replaced property lists with "fluent builders".

Drawbacks: I don't know which impacts this has for bundle size or performance (see quick and dirty implementation below). Also it's "unusual" as far as Fable goes (no prop lists). Not that the latter matters by itself. Update: See more challenges here: #54 (comment)

I'm not saying we should necessarily do this; I just wanted to get this out of my head and post it for discussion.

For the record, below is the implementation of the above syntax. I have not made any attempts to inline or erase stuff. And there's a bit of type trickery which would likely cause OO fanatics to spin in their graves (what with subtypes inheriting base types parametrized by the subtype and all).

open Fable.Core.JsInterop
open Fable.React

let reactElement (el: ReactElementType) (props: 'a) : ReactElement =
  import "createElement" "react"


[<AbstractClass>]
type PropsBase() =
  abstract ElementType : ReactElementType
  member internal __.Props = ResizeArray<string * obj>()
  member internal this.reactElement =
    reactElement this.ElementType (createObj this.Props)


[<AbstractClass>]
type PropsBase<'a>() =
  inherit PropsBase()

  member this.custom (key: string) (value: obj) =
    this.Props.Add(key, value)
    this |> unbox<'a>

  member this.children (elems: seq<ReactElement>) =
    // should also call React.Children.ToArray
    this.custom "children" elems

  member this.text (text: string) =
    this.custom "children" text


type AppBarPosition<'a when 'a :> PropsBase<'a>>(parent: 'a) =
  member __.absolute = parent.custom "position" "absolute"
  member __.relative = parent.custom "position" "relative"


type AppBar() =
  inherit PropsBase<AppBar> ()
  member this.position = AppBarPosition(this)
  override __.ElementType = importDefault "@material-ui/core/AppBar"


type TypographyVariant<'a when 'a :> PropsBase<'a>>(parent: 'a) =
  member __.h5 = parent.custom "variant" "h5"
  member __.h6 = parent.custom "variant" "h6"

type TypographyColor<'a when 'a :> PropsBase<'a>>(parent: 'a) =
  member __.primary = parent.custom "color" "primary"
  member __.secondary = parent.custom "color" "secondary"


type Typography() =
  inherit PropsBase<Typography> ()
  member this.variant = TypographyVariant(this)
  member this.color = TypographyColor(this)
  override __.ElementType = importDefault "@material-ui/core/Typography"
@Zaid-Ajaj
Copy link
Owner

If the .reactElement would go away, then the syntax would be OK-ish on it's own but it would look different when used with other third-party bindings but I think users can at least live with it:

Html.div [
  Mui.appBar()
    .position.absolute
    .children([
      Mui.typography()
        .variant.h6
        .color.primary
        .text("Foo")
    ])
]

But I still think that duplicating the base properties would the easiest to do. I don't think you need to duplicate every single property because a lot of the times you won't use them, just duplicate the ones that are most likely to be used: id, key, className, style, children and falling back to prop for the very very rare cases. What do you think?

@cmeeren
Copy link
Contributor Author

cmeeren commented Oct 10, 2019

Continuing discussion of inheritance in Shmew/Feliz.MaterialUI#20 since this issue is only about the alternative syntax, which is a consideration in its own right separate from my struggles with inheritance (sorry if that wasn't clear).

@albertwoo
Copy link

I think the fluent syntax has another drawback, for example if i want to create an extensible control, i can use yield! to append new props. Or I can create union cases for limited props and do reduce to get the base props and yield it at anywhere I want. But with fluent syntax i can only append props in the end.

let myButton props =
    Html.button [
        prop.Classes [ ... ]
        prop.Style [....]
        yield! props
    ]

@cmeeren
Copy link
Contributor Author

cmeeren commented Oct 11, 2019

@albertwoo I'm not enturely sure what you mean by

Or I can create union cases for limited props and do reduce to get the base props and yield it at anywhere I want.

But there's no problem adding support for arbitrary props using the fluent syntax:

let myButton props =
  Html.button
    .classes([...])
    .style([...])
    .custom(props)

where custom can have overloads accepting anything we want, really:

  • string * obj
  • The Feliz prop type (for interop)
  • The Fable.React prop type (for interop)
  • Sequences of any of the above

@cmeeren
Copy link
Contributor Author

cmeeren commented Oct 11, 2019

In any case, for extensible components with the fluent syntax, you probably wouldn't pass props like that. You'd do something like this:

let myButton () =
  Html.button
    .classes([...])
    .style([...])

And then use it like this:

myButton()
  .text("foo")

@albertwoo
Copy link

I kind of like the props list style. In my code I did something like:

type ISimpleFieldProp = interface end

[<RequireQualifiedAccess>]
type SimpleFieldProp =
  | Label of string
  | Errors of string list 
  | OuterClasses of string list
  | LabelClasses of string list
  | ErrorClasses of string list
  | FieldView of ReactElement
  | OuterAttrs of IHTMLProp list
  interface ISimpleFieldProp

let simpleField (props: ISimpleFieldProp list) =
    let props = props |> List.choose (function :? SimpleFieldProp as x -> Some x | _ -> None)
    let label             = props |> UnionProps.tryLast (function SimpleFieldProp.Label x -> Some x | _ -> None)
    let errorClasses      = props |> UnionProps.tryLast (function SimpleFieldProp.ErrorClasses x -> Some x | _ -> None) |> Option.defaultValue []
    let outerClasses      = props |> UnionProps.concat (function SimpleFieldProp.OuterClasses x -> Some x | _ -> None)
    let labelClasses      = props |> UnionProps.concat (function SimpleFieldProp.LabelClasses x -> Some x | _ -> None)

    div </> [
      yield! props |> UnionProps.concat (function SimpleFieldProp.OuterAttrs x -> Some x | _ -> None)
      match outerClasses with
        | [] -> Style [ Margin "5px"; Padding "5px" ]
        | cs -> Classes cs
      Children [
        if label.IsSome then fieldLabel labelClasses label.Value 
    
        match props > UnionProps.tryLast (function SimpleFieldProp.FieldView x -> Some x | _ -> None) with
          | Some x -> x
          | None    -> ()

        yield! 
          props 
          |> UnionProps.concat (function SimpleFieldProp.Errors x -> Some x | _ -> None)
          |> fieldError errorClasses
      ]
    ]

With UnionProps.concat I can merge all the things like IHTMLProp, Class string, Style etc and feed to Fable.React. Then I can use it like:

let simpleField1 props =
   simpleField [
      SimpleFieldProp.OuterClasses [ ... ]
   ]
let simpleField2 props =
   simpleField1 [
      SimpleFieldProp.OuterClasses [ ... ]
   ]

and all the OuterClasses can merge together. Of course if merge together is not I want I can just use UnionProps.tryLast.

@cmeeren
Copy link
Contributor Author

cmeeren commented Oct 11, 2019

Thank you for the detailed example. I must admit that it has me confused. For example, you don't seem to be using Feliz at all, but normal Fable.React. So I'm having a hard time understanding how it's relevant to this discussion.

Also, merging prop values for the same props from several "inherited"/composed classes seems to me like an unnecessarily messy way to do extensibility given that in React/JSX based syntax (AFAIK) a component can have only one occurrence of any given prop.

It seems like what you are trying to do, is to defining a custom component that supports a handful of optional, custom props, some of which are props and some of which are child elements.

In Feliz (as it is now, without any fluent syntax), just off the top of my head, I might solve it like this (partial implementation; hopefully the rest of the props should be clear):

open Feliz

type Html with

  member simpleField(
      ?label: string,
      ?errors: string list,
      ?labelClasses: string list,
      ?outerClasses: string list
      ?props: IReactProperty list) =
    // Unsure if this is correct, since I don't know what UnionProps.concat actually
    // does, but I'm sure you get the gist
    let lbClasses = labelClasses |> Option.defaultValue []

    Html.div [
      yield! props |> Option.defaultValue []
      outerClasses |> Option.map prop.classes |> Option.defaultValue (prop.style [ ... ])
      prop.children [
        match label with Some lb -> fieldLabel lbClasses lb | None -> ()
        ...
      ]
    ]

This also allows you to easily make any of the props required, if you desire.

As regards the fluent syntax under discussion, it would work the same way.

The usage of the example above would be:

Html.simpleField(
  label = "foo",
  outerClasses = ["bar"]
)

Note also that creating custom React components (e.g. function components with Fable.React's FunctionComponent.Of) can be a great way to do reusability.

@albertwoo
Copy link

Yes, I am not using Feliz (I tried but there is no SSR support yet ). But I just want to explain my thoughts about prop list style.
Yes for merging props like class and style together would be a little mess, but it just make things simpler for me to write my app.
Your above example is very like the Fabulous Xamarin default way (I also use that project 😀). Sometimes it is not easy to extend simpleField. For example if I want to create a simpleField2 to be reused then i have to repeat the input parameters like:

type Html with
   member simpleField2(
      ?label: string,
      ?errors: string list,
      ?labelClasses: string list,
      ?outerClasses: string list
      ?props: IReactProperty list) =
     simpleField(label, errors, labelClasses, [ "bar"; yield! outerClasses ], props)

Appreciate you guys works!!!

@cmeeren
Copy link
Contributor Author

cmeeren commented Oct 11, 2019

:)

Possible workaround:

type SimpleFieldProps = {
  Label: string option
  Errors: String list
  // ...
} with
  static member Create(?label: string, ?errors: string list, .. ) = { .. }

type Html with
  member simpleField (props: SimpleFieldProps) = // ...
  member simpleField2 (props: SimpleFieldProps) = // ...

Usage:

Html.simpleField (SimpleFieldProps.create(..))

@albertwoo
Copy link

albertwoo commented Oct 11, 2019

:) Thanks for the advise. This way can help but if I have a lot of props like in Fabulous Xamarin, it would be too much work. That is also why I also use Fabulous.SimpleElements (Thanks @Zaid-Ajaj again :))

@cmeeren
Copy link
Contributor Author

cmeeren commented Oct 14, 2019

Regarding the .reactElement: AFAIK it's not really a problem, because we can make sure it's only required to use it when interoping with existing code that requires reactElement. Otherwise, properties that accept ReactElement in the fluent API (e.g. children) can also have overloads accepting PropsBase, internally calling .reactElement.

However, a drawback then is that since lists can only contain one type, and there's no automatic upcasting inside a list, then we need an explicit upcast (e.g. to PropsBase) anyway if we have more than one element. Unless there's a good solution to that, we might just as well keep reactElement.

A possible workaround to the above is to not have lists of child elements, but use [<ParamArray>] of PropsBase. Then I think the parameters will be automatically upcasted. But then you need commas between each item. (You also replace [] with (), but I don't really care about that.)

There also needs to be an API for conditional props. In the current Feliz (and Fable.React) DSL we can selectively yield props, but with chained prop calls as proposed here, that's not possible. This is a matter of API design, but I'm not convinced it'll be pretty, e.g. for enum props where there are no parameters - then we can't add a bool parameter, which might be a solution for methods.

We could have a wrapping .conditional(bool, 'builder -> 'ignored), e.g.

.prop1("foo")
.conditional(false, fun p -> p.prop2("bar"))
.prop3("baz")

But it's not the prettiest API I have seen. (Not too bad, but the current conditional yield is nicer IMHO.)

@Zaid-Ajaj
Copy link
Owner

If I was writing the DSL in C#, this is the syntax I would have wanted to write for sure but since we have F# and all of it's nice list syntax, I very much rather use the current one instead of this

@cmeeren
Copy link
Contributor Author

cmeeren commented Oct 14, 2019

Gotcha. The only thing I really like about this syntax is that it's the most discoverable I have seen so far. Otherwise it seems to have too many drawbacks compared to the current one.

@cmeeren cmeeren closed this as completed Oct 14, 2019
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