Seed: 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]

https://github.com/seed-rs/seed/pull/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:
https://github.com/seed-rs/seed/blob/5e61a6e3d877e5bc94bc9a6df85f54c8f6e3ce26/examples/server_integration/client/src/example_a.rs#L85-L94

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

[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.

[MartinKavik]

@TatriX What do you think about implementing UpdateEl for Iterator or IntoInterator (once spec. is ready)?

[TatriX]

I don't really know enough about it to say anything reasonable.
Can you give some guide-level examples of how it can be used?

[MartinKavik]

@TatriX
UpdateEl trait allows us to throw different things into element macros e.g.:

div![
    iAmVectorOfNodes,
    iAmNode,
    iAmVectorOfSomething.iter().map(view_something),
    iAmEventHandler,
    "iAmStr",
]

We want to implement UpdateEl for Iterator (https://github.com/seed-rs/seed/issues/128) because it'll allow us to use all iterators without verbose .collect::<Vec<_>>().
Seed supports now:

• Map
• FilterMap
• Filter
• FlatMap

and it's pain to write impls for them.

With IntoIterator we would be able to easily handle also already implemented Vec and other collections and slices at once. The good or bad side effect is supporting things like Option and Result because they implement IntoIterator too.

---

So that was an introduction into the issue and it led me to my reasoning in https://github.com/seed-rs/seed/issues/365#issuecomment-590710035.
If it's not clear, just write me on chat and I'll edit this comment eventually.

[MartinKavik]

Maybe introduce helpers for empty![] case.
Personally I like to see explicit conditional logic in my views.

One native option when Option implements UpdateEl (resp. IntoIterators impls UpdateEl) is:

div![
    disabled.not().then(|| ev(Ev::Click, |_| Msg::Click),
    (!disabled).then_some("I am title")
]

[TatriX]

Can something like this do the job?

impl UpdateEl for IntoUpdateEl {
  ...
}

/// Marker trait
trait IntoUpdateEl: IntoIterator {}

impl IntoUpdateEl for Vec {}

Then we can control which iterators can be used. The disadvantage is that you can't implement the trait for foreign collections I guess.

[MartinKavik]

Then we can control which iterators can be used.

"White-list" would be pain to manage (iterators from Rust std, from itertools, etc.) and we would be fighting over our "favorite" iterators. Black-list would make more sense (if it's possible to create it) - we would add iterators which will prove problematic in real-world apps.

So.. to move forward - I suggest to allow implementing UpdateEl for entities which implement IntoIterator. We'll see how users are using it and then we can decide to make it more strict if it turns code-bases into unreadable mess - it would be breaking change, but easily fixed and supported by evidence.
Counter-arguments?