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

Ideas for DomBuilder::children() API #27

Closed
wants to merge 2 commits into from

Conversation

njam
Copy link

@njam njam commented Jan 26, 2020

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
Copy link

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

@Pauan
Copy link
Owner

Pauan commented Apr 27, 2020

@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
Copy link
Owner

Pauan commented Apr 27, 2020

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 Pauan closed this Apr 27, 2020
@Pauan
Copy link
Owner

Pauan commented Apr 27, 2020

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
Copy link
Author

njam commented Apr 28, 2020

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!

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

Successfully merging this pull request may close these issues.

3 participants