DioxusLabs / dioxus

Fullstack GUI library for web, desktop, mobile, and more.
https://dioxuslabs.com
Apache License 2.0
18.5k stars 703 forks source link

Replace `AttributeValue::Text` with `Cow<'static, str>` #2308

Open matthunz opened 2 weeks ago

matthunz commented 2 weeks ago
pub enum AttributeValue {
    Text(Cow<'static, str>),
    ...
}

This would allow for less clones when using text nodes.

However this does introduce the tradeoff of not automatically converting &str to String, only &'static str is permitted to reduce cloning.

Definitely open to changing any of this! 😄

onweru commented 2 weeks ago

However this does introduce the tradeoff of not automatically converting &str to String, only &'static str is permitted to reduce cloning.

Does this mean I won't be able to do this

#[component]
fn Text(class: String) {
  rsx! { 
     div { class, ... }
  }
}

Text {  class: "some-class" }

Hated having to

Text {  class: "some-class".into() } // or the to_string() 
matthunz commented 2 weeks ago

Hated having to

Text {  class: "some-class".into() } // or the to_string() 

Unfortunately this would be a breaking change but I'd say the optional solution is #[props(into) class: Cow<'static, str> to avoid having to allocate the String.

Then you can still do Text { class: "some-class" }

ealmloff commented 2 weeks ago

I would be interested to see benchmarks if this has a large performance impact.

Dioxus will currently not allocate with static strings inline in the rsx:

rsx! {
   div {
      // "hello world" is included in the template which only supports &'static str attributes
      width: "hello world",
   }
}

This PR would enable less allocation for the following code:

const STATIC: &str = "hello world";
const STATICS: &[&str] = &["hello world"];

let index = use_signal(|| 0);
rsx! {
   div {
      // Theoretically this could be part of the template as well, but we are currently not smart enough to know that it is static
      width: STATIC,
      // This shouldn't be part of the template because it is dynamic
      height: STATICS[index()],
   }
}

There is also some overlap with https://github.com/DioxusLabs/dioxus/pull/2258/files#diff-b96a80b4b2e2f0da84fe76143d9f213d159383fd322e41068eba85fcd7fe93f3 which introduces string type with static and dynamic segments (currently only used in debug mode for hot reloading)

matthunz commented 2 weeks ago

TODO: Greg Johnston's new oco_ref crate might fit great here 👀