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

Update signature of use_prepared_state/use_transitive_state #3376

Merged
merged 9 commits into from
Aug 19, 2023

Conversation

ranile
Copy link
Member

@ranile ranile commented Aug 14, 2023

Description

Update signature of use_prepared_state and use_transitive_state to take deps before closure.

Fixes #3371
Ref #3372 (review)

Checklist

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

@ranile ranile added breaking change A-yew Area: The main yew crate A-yew-macro Area: The yew-macro crate labels Aug 14, 2023
@ranile ranile requested a review from futursolo August 14, 2023 12:16
github-actions[bot]
github-actions bot previously approved these changes Aug 14, 2023
github-actions[bot]
github-actions bot previously approved these changes Aug 14, 2023
@github-actions
Copy link

github-actions bot commented Aug 14, 2023

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
async_clock 102.340 102.340 0 0.000%
boids 175.517 175.517 0 0.000%
communication_child_to_parent 95.229 95.229 0 0.000%
communication_grandchild_with_grandparent 108.888 108.888 0 0.000%
communication_grandparent_to_grandchild 105.550 105.550 0 0.000%
communication_parent_to_child 92.698 92.698 0 0.000%
contexts 110.887 110.887 0 0.000%
counter 89.207 89.207 0 0.000%
counter_functional 89.923 89.923 0 0.000%
dyn_create_destroy_apps 92.238 92.238 0 0.000%
file_upload 103.541 103.541 0 0.000%
function_memory_game 174.314 174.314 0 0.000%
function_router 352.123 352.123 0 0.000%
function_todomvc 163.349 163.349 0 0.000%
futures 227.711 227.711 0 0.000%
game_of_life 112.298 112.298 0 0.000%
immutable 188.932 188.932 0 0.000%
inner_html 86.013 86.013 0 0.000%
js_callback 112.990 112.990 0 0.000%
keyed_list 201.201 201.201 0 0.000%
mount_point 89.207 89.207 0 0.000%
nested_list 114.487 114.487 0 0.000%
node_refs 96.164 96.164 0 0.000%
password_strength 1582.842 1582.842 0 0.000%
portals 98.234 98.234 0 0.000%
router 318.130 318.130 0 0.000%
simple_ssr 143.645 143.645 0 0.000%
ssr_router 389.346 389.346 0 0.000%
suspense 118.708 118.708 0 0.000%
timer 91.757 91.757 0 0.000%
timer_functional 100.296 100.296 0 0.000%
todomvc 143.696 143.696 0 0.000%
two_apps 89.915 89.915 0 0.000%
web_worker_fib 154.516 154.516 0 0.000%
webgl 88.478 88.478 0 0.000%

✅ None of the examples has changed their size significantly.

@github-actions
Copy link

github-actions bot commented Aug 14, 2023

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

https://yew-rs--pr3376-ssr-hooks-sig-lnvl6y0o.web.app

(expires Fri, 25 Aug 2023 20:06:27 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

github-actions[bot]
github-actions bot previously approved these changes Aug 14, 2023
github-actions[bot]
github-actions bot previously approved these changes Aug 14, 2023
@github-actions
Copy link

github-actions bot commented Aug 14, 2023

Benchmark - SSR

Yew Master

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 338.769 339.876 339.289 0.378
Hello World 10 614.552 617.152 615.707 0.697
Function Router 10 2016.753 2032.644 2023.423 5.555
Concurrent Task 10 1006.593 1008.242 1007.700 0.538
Many Providers 10 1430.682 1460.267 1446.680 8.627

Pull Request

Benchmark Round Min (ms) Max (ms) Mean (ms) Standard Deviation
Baseline 10 338.978 339.999 339.454 0.360
Hello World 10 619.083 620.321 619.439 0.371
Function Router 10 1996.675 2009.961 2003.540 4.260
Concurrent Task 10 1006.743 1008.385 1007.584 0.625
Many Providers 10 1447.380 1465.975 1452.248 5.510

github-actions[bot]
github-actions bot previously approved these changes Aug 14, 2023
Copy link
Member

@futursolo futursolo left a comment

Choose a reason for hiding this comment

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

I only commented on the use_prepared_state tests, but the use_transitive_state tests should also have similar issues.

Comment on lines 35 to 43
error: You must specify a return type for this closure. This is used when the closure is omitted from the client side rendering bundle.
--> tests/hook_macro/use_prepared_state-fail.rs:21:41
error: expected closure
--> tests/hook_macro/use_prepared_state-fail.rs:21:58
|
21 | use_prepared_state_without_closure!(|_| { todo!() }, 123)?;
| ^^^^^^^^^^^^^^^
| ^^^
Copy link
Member

Choose a reason for hiding this comment

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

Similar to above, this testing is asserting that the closure has specified a return type.

packages/yew/tests/use_prepared_state.rs Outdated Show resolved Hide resolved
cecton
cecton previously approved these changes Aug 18, 2023
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.

LGTM

@ranile ranile dismissed stale reviews from cecton and github-actions[bot] via 07804c9 August 18, 2023 19:56
github-actions[bot]
github-actions bot previously approved these changes Aug 18, 2023
@ranile ranile requested a review from futursolo August 18, 2023 19:59
Copy link
Member

@futursolo futursolo left a comment

Choose a reason for hiding this comment

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

Thanks!
Looks good to me.

@futursolo futursolo merged commit 42e1890 into yewstack:master Aug 19, 2023
22 checks passed
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 A-yew-macro Area: The yew-macro crate breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hooks have inconsistent call signature
3 participants