Remove special handing of struct fields of type `Option` in `derive(Properties)` #3398

its-the-shrimp · 2023-09-09T16:59:59Z

Description

Fixes #3395

Checklist

I have reviewed my own code
I have added tests (not sure if a new test is needed after removing a feature)

github-actions · 2023-09-09T17:02:30Z

Visit the preview URL for this PR (updated for commit 4de7160):

https://yew-rs-api--pr3398-remove-option-specia-b2jmg0pe.web.app

_{(expires Tue, 19 Sep 2023 20:07:51 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

github-actions · 2023-09-09T17:08:52Z

Size Comparison

examples	master (KB)	pull request (KB)	diff (KB)	diff (%)
async_clock	102.637	102.637	0	0.000%
boids	175.874	175.874	0	0.000%
communication_child_to_parent	95.527	95.527	0	0.000%
communication_grandchild_with_grandparent	109.263	109.263	0	0.000%
communication_grandparent_to_grandchild	105.948	105.948	0	0.000%
communication_parent_to_child	93.015	93.015	0	0.000%
contexts	111.238	111.238	0	0.000%
counter	89.423	89.423	0	0.000%
counter_functional	90.157	90.157	0	0.000%
dyn_create_destroy_apps	92.596	92.596	0	0.000%
file_upload	103.755	103.755	0	0.000%
function_memory_game	174.855	174.855	0	0.000%
function_router	352.738	352.738	0	0.000%
function_todomvc	163.751	163.751	0	0.000%
futures	227.985	227.985	0	0.000%
game_of_life	112.589	112.589	0	0.000%
immutable	189.449	189.449	0	0.000%
inner_html	86.205	86.205	0	0.000%
js_callback	113.448	113.537	+0.089	+0.078%
keyed_list	201.528	201.528	0	0.000%
mount_point	89.414	89.414	0	0.000%
nested_list	114.828	114.828	0	0.000%
node_refs	96.516	96.516	0	0.000%
password_strength	1584.312	1584.312	0	0.000%
portals	98.591	98.591	0	0.000%
router	318.857	318.857	0	0.000%
simple_ssr	144.353	144.441	+0.089	+0.062%
ssr_router	390.498	390.498	0	0.000%
suspense	119.124	119.242	+0.118	+0.099%
timer	92.023	92.023	0	0.000%
timer_functional	100.690	100.690	0	0.000%
todomvc	143.905	143.905	0	0.000%
two_apps	90.122	90.122	0	0.000%
web_worker_fib	154.714	154.714	0	0.000%
webgl	88.780	88.780	0	0.000%

✅ None of the examples has changed their size significantly.

github-actions · 2023-09-09T17:09:09Z

Benchmark - SSR

Yew Master

Benchmark	Round	Min (ms)	Max (ms)	Mean (ms)	Standard Deviation
Baseline	10	428.321	429.623	429.016	0.424
Hello World	10	780.283	784.982	781.197	1.428
Function Router	10	2504.406	2522.572	2512.630	5.413
Concurrent Task	10	1007.339	1011.398	1008.742	1.167
Many Providers	10	1738.228	1783.301	1750.761	13.336

Pull Request

Benchmark	Round	Min (ms)	Max (ms)	Mean (ms)	Standard Deviation
Baseline	10	424.838	426.496	425.567	0.464
Hello World	10	747.628	750.016	748.633	0.861
Function Router	10	2457.074	2476.903	2464.585	5.372
Concurrent Task	10	1008.237	1009.178	1008.634	0.287
Many Providers	10	1742.475	1791.777	1765.503	17.589

kirillsemyonkin · 2023-09-09T19:51:17Z

Does converting T into Some::<T> still work? impl<T> IntoPropValue<Option<T>> for T says it probably should.

its-the-shrimp · 2023-09-09T20:52:17Z

added a test for that, it passes

ranile

Thanks for the PR! It looks good, just a couple of comments, only related to tests

ranile · 2023-09-11T17:51:00Z

packages/yew-macro/tests/derive_props/pass.rs

-mod t12 {
- #[derive(::std::clone::Clone, ::yew::Properties, ::std::cmp::PartialEq)]
- pub struct Props<T: ::std::clone::Clone + ::std::cmp::PartialEq> {
- value: ::std::option::Option<T>,
- }
-
- fn optional_prop_generics_should_work() {
- ::yew::props! { Props::<::std::primitive::bool> { } };
- ::yew::props! { Props::<::std::primitive::bool> { value: true } };
- }
-}
-


Why remove this test? We should not be removing tests. I would like to have this change not come at the cost of functionality. Please add the test back

This test (or half of it) would go into fail.rs then, as it produces the problem this PR addresses.

Also I should note that tests being removed all the time is a normal procedure for many developers. Tests can be removed if they do not reflect current reality (i.e. do not pass, in this case because behavior is opposite to what previously was tested for), are covered by other tests (which have been added (will be added per your suggestions) in this PR by the way), or simply poorly written or considered more of a burden for development than the sought for assistance they are supposed to give.

Also I should note that tests being removed all the time is a normal procedure for many developers. Tests can be removed if they do not reflect current reality (i.e. do not pass, in this case because behavior is opposite to what previously was tested for), are covered by other tests (which have been added (will be added per your suggestions) in this PR by the way), or simply poorly written or considered more of a burden for development than the sought for assistance they are supposed to give.

Absolutely, but in this case, the test can be updated to be passed and covers a very important functionality: generic props. We should have a passing test for that feature

ranile · 2023-09-11T17:53:51Z

packages/yew-macro/tests/derive_props/pass.rs

+
+ #[::yew::function_component]
+ fn Main() -> ::yew::html::Html {
+ ::yew::html! { <Inner field1=3 field2=5/> }


Can you also add a case where only the required field is passed? Also, please add an identical test but failing where it doesn't pass the required prop.

ranile

Thanks! Looks good to me

its-the-shrimp added 2 commits September 9, 2023 17:54

removed PropAttr::Option

8cc02a8

fixed formatting

247a207

removed an irrelevant test

7997d85

added a test for converting value into Some(value) in the html! macro

471f259

ranile requested changes Sep 11, 2023

View reviewed changes

its-the-shrimp added 2 commits September 12, 2023 21:02

added more tests for derive(Properties)

1771ce8

added more tests (again)

4de7160

ranile approved these changes Sep 22, 2023

View reviewed changes

ranile requested a review from futursolo September 22, 2023 22:00

ranile added breaking change A-yew-macro Area: The yew-macro crate labels Sep 22, 2023

github-actions bot approved these changes Sep 22, 2023

View reviewed changes

futursolo approved these changes Sep 23, 2023

View reviewed changes

futursolo merged commit ce7702e into yewstack:master Sep 23, 2023
23 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove special handing of struct fields of type `Option` in `derive(Properties)` #3398

Remove special handing of struct fields of type `Option` in `derive(Properties)` #3398

its-the-shrimp commented Sep 9, 2023 •

edited

Loading

github-actions bot commented Sep 9, 2023 •

edited

Loading

github-actions bot commented Sep 9, 2023 •

edited

Loading

github-actions bot commented Sep 9, 2023 •

edited

Loading

kirillsemyonkin commented Sep 9, 2023

its-the-shrimp commented Sep 9, 2023

ranile left a comment

ranile Sep 11, 2023

kirillsemyonkin Sep 11, 2023

kirillsemyonkin Sep 11, 2023

ranile Sep 11, 2023

ranile Sep 11, 2023

ranile left a comment

Remove special handing of struct fields of type Option in derive(Properties) #3398

Remove special handing of struct fields of type Option in derive(Properties) #3398

Conversation

its-the-shrimp commented Sep 9, 2023 • edited Loading

Description

Checklist

github-actions bot commented Sep 9, 2023 • edited Loading

github-actions bot commented Sep 9, 2023 • edited Loading

Size Comparison

github-actions bot commented Sep 9, 2023 • edited Loading

Benchmark - SSR

Yew Master

Pull Request

kirillsemyonkin commented Sep 9, 2023

its-the-shrimp commented Sep 9, 2023

ranile left a comment

Choose a reason for hiding this comment

ranile Sep 11, 2023

Choose a reason for hiding this comment

kirillsemyonkin Sep 11, 2023

Choose a reason for hiding this comment

kirillsemyonkin Sep 11, 2023

Choose a reason for hiding this comment

ranile Sep 11, 2023

Choose a reason for hiding this comment

ranile Sep 11, 2023

Choose a reason for hiding this comment

ranile left a comment

Choose a reason for hiding this comment

Remove special handing of struct fields of type `Option` in `derive(Properties)` #3398

Remove special handing of struct fields of type `Option` in `derive(Properties)` #3398

its-the-shrimp commented Sep 9, 2023 •

edited

Loading

github-actions bot commented Sep 9, 2023 •

edited

Loading

github-actions bot commented Sep 9, 2023 •

edited

Loading

github-actions bot commented Sep 9, 2023 •

edited

Loading