leptos_dom v2

[jquesada2016]

This pull request addresses #116 .

[gbj]

This all seems reasonable to me, API-wise. One concern I have is that the distinction between dyn_text and dyn_child would require the view! macro to know at compile time whether a particular block is going to return a text node or a child:

view! {  cx, <p>"Value: " {some_function()} "!"</p> }

Translates into dyn_text or dyn_child? I guess if Fn() -> String implements IntoNode it can be dyn_child, but then I don't know what dyn_text is for.

There are a few other small optimizations I can see (tag names can probably be &'static str instead of String, attributes probably don't need to be a HashMap; since there are so few, Vec<(&'static str, String)> is probably more efficient, etc.)

But yeah, looks good for an API and keep on going!

[jquesada2016]

One concern I have is that the distinction between dyn_text and dyn_child would require the view! macro to know at compile time whether a particular block is going to return a text node or a child:

Yeah! This is for people who wanted to use the builder API without the macro. For the macro, it would only be using child and dyn_child.

(tag names can probably be &'static str instead of String

This was my initial instinct too, 99% of nodes don't need to be String, but there's that 1% that does, especially for custom components which are not known at compile time, such as loading from a database. Perhaps using Cow<str might be an option.

attributes probably don't need to be a HashMap; since there are so few, Vec<(&'static str, String)> is probably more efficient

I totally agree on Vec<_> over HashMap<_>, but the same applies as before. I know with the macro it isn't possible to add attributes at runtime, but with the builder API, it would be, which can sometimes come in handy, perhaps another use of Cow<str>?

There are some questions, for example, when creating elements, should the context be collected at the begining:

let div = div(cx);

Or ar the end?

let node = div().text("Hello").into_node(cx);

Should IntoNode::into_node even take a Scope? I think in all cases but maybe one, it wasn't needed.

I know for the macro this is a non issue, but for a builder, what would happen in this case:

Let's say I have a function that returns an HtmlElement<Button>,
if I collect the scope when it's built, and I return it, perhaps the scope will be discarded before the user calls into_node.

This can happen too if we just collect the Scope at the end of the builder chain, but I might be less likely.

[gbj]

Yeah! This is for people who wanted to use the builder API without the macro. For the macro, it would only be using child and dyn_child.

Right, makes sense.

This was my initial instinct too, 99% of nodes don't need to be String, but there's that 1% that does, especially for custom components which are not known at compile time, such as loading from a database. Perhaps using Cow<str might be an option.

Also makes sense as a use case for the builder, and probably not a meaningful de-optimization: the cost of strings is in copying them across to JS-land, not in the allocation for String.

I totally agree on Vec<_> over HashMap<_>, but the same applies as before. I know with the macro it isn't possible to add attributes at runtime, but with the builder API, it would be, which can sometimes come in handy, perhaps another use of Cow<str>?

Well if you have mutable access you can push into a Vec<_> just as easily as inserting into a HashMap<_>. I just meant that for a very small n like the attributes on a DOM node, an O(n) search through a Vec<_> is going to be faster than the O(1) access of a HashMap. Again, very small difference, but maybe a small bundle size + speed improvement. The linear_map crate does this but we can just do it manually too.

There are some questions, for example, when creating elements, should the context be collected at the begining:

let div = div(cx);

Or ar the end?

let node = div().text("Hello").into_node(cx);

Depends on when the dyn_ functions actually do their work. They'll need to call create_effect at some point, so they need the cx at that point. If they do it when you call dyn_child then you need to provide cx at the top. If all this is set up at into_node then I agree, it should be into_node(cx) for the reason you gave about possibly disposing the scope.