Implement IntoPropValue for Key

[maurerdietmar]

I use "Key" inside component properties, so it would be convenient to have IntoPropValue for Key.

[github-actions[bot]]

Visit the preview URL for this PR (updated for commit 996f5b9):

https://yew-rs-api--pr2804-key-into-prop-value-7peqwwq4.web.app

_{(expires Wed, 10 Aug 2022 10:17:30 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

[github-actions[bot]]

Size Comparison

examples	master (KB)	pull request (KB)	diff (KB)	diff (%)
boids	172.691	172.683	-0.009	-0.005%
contexts	109.584	109.574	-0.010	-0.009%
counter	86.526	86.524	-0.002	-0.002%
counter_functional	87.180	87.179	-0.001	-0.001%
dyn_create_destroy_apps	89.622	89.621	-0.001	-0.001%
file_upload	102.760	102.759	-0.001	-0.001%
function_memory_game	166.868	166.869	+0.001	+0.001%
function_router	351.615	351.681	+0.065	+0.019%
function_todomvc	161.578	161.588	+0.010	+0.006%
futures	225.433	225.433	0	0.000%
game_of_life	107.108	107.107	-0.001	-0.001%
immutable	208.686	208.690	+0.005	+0.002%
inner_html	83.686	83.683	-0.003	-0.004%
js_callback	112.746	112.744	-0.002	-0.002%
keyed_list	195.253	195.251	-0.002	-0.001%
mount_point	86.170	86.171	+0.001	+0.001%
nested_list	115.649	115.642	-0.008	-0.007%
node_refs	93.465	93.459	-0.006	-0.006%
password_strength	1545.733	1545.730	-0.003	-0.000%
portals	97.262	97.259	-0.003	-0.003%
router	320.945	321.026	+0.081	+0.025%
simple_ssr	154.249	154.255	+0.006	+0.004%
ssr_router	397.747	397.683	-0.064	-0.016%
suspense	110.359	110.367	+0.008	+0.007%
timer	89.229	89.229	-0.001	-0.001%
todomvc	142.622	142.620	-0.002	-0.001%
two_apps	87.188	87.188	+0.001	+0.001%
web_worker_fib	153.600	153.607	+0.008	+0.005%
webgl	87.541	87.540	-0.001	-0.001%

✅ None of the examples has changed their size significantly.

[WorldSEnder]

Can you give a short example where it makes sense to use a Key as a prop value? While I agree that if you need it, Key is better than taking &'static str for a more uniform API with less surprises, what I don't quite see is the need to pass a Key further down.

Keep in mind that a key is only specific to a single list of children, has to be unique and has no influence on the order of rendering, so I was under the impression that there's no need to pass it from a parent to some child via props, rather generate it locally in each component.

In any case, it might also make sense, after having the IntoPropValue impls to change the macro to use that trait as well, instead of https://github.com/yewstack/yew/blob/5570710cdf88658877b0ddc41e4b531cd7bdc7e6/packages/yew-macro/src/props/prop.rs#L352

[maurerdietmar]

Can you give a short example where it makes sense to use a Key as a prop value? While I agree that if you need it, Key is better than taking &'static str for a more uniform API with less surprises, what I don't quite see is the need to pass a Key further down.

I am implementing a Tab panel, where each child is identified by a Key and has a render function. I also want to store which panel should be rendered as default.

#[derive(Clone, PartialEq, Properties)]
pub struct TabPanel {
    pub tabs: IndexMap<Key, RenderFn>,
    pub default_tab: Option<Key>,
}

Keep in mind that a key is only specific to a single list of children, has to be unique and has no influence on the order of rendering, so I was under the impression that there's no need to pass it from a parent to some child via props, rather generate it locally in each component.

In my example, the user should be able to set the default_tab ...

I create all my component properties using a builder pattern. Not sure if such setup makes sense with the html makro...

[maurerdietmar]

@maurerdietmar Is there any reason why your component props cannot accept a concrete type of Key (e.g.: &'static str) instead of Key in the props and pass the concrete type to the component where the key prop is assigned?

To be flexible, I want to accept more than one type (&str, String, Rc<&str>).

[WorldSEnder]

Ping @hamza1311 for his opinion on opening Key to be used as a key in a user hashmap. For now, it's exposed and used for the consumption in vnode/vlist. Do we want to guarantee the Hash and From<_> as a stability guarantee like this?

[ranile]

I'm not sure about any guarantees about Key. Really, it's just IString::Rc, so hashing would be performed on the inner str.

[maurerdietmar]

For now, it's exposed and used for the consumption in vnode/vlist. Do we want to guarantee the Hash and From<_> as a stability guarantee like this?

Implementing Hash is trivial, even if you switch to some other (binary) key representation (and you need it anways). Also, I can't imagine that you don't want to use strings for keys, so it must always be possible to convert strings into keys?

[cecton]

Wouldn't it be better to just drop the type Key and use IString instead? It feels like unnecessary maintenance burden.

[maurerdietmar]

Wouldn't it be better to just drop the type Key and use IString instead? It feels like unnecessary maintenance burden.

Or AttrValue, which is more flexible...

[cecton]

Or AttrValue, which is more flexible...

It's the same object, just a type alias

pub type AttrValue = implicit_clone::unsync::IString;

[maurerdietmar]

Or AttrValue, which is more flexible...

It's the same object, just a type alias

Ah, forgot that. Anyways, drop the type Key and use IString instead would solve the problem too.

[futursolo]

Wouldn't it be better to just drop the type Key and use IString instead? It feels like unnecessary maintenance burden.

If Key is calculated with std::hash::Hash, the internal representation of Key can be switched from Rc<str> to u64. We can avoid an allocation and u64 should be faster to compare than Strings.

A previous attempt: #2616

[cecton]

I'm not sure I understand why the #2616 was closed... did OP just gave up? That's sad....

Well okay so if I understand correctly we have 3 paths:

1. Accepting the current PR proposal: this would officialize a bit more our current implementation of Key which we don't want but it's easy to revert
2. Replace Key by IString: this would officialize using strings as key which is not super performant... but I guess it's also an easy thing to rollback and it might reduce the binary size
3. Give another shot at #2616: faster, better, stronger... hopefully does not increase the binary size?

Maybe we can pick a temporary solution for 0.20, that would be solution 1 or 2. Then leave the better solution (3) for 0.21 or later?

(I'm just trying to help unblock the issue, not pushing anyone to do something they don't want)

[futursolo]

did OP just gave up? That's sad....

I think OP deleted their fork of yew so the pull request got closed automatically.

Maybe we can pick a temporary solution for 0.20, that would be solution 1 or 2. Then leave the better solution (3) for 0.21 or later?

Maybe we can maintain status quo for 0.20?

This pull request does not change how keys are being created when it is actually used as keys. It allows strings to be converted to keys when present in properties (which is not the intended usage).

I think eventually #2616 (or something similar that does not involve an allocation) will land and at that time Key will become an opaque type, so I am not in favour of merging it with a type that guarantees access to an underlying value (e.g.: IString) as people will start to use them interchangeably and that might cause some migration issues for a future version.

[voidpumpkin]

From what I gathered in this PR discussions I see we will not go with this PR's change. Sorry, @maurerdietmar. Still, thx a lot for the proposal and effort!

I have made an issue to move further discussions, and link other PR's to: #3205