Add custom type for attribute values

[ranile]

Description

This PR introduces a new type, AttrValue, for attribute values. This is a replacement for Cow<'static, str> so that:

• We provide our own type for attribute values
• Reasons described in Support Rc in html! attributes #1851

Fixes #1851

Unsolved questions

• Should VTag also use this for the tag's name
• Should Classes use this instead of Cow<'static, str>

Checklist

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

[bakape]

What is the motivation for having a distinct AttrValue::Owned? With reference counting added, seems like an opt-in deoptimization.

[ranile]

What is the motivation for having a distinct AttrValue::Owned? With reference counting added, seems like an opt-in deoptimization.

Cases where the user already has a String. I guess it would be possible to do it in From impl though. There might be cases where it would be cheaper to clone the String (think of passing String::new()) than keeping a Rc<str>. I haven't benchmarked it so can't really say for sure.

[siku2]

I'm wondering whether we should perhaps make AttrValue an opaque type that doesn't expose the underlying enum and just implements the various From traits.
Do you think there's any benefit to exposing the enum?

[ranile]

Do you think there's any benefit to exposing the enum?

const VALUE: AttrValue = AttrValue::Static("value");

This isn't possible with From trait as it's method isn't const

[siku2]

const VALUE: AttrValue = AttrValue::Static("value");

This isn't possible with From trait as it's method isn't const

Sure, but we could "fix" this by introducing a const constructor in this case.
I'm just worried that at some point we'll want to change the enum values in some way (perhaps adding a Number variant for faster diffing of numbers) and while non_exhaustive might come to mind, I don't think it's appropriate here.

[ranile]

Sure, but we could "fix" this by introducing a const constructor in this case.
I'm just worried that at some point we'll want to change the enum values in some way (perhaps adding a Number variant for faster diffing of numbers)

What will the constructor be called?

impl AttrValue {
    fn new_static(value: &'static str) -> Self { .. }
    fn new_number(value: u32) -> Self { .. }
    // ...
}

feels wrong

while non_exhaustive might come to mind, but I don't think it's appropriate here.

Why not? non_exhaustive is exactly for these kinds of cases: where the variants may change in the future in a way that isn't a breaking change.

[mc1098]

I think once we start tackling #1322 and more specifically allow objects to be set to properties then AttrValue will probably look very different (or not even exist), so future proofing it doesn't seem worthwhile(?) unless we are planning to add u32 support and nothing else soon.

[siku2]

I think once we start tackling #1322 and more specifically allow objects to be set to properties then AttrValue will probably look very different (or not even exist), so future proofing it doesn't seem worthwhile(?) unless we are planning to add u32 support and nothing else soon.

That's a good point, but I don't think #1322 is going to happen anytime soon.

For me the question only boils down to whether we want users to be able to destructure AttrValue or not. If there's a good use-case then I'm fine with keeping the variants exposed, but so far I haven't been able to come up with any. If there's no good reason, I'd much prefer to keep the API surface minimal. This isn't even strictly about future-proofing.

[siku2]

But I just want to make it clear that I don't feel that strongly about this, I'm just always looking for ways to reduce the public API surface to what's truly needed.

[ranile]

I don't see how #1322 is gonna be touching the AttrValue. That will just switch out the set_attribute call with Reflect::set. I will take a look at implementing that soon

[bakape]

I think once we start tackling #1322 and more specifically allow objects to be set to properties then AttrValue will probably look very different (or not even exist), so future proofing it doesn't seem worthwhile(?) unless we are planning to add u32 support and nothing else soon.

If you are talking about breaking the enum, I am already planning to. There are some really nice optimizations we can get by specializing VTag trees at compile time, effectively turning the html macro into an optimizing compiler. This is still a while off though - after I am done experimenting with render deduplication in the scheduler.

[siku2]

If you are talking about breaking the enum, I am already planning to. There are some really nice optimizations we can get by specializing VTag trees at compile time, effectively turning the html macro into an optimizing compiler. This is still a while off though - after I am done experimenting with render deduplication in the scheduler.

Unless I'm misunderstanding what you're trying to do that should still be possible whether the enum is publicly exposed or not.

[bakape]

Unless I'm misunderstanding what you're trying to do that should still be possible whether the enum is publicly exposed or not.

The basic idea is "why have enums at all?" - much of diffing can be specialised at compile time. Concerning attributes, make a trait like Attributes and implement it for however many types you want (including () for no attributes and IndexedMap<&str, Attvalue> for dynamic cases). We already replace the element, if tags don't match, so why not do the same, if the type parameters on VTag don't match? Tag matching and attributes sets not matching is not very common. With this as a framework, there can be zero memory footprint, zero cost comparisons and all kinds of specialized cases, including number diffing and heterogenic attribute sets, if we so fancy.

[bakape]

To clarify, I was specifically replying to

perhaps adding a Number variant for faster diffing of numbers

[ranile]

make a trait like Attributes and implement it for however many types you want

That trait will need to provide a function that returns a string that will be passed to the DOM. We can do all the diffing that we want on the Rust types but we still need (iirc) &str to pass to web_sys. How do you think we'd handle that without an enum while not allocating strings for 'statics or Rcs?

[bakape]

Yes, it would need to be convertible to a &str for web_sys. There are then 2 cases:

• Types that implement AsRef<str>
• Types that need to implement ToString
  For the latter, there will be an allocation, but only after we diff and determine we actually need to set the attribute. There could be performance gains in not converting to string prematurely, but benchmarks and considerations on prevalence are needed to ascertain it's actually worth it.

[ranile]

So, for now, should we just roll with it? Or should I make this an opaque type with const constructors? I like the former, const constructors don't feel right when the enum can be constructed directly.

[siku2]

Here are the long overdue comments :D

[siku2]

We should implement PartialEq<str> too. Maybe we don't even need PartialEq<String> then, but not quite sure about that.

[ranile]

How about removing PartialEq implementations altogether? AsRef and Deref can get the job done.

I added the String one because some place needed it but seems like it works fine if I use as_ref.

[ranile]

I removed the implementation. Let me know if one should be added.