yewstack / yew

Rust / Wasm framework for creating reliable and efficient web applications
https://yew.rs
Apache License 2.0
30.52k stars 1.42k forks source link

Make Option<Html<...>> renderable #257

Closed DenSA-Inc closed 4 years ago

DenSA-Inc commented 6 years ago

Allow Option<Html<CTX, Self>> in the html!-macro

I'm submitting a ... feature request

I'm sure some people will ask for this at some point, myself included. Imagine you have a component that only renders certain elements when a condition is met. It might look like this:

    fn view(&self) -> Html<CTX, Self> {
        if condition {
            html! {
                <nav class="menu",>
                    <button onclick=|_| Msg::Increment,>{ "Increment" }</button>
                    <button onclick=|_| Msg::Decrement,>{ "Decrement" }</button>
                </nav>
            }
        } else {
            html! {
                <nav class="menu",>
                    <button onclick=|_| Msg::Increment,>{ "Increment" }</button>
                    <button onclick=|_| Msg::Decrement,>{ "Decrement" }</button>
                    <div class="secret",>...</div>
                </nav>
            }
        }
    }

Sure, many things can be done with css- and html-attributes instead too, but at some points they won't be enough anymore. And, if you're rendering other components, you might want to have your children really removed instead of telling them to hide.

With Option<Html<...>> as renderable one would have instead:

    fn view(&self) -> Html<CTX, Self> {
            html! {
                <nav class="menu",>
                    <button onclick=|_| Msg::Increment,>{ "Increment" }</button>
                    <button onclick=|_| Msg::Decrement,>{ "Decrement" }</button>
                    { self.secretDiv() }
                </nav>
            }
        }
    }

    fn secretDiv(&self) -> Option<Html<CTX, Self>> {
        if someCondition {
            Some(html! {
                <div class="secretDiv",>...</div>
            });
        } else {
           None
        }
    }

You can see what I'm getting at. None will not be rendered at all and Some will have its boxed value rendered.
I'll just leave this here and wait what other people have to say about this.

Context (Environment)

hgzimmerman commented 6 years ago

I came up with a hack to allow if expressions to return Html by just providing an empty node in the else block.

pub fn empty_vdom_node<CTX, CMP>() -> Html<CTX, CMP>
    where
        CTX: 'static,
        CMP: Component<CTX>
{
    VNode::from(VList::new())
}

While this hack works, I think support for Option would be nice as it squares well with the rest of the Rust ecosystem.

DenSA-Inc commented 6 years ago

I tried to add this code to src/virtual_dom/vnode.rs:

impl<CTX, COMP: Component<CTX>> From<Option<VNode<CTX, COMP>>> for VNode<CTX, COMP> {
    fn from(value: Option<VNode<CTX, COMP>>) -> Self {
        match value {
            Some(t) => t,
            None => VNode::VText(VText::new(""))
        }
    }
}

impl<CTX: 'static, COMP: Component<CTX>, T: ToString> From<T> for VNode<CTX, COMP> {
    fn from(value: T) -> Self {
        VNode::VText(VText::new(value.to_string()))
    }
}

(The last function already existed). I get an "conflicting methods"-error:

error[E0119]: conflicting implementations of trait `std::convert::From<std::option::Option<virtual_dom::vnode::VNode<_, _>>>` for type `virtual_dom::vnode::VNode<_, _>`:
   --> /home/line/Projects/yew/src/virtual_dom/vnode.rs:108:1
    |
99  | impl<CTX, COMP: Component<CTX>> From<Option<VNode<CTX, COMP>>> for VNode<CTX, COMP> {
    | ----------------------------------------------------------------------------------- first implementation here
...
108 | impl<CTX: 'static, COMP: Component<CTX>, T: ToString> From<T> for VNode<CTX, COMP> {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `virtual_dom::vnode::VNode<_, _>`
    |
    = note: upstream crates may add new impl of trait `std::fmt::Display` for type `std::option::Option<virtual_dom::vnode::VNode<_, _>>` in future versions

From what I can see Option<_> is or can be a ToString and the newly created function would confuse the compiler. I'm only a rust beginner so i'd like you to take a look at this and give your opinion. @hgzimmerman

hgzimmerman commented 6 years ago

Without looking into Yew's source too much at the moment, this strikes me as odd, as ToString (or Display) don't seem to be implemented for std::option::Option<_>. My best guess would be that somewhere in Yew's source, one of those traits are implemented for Option<VNode<_,_>>, as I would think that that is the only scenario that Rust's coherence rules would allow for this compiler error to exist. Unfortunately, a quick ripgrep of the source doesn't bring up any implementations that would cause this error, leaving me confused as well.

I'll take a closer look in the near future.

DenSA-Inc commented 6 years ago

I just found out about that too, and the solution is brought upon us by the last line of the error message. upstream creates may add ..., I have no idea what this means. But I believe if we want to add Option<_> as html-able we need to cut back on the stringable things that can be converted. From what i've seen every object that implements the Debug-Trait can be converted to a string, and normally you don't want to use them in your html-macro. Maybe we have to only allow strings to be converted to nodes, and some hand-picked types that implement ToString.

DenSA-Inc commented 6 years ago

Update: I've gotten it to work by removing the support for everything other than strings (e.g. int32's etc.). Code will follow soon and as said before i want some people to look over it to give their opinion. On the one hand I want Option to be renderable, on the other hand re-adding render-support for every datatype (int's, uint's, things i don't know i've broken) will be ugly.

DenSA-Inc commented 6 years ago

Update: With this macro you can easily add types that should render into a text-node with their stringified version as content:

macro_rules! textual_vnode_type {
    ($($type:tt),*) => {
        $(
        impl<CTX: 'static, COMP: Component<CTX>> From<$type> for VNode<CTX, COMP> {
            fn from(value: $type) -> Self {
                VNode::VText(VText::new(value.to_string()))
            }
        }
        )*
    };
}

textual_vnode_type!(u8, u16, u32, u64, u128,
                    i8, i16, i32, i64, i128);

For testing i used the macro with the integer-types, and it works. If we were to remove the From<T: ToString>-Implementation and re-implement a certain subset of them manually this might be a good way to do it. Still ugly, but ok.

DenSA-Inc commented 5 years ago

After trying it again today, and asking the rust-discord-community for help, I can't seem to get a solution that satisfies me.

Either Option<T> is Renderable and the conflicting ToString-implementations are added manually afterwards (which will make things very tedious) or there is a conflict between Option<T> and ToString.

The problem is that some day some drunk maintainer could decide to implement ToString for Option<T> and then the compiler has conflicting options which Renderable-impl to use.
I tried to do something like impl<...> From<Option<T>> for VNode<COMP> where Option<T>: !ToString and

trait MyMarker {}

impl<T: ToString> MyMarker for T {}
impl<T> !MyMarker for Option<T> {}

The second one was my attempt to create a trait that only applies to everything ToString except Option<T>. But I'm hitting the limits of stable-rust and nightly-rust and I don't think there will be any improvements to this in the near future.

My only idea I have left is adding an own Option<T>-type in the yew crate (=> that way we can be sure it doesn't implement ToString) and provide reasonable support to convert it to a std::option::Option<T> and do other stuff with it.

jstarry commented 4 years ago

Closing because returning html! {} is now possible