Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added IntoPropValue implementation to convert from Cows #3346

Merged
merged 8 commits into from
Aug 21, 2023

Conversation

its-the-shrimp
Copy link
Contributor

Description

  • Added implementations for IntoPropValue to allow for conversion from Cows
  • Turned impl IntoPropValue<String> for &'static str into impl<'a> IntoPropValue<String> for &'a str, making it more generic

Checklist

  • I have reviewed my own code
  • I have added tests

@its-the-shrimp its-the-shrimp changed the title added IntoPropValue impls to & from Cows added IntoPropValue implementation to convert from Cows Jul 8, 2023
@its-the-shrimp its-the-shrimp changed the title added IntoPropValue implementation to convert from Cows added IntoPropValue implementation to convert from Cows Jul 8, 2023
@github-actions
Copy link

github-actions bot commented Jul 8, 2023

Visit the preview URL for this PR (updated for commit 0cdeca7):

https://yew-rs-api--pr3346-add-into-prop-value-wx6r6769.web.app

(expires Sun, 27 Aug 2023 22:15:17 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

github-actions bot commented Jul 8, 2023

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 102.341 102.341 0 0.000%
boids 175.512 175.512 0 0.000%
communication_child_to_parent 95.222 95.222 0 0.000%
communication_grandchild_with_grandparent 108.890 108.890 0 0.000%
communication_grandparent_to_grandchild 105.540 105.540 0 0.000%
communication_parent_to_child 92.702 92.702 0 0.000%
contexts 110.891 110.891 0 0.000%
counter 89.208 89.208 0 0.000%
counter_functional 89.923 89.923 0 0.000%
dyn_create_destroy_apps 92.289 92.289 0 0.000%
file_upload 103.544 103.544 0 0.000%
function_memory_game 174.318 174.318 0 0.000%
function_router 352.167 352.167 0 0.000%
function_todomvc 163.354 163.354 0 0.000%
futures 227.713 227.713 0 0.000%
game_of_life 112.296 112.296 0 0.000%
immutable 188.928 188.928 0 0.000%
inner_html 86.015 86.015 0 0.000%
js_callback 112.997 112.997 0 0.000%
keyed_list 201.203 201.203 0 0.000%
mount_point 89.208 89.208 0 0.000%
nested_list 114.491 114.491 0 0.000%
node_refs 96.169 96.169 0 0.000%
password_strength 1583.158 1583.158 0 0.000%
portals 98.240 98.240 0 0.000%
router 318.157 318.157 0 0.000%
simple_ssr 143.636 143.636 0 0.000%
ssr_router 389.362 389.362 0 0.000%
suspense 118.648 118.648 0 0.000%
timer 91.754 91.754 0 0.000%
timer_functional 100.293 100.293 0 0.000%
todomvc 143.696 143.696 0 0.000%
two_apps 89.917 89.917 0 0.000%
web_worker_fib 154.503 154.503 0 0.000%
webgl 88.530 88.530 0 0.000%

✅ None of the examples has changed their size significantly.

@github-actions
Copy link

github-actions bot commented Jul 8, 2023

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 391.336 419.425 403.872 8.222
Hello World 10 722.430 739.436 731.232 7.193
Function Router 10 2283.467 2319.563 2295.926 13.042
Concurrent Task 10 1008.135 1011.217 1009.597 1.119
Many Providers 10 1645.507 1774.164 1692.123 36.783

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 368.752 405.303 388.251 10.675
Hello World 10 691.344 739.614 715.094 13.508
Function Router 10 2185.447 2321.693 2252.309 35.574
Concurrent Task 10 1007.530 1010.483 1009.532 0.998
Many Providers 10 1579.633 1672.946 1624.360 34.015

Copy link
Member

@ranile ranile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apart from the couple of comments about conversions, this looks good to me. Thanks for the PR!

@@ -182,10 +183,13 @@ macro_rules! impl_into_prop {
}

// implemented with literals in mind
impl_into_prop!(|value: &'static str| -> String { value.to_owned() });
impl_into_prop!(<'a> |value: &'a str| -> String { value.to_owned() });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a fan of this conversion, but since we were already doing it for 'static, this isn't much worse either

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally it should be done for AttrValue anyway. I guess those are here for backward compatibility more than anything

impl_into_prop!(|value: &'static str| -> AttrValue { AttrValue::Static(value) });
impl_into_prop!(|value: String| -> AttrValue { AttrValue::Rc(Rc::from(value)) });
impl_into_prop!(|value: Rc<str>| -> AttrValue { AttrValue::Rc(value) });
impl_into_prop!(|value: Cow<'static, str>| -> AttrValue { AttrValue::from(value) });
impl_into_prop!(<'a> |value: Cow<'a, str>| -> String { Cow::into_owned(value) });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if Cow is Borrowed? I don't think we should be implicitly cloning it here.

Copy link
Contributor Author

@its-the-shrimp its-the-shrimp Jul 8, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already implicitly convert a &str into String, implicitly converting a Cow<'a, str>, which is either a String or a &'a str, to String is just a logical continuation in my opinion

Copy link
Member

@cecton cecton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure if we should add implementation on something that should be deprecated.

The way I see it, everybody should use AttrValue and not String in their Props.

The fact that things work for String is probably just a backward compatibility thing.

Anybody else has an opinion on this?

@its-the-shrimp
Copy link
Contributor Author

Now that I look back at it, I do agree that an impl IntoPropValue<String> for Cow doesn't make much sense, I can issue a commit right now to remove it if others agree.

@ranile
Copy link
Member

ranile commented Aug 20, 2023

I'm not entirely sure if we should add implementation on something that should be deprecated.

The way I see it, everybody should use AttrValue and not String in their Props.

The fact that things work for String is probably just a backward compatibility thing.

Anybody else has an opinion on this?

I agree with this. I think we should emit a deprecation warning if String conversion is used and redirect everyone to use AttrValue

EDIT: apparently we can't deprecate trait impls (rust-lang/rust#39935). Here's a workaround that can be used in Yew's case: playground

Now that I look back at it, I do agree that an impl IntoPropValue<String> for Cow doesn't make much sense, I can issue a commit right now to remove it if others agree.

Sounds good.

Curious, can we have a blanket implementation for anything that AttrValue can be constructed from?

@its-the-shrimp
Copy link
Contributor Author

EDIT: apparently we can't deprecate trait impls (rust-lang/rust#39935). Here's a workaround that can be used in Yew's case: playground

I've tested it and it unconditionally reports the implementation itself as calling to deprecated code, seems like it doesn't work

@ranile
Copy link
Member

ranile commented Aug 20, 2023

EDIT: apparently we can't deprecate trait impls (rust-lang/rust#39935). Here's a workaround that can be used in Yew's case: playground

I've tested it and it unconditionally reports the implementation itself as calling to deprecated code, seems like it doesn't work

Oh, that's too bad then. I was hoping the macro call would make it not do that...

ranile
ranile previously approved these changes Aug 20, 2023
Copy link
Member

@ranile ranile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Can you please fix the conflicts so this can be merged?

@its-the-shrimp
Copy link
Contributor Author

I agree with this. I think we should emit a deprecation warning if String conversion is used and redirect everyone to use AttrValue

I wonder if we can emit such a warning from the derive macro of Properties when it encounters a field of type String or Rc<str>, I'll look into that

Copy link
Member

@cecton cecton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm already approving the change so the PR won't stale. But I do think it would be useful to add a warning for the users to discourage the use of String instead of AttrValue.

@its-the-shrimp
Copy link
Contributor Author

I've just added such a warning in #3382

@ranile ranile merged commit 3c4ac56 into yewstack:master Aug 21, 2023
20 checks passed
@ranile ranile added the A-yew Area: The main yew crate label Aug 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew Area: The main yew crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants