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

Evaluate props in the order they're defined #2887

Merged
merged 5 commits into from Oct 8, 2022

Conversation

hamza1311
Copy link
Member

Description

Evaluate props in the order they're defined and document this behavior

Fixes #1634

Checklist

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

@hamza1311 hamza1311 added the A-yew-macro Area: The yew-macro crate label Sep 25, 2022
@hamza1311 hamza1311 added this to the v0.20 milestone Sep 25, 2022
github-actions[bot]
github-actions bot previously approved these changes Sep 25, 2022
@github-actions
Copy link

github-actions bot commented Sep 25, 2022

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

https://yew-rs--pr2887-prop-order-bwojsxjn.web.app

(expires Sun, 02 Oct 2022 16:01:48 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

github-actions bot commented Sep 25, 2022

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 375.109 381.707 375.905 2.042
Hello World 10 643.664 668.767 647.396 7.727
Function Router 10 2346.768 2377.796 2365.758 9.966
Concurrent Task 10 1007.755 1008.765 1008.474 0.322

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 393.407 398.120 394.325 1.433
Hello World 10 653.088 655.374 653.996 0.643
Function Router 10 2233.151 2275.302 2245.546 12.712
Concurrent Task 10 1006.515 1013.180 1008.750 1.705

@github-actions
Copy link

github-actions bot commented Sep 25, 2022

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
boids 170.354 170.329 -0.025 -0.015%
communication_child_to_parent 90.508 90.508 0 0.000%
communication_grandchild_with_grandparent 105.375 105.371 -0.004 -0.004%
communication_grandparent_to_grandchild 101.263 101.260 -0.003 -0.003%
communication_parent_to_child 87.645 87.647 +0.003 +0.003%
contexts 107.789 107.790 +0.001 +0.001%
counter 85.521 85.521 +0.001 +0.001%
counter_functional 86.049 86.050 +0.001 +0.001%
dyn_create_destroy_apps 88.425 88.427 +0.002 +0.002%
file_upload 100.101 100.101 0 0.000%
function_memory_game 163.271 163.253 -0.019 -0.011%
function_router 347.940 347.950 +0.010 +0.003%
function_todomvc 158.063 158.048 -0.016 -0.010%
futures 222.397 222.396 -0.001 -0.000%
game_of_life 105.067 105.067 0 0.000%
immutable 181.739 181.736 -0.003 -0.002%
inner_html 82.388 82.387 -0.001 -0.001%
js_callback 111.419 111.418 -0.001 -0.001%
keyed_list 195.637 195.632 -0.005 -0.002%
mount_point 85.142 85.139 -0.003 -0.003%
nested_list 111.961 111.958 -0.003 -0.003%
node_refs 93.040 93.041 +0.001 +0.001%
password_strength 1548.270 1548.268 -0.002 -0.000%
portals 96.330 96.335 +0.005 +0.005%
router 317.657 317.639 -0.019 -0.006%
simple_ssr 152.245 152.249 +0.004 +0.003%
ssr_router 393.523 393.511 -0.013 -0.003%
suspense 109.150 109.143 -0.008 -0.007%
timer 88.337 88.337 0 0.000%
todomvc 139.156 139.155 -0.001 -0.001%
two_apps 86.135 86.134 -0.001 -0.001%
web_worker_fib 152.450 152.449 -0.001 -0.001%
webgl 84.828 84.828 0 0.000%

✅ None of the examples has changed their size significantly.

@hamza1311 hamza1311 enabled auto-merge (squash) September 25, 2022 15:58
Copy link
Member

@WorldSEnder WorldSEnder left a comment

Choose a reason for hiding this comment

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

Wow, kind of a big and important change. One question before I approve: this doesn't change the order in which the special attributes and different kinds are executed.

I.e. in components, props are constructed before children, always.
In elements, the order is normal attributes, boolean attributes, class, listeners, children.

Is it confusing that there is still some kind of ordering going on, and these different kinds of attributes are not interleaved?

@hamza1311
Copy link
Member Author

That's a good point. I tested with props macro but I should also add a test with html macro.

The evaluation is happening of all props, except children. Order doesn't matter for ref. Not sure if we shouldn't document it at all or describe that children are always last.

I think that execution "in order they're defined" works for children too since children are always specified at the end of props list

Copy link
Member

@WorldSEnder WorldSEnder left a comment

Choose a reason for hiding this comment

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

I think it's fine for now to have this components and figure out how to do it for elements later. children being evaluated last is probably also expected, I don't recall explicitly specifying them as a prop instead of as html being supported behaviour.

@hamza1311 hamza1311 merged commit 426a1fd into yewstack:master Oct 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-yew-macro Area: The yew-macro crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Define prop execution order
2 participants