Impl IntoOptPropValue for Option<V> -> Option<T>

[MuhannadAlrusayni]

The only implemented types are those passed to impl_into_prop! macro

Description

I need this so I can pass Option<String> to some html attributes such as style and id.

Here is an simplified example of what I have:

impl Compoent for Input {
    fn view(&self) -> Html {
        // this is Option<String>
        let input_style = self.design_system.get_style::<Input>(self);
        <input style=input_style ...etc />
    }
}

without this PR I get error saying:

error[E0277]: the trait bound `Option<std::string::String>: IntoPropValue<Option<Cow<'static, str>>>` is not satisfied
   --> src/input.rs:258:30
    |
258 |                        style=input_style
    |                              ^^^^^^^^^^^ the trait `IntoPropValue<Option<Cow<'static, str>>>` is not implemented for `Option<std::string::String>`
    | 
   ::: .../yew/packages/yew/src/virtual_dom/mod.rs:69:47
    |
69  |     pub fn new(key: &'static str, value: impl IntoOptPropValue<AttrValue>) -> Self {
    |                                               --------------------------- required by this bound in `PositionalAttr::new`
    |
    = note: required because of the requirements on the impl of `IntoOptPropValue<Cow<'static, str>>` for `Option<std::string::String>`

Checklist

• I have run cargo make pr-flow
• I have reviewed my own code
• I have added tests

[siku2]

@cecton I'd love to hear your opinion on this too. I think it's okay for consistency, but it might encourage users to expose Option<String> rather than Option<Cow>>

[cecton]

🤔 I'm starting to wonder if it would be possible to add a compiler warning to discourage people using String or make it a hard error. It's not like Cow<'static, str> has any inconvenient for the developers... does it?

[ranile]

thinking I'm starting to wonder if it would be possible to add a compiler warning to discourage people using String or make it a hard error.

Can we even warn about this? As far as I know, there's no way to check if the type is String at compile time in the proc-macro.

It's not like Cow<'static, str> has any inconvenient for the developers... does it?

It has the downside of having to manually create the Cow. I brought it up here on Discord.

[MuhannadAlrusayni]

It's not always feasible to create Cow<'static, str> especially when user use 3rd party crate that
return Option<String/&'static str> that user want to pass directly to component just like in my case. If we force the user code to only pass Cow<'static, str> then the user would need to make the conversions before passing it to the prop which is verbose and we can do that for user.

thinking I'm starting to wonder if it would be possible to add a compiler warning to discourage
people using String or make it a hard error. It's not like Cow<'static, str> has any
inconvenient for the developers... does it?

I think this apply on the component code not on user code, since the user code only use what the component props type. So it's the components developer responsibility to make sure the props type are of Cow<'static, str> (if that make sense for the component which is usually is)

[cecton]

I think we need to split the problem in 2 concerns we want to address:

1. The complication due to the conversion
2. The intuitiveness/ergonomic of having all components using Cow<str> instead of String

Point1: conversion

I just made this temporary project:

use yew::prelude::*;

struct MyComponent;

impl Component for MyComponent {
    type Properties = ();
    type Message = ();

    fn create(_: <Self as yew::Component>::Properties, _: ComponentLink<Self>) -> Self {
        todo!()
    }
    fn update(&mut self, _: <Self as yew::Component>::Message) -> bool {
        todo!()
    }
    fn change(&mut self, _: <Self as yew::Component>::Properties) -> bool {
        todo!()
    }
    fn view(&self) -> Html {
        let option_string: Option<String> = None;
        let string: String = String::new();
        html! {
            <>
            <span style=option_string.map(Into::into) />
            <span style=string />
            </>
        }
    }
}

fn main() {
    println!("Hello, world!");
}

Which seems to work fine AND it's very concise:

   Compiling tmp-tjisdy v0.1.0 (/home/cecile/.cache/cargo-temp/tmp-tjiSdy)
    Finished dev [unoptimized + debuginfo] target(s) in 0.23s
     Running `target/debug/tmp-tjisdy`
Hello, world!

Now we can debate whether or not this is ergonomic but it seems to me using map() on Option is a pretty common thing to do in Rust.

Point 2: intuitiveness/ergonomic

On intuitiveness

Rust is a programming language without GC. We use the borrow checker to enforce safety. The alternative to borrowing is reference counting (Rc).

Either our API is capable to handle borrowed values (this means having lifetimes everywhere) or we extensively use Rc. I have personally not explored the path of using more lifetimes but I suspect it might be getting crazy with the React-like component hierarchy. The other thing is that v0.18 will not change its implementation. So cloning and reference counting in Yew 0.18 is going to stay. This doesn't mean we can't create an issue and try to find new solutions for further versions.

Because of the requirement of cloning everywhere, it means we are going to clone a lot of unnecessary things like String. Strings can be costly to clone and since HTML is kinda "stringly typed", using &'static str can be a huge benefit.

Of course it's not because HTML is stringly typed that we want a web framework API to rely on string typing. That is why it is important to discourage the use of String and Option<String> by the user. At this time we do not discourage the use of String but we do discourage the use of Option<String>. It's interesting to know we might not be very consistent here.

(Side note: one alternative would be to use Rc<String> everywhere and force the user to deal with that instead. This is a breaking change so it's not really worth to talk about it here.)

On ergonomic

It's kinda annoying to type Cow<'static, str>. It is possible to make things more "friendly" by using type alias like we have Html and ShouldRender. The unfortunate cost of that is the lack of explicitness: it makes the user wonder what is it and force them to use the doc to understand how it works. I'm personally not against this solution (but I'm not for it either).

To improve the situation we could improve the error message provided by the macro. As @hamza1311 said it is not possible to check at compile time what the type is. But we can definitely parse the types of the user's own components and detect Option<String>. Because that is the anti-pattern we want to avoid. Though I'm not sure if we can print warnings.

Possible solution

1. Add a warning for Option<String> (and String) in user's component props
2. Allow automatic conversion like you did in this PR

If we can't do the point 1, than we shouldn't do the point 2.

[ranile]

Now we can debate whether or not this is ergonomic but it seems to me using map() on Option is a pretty common thing to do in Rust.

While that approach is fine for elements, I'm not sure how well it'd work for components which take Option<Cow<'static, str>> in their props.
In that situation Cow must be created manually (do note that into was unable to infer the type when I tried to do it) which isn't ergonomic but that's a limitation of type system.

@siku2 mentioned on discord that we should have a string type in Yew which is a wrapper around Cow<'static, str> and I agree with that.

one alternative would be to use Rc<String> everywhere and force the user to deal with that instead. This is a breaking change so it's not really worth to talk about it here

I'm not sure if that's a good idea because there would be no way mutate it without cloning or interior mutability.

At this time we do not discourage the use of String but we do discourage the use of Option<String>. It's interesting to know we might not be very consistent here.

I think we should stay consistent. I don't think Cow<'static, str> is any less ergonomic than String. It might be a good idea to also discourage use of String.

---

If nothing else, I think we should clarify anti-patterns in the documentation. If we can, I wouldn't mind logging a warning to browser's console on use String or Option<String>.

[siku2]

Thank you both for your input, @cecton and @hamza1311.
I've decided to accept this change mainly because it follows the principle of least surprise.
It doesn't directly encourage bad design patterns. Sure, it makes it easier to expose a Option<String> prop type, but String itself doesn't implement ImplicitClone so whenever someone uses the props they will have to explicitly clone their string type.
That's the important part, the potentially expensive cost isn't hidden from the user. This also isn't unlike normal Rust structs / functions so there should already be a sense of familiarity with this.

[ranile]

I think we should document anti-patterns. I would be fine with making a PR for those but first we need to list them.

1. Taking String, instead of Cow<'static, str> as props.
2. What else?

[siku2]

I think we should document anti-patterns. I would be fine with making a PR for those but first we need to list them.

1. Taking `String`, instead of `Cow<'static, str>` as props.

2. What else?

Let's open an issue to track those