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

Port yew-autoprops to yew-macro #3505

Merged
merged 22 commits into from
Dec 22, 2023
Merged

Port yew-autoprops to yew-macro #3505

merged 22 commits into from
Dec 22, 2023

Conversation

cecton
Copy link
Member

@cecton cecton commented Nov 1, 2023

Description

cc @kirillsemyonkin @valyagolev

Proposal for #3499

Checklist

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

github-actions[bot]
github-actions bot previously approved these changes Nov 1, 2023
Copy link

github-actions bot commented Nov 1, 2023

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

https://yew-rs--pr3505-autoprops-3i3ue3yy.web.app

(expires Fri, 29 Dec 2023 07:00:41 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

Copy link

github-actions bot commented Nov 1, 2023

Benchmark - core

Yew Master

vnode           fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ vnode_clone  2.756 ns      │ 5.577 ns      │ 2.759 ns      │ 2.819 ns      │ 100     │ 1000000000

Pull Request

vnode           fastest       │ slowest       │ median        │ mean          │ samples │ iters
╰─ vnode_clone  2.47 ns       │ 3.438 ns      │ 2.474 ns      │ 2.486 ns      │ 100     │ 1000000000

github-actions[bot]
github-actions bot previously approved these changes Nov 1, 2023
github-actions[bot]
github-actions bot previously approved these changes Nov 1, 2023
Copy link

github-actions bot commented Nov 1, 2023

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 101.085 101.083 -0.002 -0.002%
boids 174.754 174.749 -0.005 -0.003%
communication_child_to_parent 93.473 93.473 0 0.000%
communication_grandchild_with_grandparent 106.465 106.454 -0.011 -0.010%
communication_grandparent_to_grandchild 101.711 101.708 -0.003 -0.003%
communication_parent_to_child 89.812 89.813 +0.002 +0.002%
contexts 106.578 106.581 +0.003 +0.003%
counter 86.855 86.854 -0.002 -0.002%
counter_functional 87.257 87.261 +0.004 +0.004%
dyn_create_destroy_apps 89.701 89.699 -0.002 -0.002%
file_upload 100.729 100.729 0 0.000%
function_memory_game 173.502 173.496 -0.006 -0.003%
function_router 350.721 350.745 +0.024 +0.007%
function_todomvc 162.199 162.206 +0.007 +0.004%
futures 230.407 230.408 +0.001 +0.000%
game_of_life 111.108 111.113 +0.005 +0.004%
immutable 186.096 188.435 +2.339 +1.257%
inner_html 80.552 80.555 +0.003 +0.004%
js_callback 110.316 110.321 +0.005 +0.004%
keyed_list 200.063 200.069 +0.006 +0.003%
mount_point 83.446 83.444 -0.002 -0.002%
nested_list 114.661 114.665 +0.004 +0.003%
node_refs 90.963 90.965 +0.002 +0.002%
password_strength 1751.697 1751.691 -0.006 -0.000%
portals 94.202 94.197 -0.005 -0.005%
router 319.571 319.565 -0.006 -0.002%
simple_ssr 141.325 141.324 -0.001 -0.001%
ssr_router 388.968 388.956 -0.012 -0.003%
suspense 116.549 116.551 +0.002 +0.002%
timer 89.367 89.367 0 0.000%
timer_functional 98.732 98.733 +0.001 +0.001%
todomvc 142.512 142.512 0 0.000%
two_apps 86.570 86.574 +0.004 +0.005%
web_worker_fib 135.578 135.577 -0.001 -0.001%
web_worker_prime 185.973 185.961 -0.012 -0.006%
webgl 83.118 83.116 -0.002 -0.002%

⚠️ The following example has changed its size significantly:

examples master (KB) pull request (KB) diff (KB) diff (%)
immutable 186.096 188.435 +2.339 +1.257%

Copy link

github-actions bot commented Nov 1, 2023

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 308.890 310.843 309.359 0.558
Hello World 10 490.457 516.646 496.910 10.482
Function Router 10 1652.826 1684.620 1667.044 10.730
Concurrent Task 10 1005.940 1006.783 1006.503 0.266
Many Providers 10 1175.098 1250.361 1201.625 21.011

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 289.275 292.685 289.930 1.158
Hello World 10 493.902 513.369 499.176 7.129
Function Router 10 1631.915 1643.194 1637.777 3.241
Concurrent Task 10 1005.832 1007.311 1006.556 0.592
Many Providers 10 1131.115 1207.548 1155.489 25.386

github-actions[bot]
github-actions bot previously approved these changes Nov 1, 2023
github-actions[bot]
github-actions bot previously approved these changes Nov 1, 2023
github-actions[bot]
github-actions bot previously approved these changes Nov 1, 2023
@its-the-shrimp
Copy link
Contributor

its-the-shrimp commented Nov 1, 2023

This is awesome, this will reduce so much boilerplate, might be one of the best, if not the best feature introduced by the next version of Yew 🔥

@kirillsemyonkin
Copy link
Contributor

It will probably take a while to merge, so for the time being, consider yewstack/yew-autoprops#9 (which is also yewstack/yew-autoprops#7, but this one is not really breaking) and yewstack/yew-autoprops#8, as they are kind of breaking issues, if you have not already fixed them in your port

@cecton
Copy link
Member Author

cecton commented Nov 1, 2023

This is awesome, this will reduce so much boilerplate, might be one of the best, if not the best feature introduced by the next version of Yew 🔥

i know right!!! all the credits is to @valyagolev seriously

@ranile
Copy link
Member

ranile commented Nov 1, 2023

I'd rather use the function_component attribute instead of adding a new autoprops one

@its-the-shrimp
Copy link
Contributor

oh yeah, smth like #[function_component(+autoprops)] or #[function_component(App +autoprops)] for a custom name would be much better

@kirillsemyonkin
Copy link
Contributor

I just checked Dioxus, their alternative is called #[inline_props]. They do not use #[function_component] though, their alternative of HookContext is not hidden from user and has to be explicitly pass on each step, but in our case we would have two attributes on same function.

I recommend the following variants:

  • #[function_component]
  • #[function_component(autoprops)]
    • limits from calling struct autoprops - is this an issue even? also consider:
    • #[function_component(_, autoprops)]
    • #[function_component(autoprops=_)]
  • #[function_component(App)]
  • #[function_component(App, autoprops)]
    • #[function_component(App, autoprops=_)]
  • #[function_component(autoprops=AppProps)]
    • #[function_component(_, autoprops=AppProps)]
  • #[function_component(App, autoprops=AppProps)]

Optionally (if user wants to be more explicit, like in Python's functions and Java's annotations),

  • #[function_component(name=App, autoprops=AppProps)] (args would be swappable then)

Instead of equals it can look like calls (e.g. #[function_component(autoprops(AppProps))]), if that looks normal.

Also remember about #3447.

@cecton cecton marked this pull request as draft November 8, 2023 07:32
@rogusdev
Copy link

I, for one more, am very interested in this. Writing props structs seems tedious and redundant compared to just using the function arguments on the handler function. I would love to see this functionality available somewhere, and it does seem like something that should be a built in part of yew, but if it's a separate crate, that's fine for me too. I just want to use it, ideally sooner than later ;)

@futursolo
Copy link
Member

futursolo commented Nov 12, 2023

My stance on this feature request:

  1. There is no advantage for a first-party integration for this particular feature.
  2. For a first-party integration, the syntax should be the best available and look like something that would uphold the standard of Yew 1.0.
    (Currently none of the syntax purposed is without some caveats.)
  3. Due to 1 and 2, I think it is a better idea to provide an implementation outside of Yew. This might allow a better syntax to develop over the time which might solve 2.
  4. First-party implementation will discourage further development of third-party alternative implementations, which will not help to solve 2.
  5. Manually defined props cannot be opted out due to various reasons and is there for the foreseeable future.

@rogusdev
Copy link

@cecton is this something you could put up as a change on the old repo, that gets transferred to you?

@futursolo is the debate over the feature in concept, or just the specific syntax chosen? The implementation seems to do the thing which is indeed desirable, or do you think this is not a desirable feature at all?

To be clear, it makes sense that props do need to exist sometimes, and this autoprops feature should be purely opt-in. Do you think this is valuable given those conditions?

@kirillsemyonkin
Copy link
Contributor

@rogusdev yewstack/yew-autoprops#10

}

#[function_component]
fn HelloWorld(props: &Props) -> Html {
html! {<>{"Hello world"}{props.name.clone()}</>}
fn Hello(&Props { is_loading, ref name }: &Props) -> Html {
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm using the syntax proposed here: yewstack/implicit-clone#39 (comment) by Alex Parrill. I think this should appear a lot more in the doc. (Please let me know your opinion on this.)

I think I learned that syntax in the context of match at the beginning I was using Rust but then I completely forgot about it and I'm not sure I have ever saw it used in destructuring like that.

Most of the time you will want to use this syntax. Actually probably always. Because it allows dereferencing the primitive types automatically (the Copy-able types). For types that are Implicit-lyCloneable like AttrValue it's actually best to take them by reference because you can pass that reference directly to the properties of child component. And it won't move it because it's a reference.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's certainly handy to know, but not to always use, since writing the name of the props type twice can get cumbersome really fast. If only Rust had something like anonymous struct patterns...

Copy link
Member Author

Choose a reason for hiding this comment

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

YESSS I know..... well at least autoprops makes it easier in both ways (Prop struct creation + destructuring)

@cecton cecton marked this pull request as ready for review November 17, 2023 08:21
github-actions[bot]
github-actions bot previously approved these changes Nov 17, 2023
@cecton
Copy link
Member Author

cecton commented Nov 17, 2023

@yewstack/yew another review also here please.

github-actions[bot]
github-actions bot previously approved these changes Nov 19, 2023
cecton added a commit to yewstack/yew-autoprops that referenced this pull request Dec 4, 2023
@cecton cecton dismissed futursolo’s stale review December 22, 2023 06:13

old review, the code has been moved to yew-autoprops

github-actions[bot]
github-actions bot previously approved these changes Dec 22, 2023
@cecton
Copy link
Member Author

cecton commented Dec 22, 2023

@yewstack/yew another review also here please.

This is taking too long. Since this PR is not touching anything critical, not even code, I will be merging soon

@cecton cecton merged commit b25703a into yewstack:master Dec 22, 2023
21 checks passed
@cecton cecton deleted the autoprops branch December 22, 2023 07:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants