Pauan / rust-dominator

Zero-cost ultra-high-performance declarative DOM library using FRP signals for Rust!
MIT License
999 stars 63 forks source link

Ideas for DomBuilder::children() API #27

Closed njam closed 4 years ago

njam commented 4 years ago

Two ideas for DomBuilder::children():

  1. How about passing children elements as-owned instead of by-reference? There was a comment in the code indicating that you wanted to do that. It means we need to usually create a Vector instead of an array of Dom elements, because IntoIterator is not implemented for arrays.
  2. How about accepting not only Dom, but more generally Into<Dom>, and then implementing the corresponding conversion for DomBuilder?

Change 1) would make it easier to make a "children" list of Dom elements from any iterator/collection. I'm often collect()ing to Vec, and then converting to a slice with as_mut_slice().

Change 2) could be useful because it will allow to create Dom elements more easily without macros. For example:

.children(vec![
    html("div")
        .text("Hello World"),
    html("button")
        .text("Increase")
        .event({
            let state = state.clone();
            move |_: events::Click| {
                state.counter.replace_with(|x| *x + 1);
            }
        }),
])

Where html() is a function to create a DomBuilder like this:

fn html(name: &str) -> DomBuilder<web_sys::HtmlElement> {
    DomBuilder::new_html(name)
}

Of course I'm just opening this to get your opinion on these ideas, maybe there are good reasons why it's not that way?

rrmckinley commented 4 years ago

@Pauan did you have thoughts on this PR? I'm curious, but I don't have an opinion myself yet, thanks

Pauan commented 4 years ago

@njam Unfortunately it cannot accept Dom by value, because of the lack of IntoIterator (as you noted). It must accept arrays, because arrays do not allocate (but Vec does). Requiring allocation is unacceptable for performance.

However, I just realized that it can accept BorrowMut<Dom>, which would allow it to work with IntoIterator<Item = Dom> and also IntoIterator<Item = &mut Dom>. That means you would be able to do this:

html!("div", {
    .children(foo.iter().map(|foo| {
        html!("div", {
            // ...
        })
    }))
})

Now it doesn't allocate at all, no more Vec needed!

However, Into<Dom> still won't work, because of type inference.

Change 2) could be useful because it will allow to create Dom elements more easily without macros.

The best you can do right now is this:

.children(&mut [
    html("div")
        .text("Hello World")
        .into_dom(),
    html("button")
        .text("Increase")
        .event({
            let state = state.clone();
            move |_: events::Click| {
                state.counter.replace_with(|x| *x + 1);
            }
        })
        .into_dom(),
])

This also doesn't allocate, so it's faster than using a Vec.

Pauan commented 4 years ago

I've released version 0.5.9 which adds BorrowMut, so it's now possible to use iterators with children without allocating a Vec.

Pauan commented 4 years ago

P.S. Unrelated to this PR, but it's nicer to use clone!, like this:

.event(clone!(state => move |_: events::Click| {
    state.counter.replace_with(|x| *x + 1);
}))
njam commented 4 years ago

It must accept arrays, because arrays do not allocate (but Vec does).

Makes sense.

I've released version 0.5.9 which adds BorrowMut, so it's now possible to use iterators with children without allocating a Vec.

Very nice, works great. Thanks!