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
Your opinion on v2 idea? #334
Comments
Adding this kind of support will make the type inference a lot more permissive (I would even say useless) because then we will have Also, in order to make that possible, we will need to add an extra layer on top of React to split the attributes from the children "manually" which will impact the performance as we will always have an extra steps needed. This will also increase the bundle size in order to store the information needed to make the sepration. Personally, I am not for this feature. |
I have mixed feelings about this approach, mainly because of the runtime overhead it adds to rendering. Also I would prefer to keep the idea of a In my experience, I hear about this issue from users a lot but only in the context of div [ "myClass" ] [
Html.div "nested"
] |
Seems to me there's no reason not write your own extension methods if they better suit the effect of applying classes or other frequently used properties as I did here with Material UI. Composability is dead easy with F# without further confusing the compiler or reducing type safety.
|
Thanks for all opinions, I just wanted to discussion flow. 👍 I have also my own helpers like |
This should actually be possible while keeping type restrictions if we changed React elements from being a list to being computation expressions: div {
className "myClass"
div { ... }
} The CE implementation would increase the size of the codebase, definitely make it harder to maintain, and handling some things like primitives when props are defined is a little tricky/would have to fail on compile rather than during development like it does now: // Would probably need to make the base builder generic so we can use a type restriction and define this DU for
// each element so we can properly restrict what types are valid as primatives
type ReactBuilderChild =
| Child of ReactElement
| Children of ReactElement list
| Float of float
| Int of int
| String of string
| None
member this.GetChildren () =
match this with
| ReactBuilderChild.Children children -> children
| _ -> failwith "Cannot combine children with primitive values."
type ReactBuilderState =
{ Children: ReactElement list
Props: IReactProperty list }
[<AbstractClass;NoEquality;NoComparison>]
type ReactBuilder () =
[<EditorBrowsable(EditorBrowsableState.Never)>]
member _.Yield (x: ReactElement) = { Children = ReactBuilderChild.Child x; Props = [] }
[<EditorBrowsable(EditorBrowsableState.Never)>]
member _.Yield (x: ReactElement list) = { Children = ReactBuilderChild.Children x; Props = [] }
[<EditorBrowsable(EditorBrowsableState.Never)>]
member _.Yield (x: float) = { Children = ReactBuilderChild.Float x; Props = [] }
...
[<EditorBrowsable(EditorBrowsableState.Never)>]
member _.Yield (x: unit) = { Children = ReactBuilderChild.None; Props = [] }
[<EditorBrowsable(EditorBrowsableState.Never)>]
member _.Combine (state: ReactBuilderState, x: ReactBuilderState) =
{ Children = (state.Children.GetChildren()) @ (x.Children.GetChildren()); Props = state.Props @ x.Props }
[<EditorBrowsable(EditorBrowsableState.Never)>]
member _.Zero () = { Children = ReactBuilderChild.None; Props = [] }
[<EditorBrowsable(EditorBrowsableState.Never)>]
member this.For (state: ReactBuilderState, f: unit -> ReactBuilderState) = this.Combine(state, f())
[<CustomOperation("className")>]
member _.className (state: ReactBuilderState, value: string) =
{ state with Props = (Interop.mkAttr "className" value)::state.Props }
[<CustomOperation("children")>]
member _.children (state: ReactBuilderState, value: ReactElement list) =
{ state with Children = state.Children @ value }
abstract Run : ReactBuilderState -> ReactElement
[<NoEquality;NoComparison>]
type DivBuilder () =
inherit ReactBuilder()
[<EditorBrowsable(EditorBrowsableState.Never)>]
member _.Run (state: ReactBuilderState) =
match state with
| { Children = ReactBuilderChild.None; Props = props } -> Interop.createElement "div" props
| { Children = ReactBuilderChild.Children children } -> Interop.reactElementWithChildren "div" children
| { Children = ReactBuilderChild.Float f } -> Interop.reactElementWithChild "div" f
| _ -> ...
let div = DivBuilder() What I'm unsure about, is how to make Fable properly compile away any/all of the CE plumbing, as well as making the operations discoverable (like it is currently with the |
Petition to change the title to indicate the discussion topic? 😆 @Dzoukr There have been previous discussions on this -- I am definitely on your side. Helper functions are not ideal because they bring back the awkward double list syntax, basically eliminating one of the key benefits of this lib in the first place, and they don't help with real prop usage which is much more than just div+class. @Zaid-Ajaj Would you be open to an alternate namespace to explore this area? Like Feliz.Experimental or something? So main Feliz namespace keeps goal of thin layer over React while alternate can try more opinionated approach. Anyway, I like something like this to maintain prop grouping:
Prop nesting seems cleaner than element nesting since props are not subject to infinite nesting the way that elements are. The Performance would be something to keep an eye on but I don't think the impact is known without trying. Who knows, maybe the extra work only sounds scary but turns out insignificant in practice relative to everything else happening on render. |
@zanaptak I quite like that idea because as you say, the props don't tend to nest deeply like the children. On the topic of helpers and the double list syntax
let inline withProps elementFunction props (children: #seq<ReactElement>) =
(prop.children (children :> ReactElement seq))
:: props
|> elementFunction
let myDiv = withProps Html.div
myDiv [ prop.className "myClass" ] [
Html.div "Nested"
] You could even define an operator to use the "double list"-like syntax with any element if you wanted: let inline (@+) elementFunction props =
fun children -> withProps elementFunction props children
let inline (@++) elementFunction prop =
fun children -> withProps elementFunction [ prop ] children
Html.div @+ [ prop.className "myClass" ]
<| [ Html.div "nested" ]
Html.div @++ prop.className "myClass"
<| [ Html.div "nested" ] |
This is a tricky topic because what "looks better" can be subjective. Probably the most important thing is to avoid breaking changes, but if you just want to experiment you can use Feliz.Engine for that, as it's very easy to adapt it to React. As mentioned in other issues, I don't want to publish Feliz.Engine.React because it would be confusing to have two similar "competing" packages. But for example, if you want to have everything on the same level you just need a module like this (it would be easy to adapt it to having props in a separate list too): // Dependencies:
// - Fable.React
// - Feliz.Engine.Event dependencies
open Fable.Core
open Fable.Core.JsInterop
open Feliz
open Fable.React
type Node =
| El of ReactElement
| Style of string * string
| Prop of string * obj
member this.AsReactEl =
match this with
| El el -> el
| _ -> failwith "not an element"
let private makeNode tag nodes =
let props = obj()
let style = obj()
let children = ResizeArray()
nodes |> Seq.iter (function
| El el -> children.Add(el)
| Style(k, v) -> style?(k) <- v
| Prop(k, v) -> props?(k) <- v
)
if JS.Constructors.Object.keys(style).Count > 0 then
props?style <- style
ReactBindings.React.createElement(tag, props, children) |> El
let Html = HtmlEngine(makeNode, str >> El, fun () -> El nothing)
let Svg = SvgEngine(makeNode, str >> El, fun () -> El nothing)
let prop = AttrEngine((fun k v -> Prop(k, v)), (fun k v -> Prop(k, v)))
let style = CssEngine(fun k v -> Style(k, v))
let ev = EventEngine(fun k f -> Prop("on" + k.[0].ToString().ToUpper() + k.[1..], f)) Test: let myEl = // Node
Html.div [
prop.className "myClass"
Html.div "Nested"
]
let myElAsReactEl = myEl.AsReactEl // ReactElement |
@zanaptak Helper functions don't need to take two lists as input, Todo.List [ for item in items -> Todo.Item item ] Although you can technically infinitely nest the lists, that's not the idea. There is a balance there by using smaller functions or components in your bigger UI code. However, ideally you wouldn't need to break down the code as often and the one function should be as readable as it can be because thinking of organising things takes away the flow sometimes. This was the problem with the DSL of the Fable.React where you had to think about how to format things every time. Feliz fixes that at the cost of two levels of nesting of To be honest, I don't know what the silver bullet is. Like @alfonsogarciacaro said
On the topic of experiments:
At this point in time, no plans on maintaining different DSLs in the Feliz project. I am very limited on OSS time/capacity and would hope to direct efforts into improving the current approach: documentation, samples and more well tested ecosystem libraries. That said, feel free to experiment with Feliz.Engine by @alfonsogarciacaro and use the DSL that suit you better. |
Hello friend,
as a daily user of Feliz (and Feliz.Bulma indeed) I am always struggling with nesting
prop.children
which makes it somehow uglier for me. I'd like to know your opinion on v2 proposal where we would have:Maybe something like that is not even possible, but I saw @alfonsogarciacaro's work on Feliz.Engine and it seems so tempting to have props and children on the same level. 😄
The text was updated successfully, but these errors were encountered: