Add More impls for UpdateEl ..

[MuhannadAlrusayni]

Hi, I would like to implement UpdateEl for missing types, such as:

impl<Ms, T> UpdateEl<El<Ms>> for Option<T>
where
    T: UpdateEl<El<Ms>> { .. }

impl<Ms, T, E> UpdateEl<El<Ms>> for Result<T, E>
where
    T: UpdateEl<El<Ms>>,
    E: UpdateEl<El<Ms>> { .. }

// this would replace Vec<Style>, Vec<&Style>, Vec<EventHandler<Ms>>, Vec<El<Ms>> ...etc
impl<Ms, T> UpdateEl<El<Ms>> for Vec<T>
where
    T: UpdateEl<El<Ms>> { .. }

[MartinKavik]

I'm not sure if stable Rust allows you to implement it for Vec<T> but you can try.
And write an example / test where you use Some & None and Ok and Err please so we know it works and doesn't break in the future.

[MuhannadAlrusayni]

I'll write PR soon, but what about other types .. Rc, Box ..etc. And do you think we miss any type that we should implement UpdateEl<..> for ?

[MartinKavik]

Impls are "realworld-driven" - i.e. once you have problem with it in your app, add another implementation.
I would like to add general support for Iterator but we have to wait for specialization in stable Rust.

[MuhannadAlrusayni]

#367

[David-OConnor]

How do you imagine the Option and Result ones would work? If Some/Ok, render, and if not don't? The key here is balancing convenience, with not allowing surprising or ambiguous results.

[MuhannadAlrusayni]

How do you imagine the Option and Result ones would work? If Some/Ok, render, and if not don't? The key here is balancing convenience, with not allowing surprising or ambiguous results.

Hmm, if I understand you correctly, you're saying that users might have surprising result when they pass Option or Result value to div[], right ? would you please provide some example of these surprising result, thanks.

[David-OConnor]

No - I'm saying that balance is a good rule when adding more UpdateEl impls in general. Is what I described how you imagine Options/Result impls to work? If so, I like it.

[MartinKavik]

@MuhannadAlrusayni As I said - Impls are "realworld-driven" - so why you need those implementations?
We want to have API and code base as small as possible, so there are fewer bugs, fewer breaking changes, fewer outdated docs, people use fewer approaches, it's faster, smaller, etc. So we want to know your reasons for it and eventually try to find better solution.

[MuhannadAlrusayni]

Some time you have some sort of data inside Option<..> and you want to convert that to Node<..>:

pub fn get_weather_state() -> Option<WeatherState> { .. }

// in current Seed
div![
    get_weather_state()
        .map(|state| todo!())
        .unwrap_or(empty![]),
    // other div content ..
]

// after this PR
div![
    get_weather_state().map(|state| todo!()),
    // other div content ..
]

same applies to Result<..>:

pub fn get_online_users() -> Result<Vec<User>, Error> { .. }

// in current Seed
div![
    // here we unwrap the inner value and convert it to Node<..>
    match get_online_users() {
        Ok(users) => users_to_node(),
        Err(err) => err_to_node(),
    }
]

// after this PR
div![
    // here we only covert the inner value without unwrapping it
    get_online_users()
        .map(|users| users_to_node())
        .map_err(|err| err_to_node())
]

I tend to store Option<Node<..>> in elements, and later in the view/render function I try to view those stored Option<Node<..>>:

struct Page {
    news: Option<Node<Msg>>,
    // ..
}

impl Page {
    pub fn view(&self) -> Node<Msg> {
        // in current Seed
        div![
            self.news.unwrap_or(empty![]),
            // other page content ..
        ]

        // after this PR
        div![
            self.news,
            // other page content ..
        ]
    }
}

Here is a use case in Seed that we can use this PR with, for example we can return Option<Node<Msg>> in this view function:

seed/examples/server_integration/client/src/example_a.rs

Lines 85 to 94 in 5e61a6e

	fn view_message(message: &Option<shared::SendMessageResponseBody>) -> Node<Msg> {
	let message = match message {
	Some(message) => message,
	None => return empty![],
	};
	div![format!(
	r#"{}. message: "{}""#,
	message.ordinal_number, message.text
	)]
	}

Please use simple English when possible, since it's not my language :D, thanks for your patient.

[MartinKavik]

1st example - Option

Well.. I like // in current Seed version more because it's more explicit - I won't be surprised when I don't see weather element on the page.

2nd example - Result

I like combinators very much but match is often better because it works nicely with lifetimes and you can return early or use ?.

3rd example - Option<Node<..>>

I tend to store Option<Node<..>> in elements

Why? ; And it sounds like an edge-case.

4th example - server integration
It's relatively old code, I would write it as:

fn view_message(message: &Option<shared::SendMessageResponseBody>) -> Option<Node<Msg>> {
    message.as_ref().map(|message| {
        div![format!(
            r#"{}. message: "{}""#,
            message.ordinal_number, message.text
        )]
    })
}
...
view_message(&model.response_data).unwrap_or(empty![]),

That way you know the message can be blank and that it's an element.

[MuhannadAlrusayni]

Why? ; And it sounds like an edge-case.

I do store Node<..> in flexbox::Item element since there is no way to store different element types in the same Vec.

It's not always Node<..>, some times I store types that can be converted to Node<..>, I do so in Button element.

I like combinators very much but match is often better because it works nicely with lifetimes and you can return early or use ?.

We still can use match.

---

I believe that in most cases, we use:

val.unwrap_or(empty![])

So, I don't think doing this implicitly would make any surprising result.

Simply you pass None, nothing happens to the El, you pass Some(inner), the inner will update the El.

[MartinKavik]

I was thinking about it and I'll maybe vote for merging (after code review) - reasoning:
We plan to implement UpdateEl for all Iterators once specialization is stabilized. It's also possible we'll implement IntoIterator to cover also Vec and slices at once. Both Result and Option implement IntoIterator so the result will be the same like with this PR. Also the argument that we want to force users to write more explicit code isn't very strong and it can backfire - we will see code like div![view_foo.iter()] in the wild where view_foo returns Option, Result or Vec even if we implemented UpdateEl only for Iterator. Another easy "workaround" is just return Node instead of Option<Node> - like in official example.

but what about other types .. Rc, Box ..etc. And do you think we miss any type that we should implement UpdateEl<..> for ?

So as the result we can define/narrow UpdateEl scope to basic things like String and things that implement IntoIterator.

Opinions?

[TatriX]

Explicit is better than implicit.

Sure, it's more typing but I think that it's up to a user to decide what None means.
It can be empty![] or it can be placeholder or anything else, like "No results" message.
I would say, let's wait and see before making this decision. Maybe introduce helpers for empty![] case.

Personally I like to see explicit conditional logic in my views.