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

use_prepared_state & use_transitive_state #2650

Merged
merged 38 commits into from May 24, 2022

Conversation

futursolo
Copy link
Member

@futursolo futursolo commented Apr 30, 2022

Description

This pull request implements 2 hooks that allows states created during SSR to be carried over to the client side for hydration.

See: #2649

Please refer to the API docs for how to use these hooks:

About the implementation

I have tested the following implementation for data serialisation:

  • bincode + base64
  • serde_json
  • serde_qs

The bincode + base64 option results in the smallest hydration bundle and html artifact size overall when tested with a combination of multiple data types.

Checklist

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

github-actions[bot]
github-actions bot previously approved these changes Apr 30, 2022
github-actions[bot]
github-actions bot previously approved these changes Apr 30, 2022
@github-actions
Copy link

github-actions bot commented Apr 30, 2022

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

https://yew-rs-api--pr2650-fc-prepared-state-nb5wk40u.web.app

(expires Mon, 30 May 2022 23:44:04 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@github-actions
Copy link

github-actions bot commented Apr 30, 2022

Size Comparison

examples master (KB) pull request (KB) diff (KB) diff (%)
boids 172.826 172.869 +0.043 +0.025%
contexts 109.687 109.688 +0.001 +0.001%
counter 86.654 86.654 0 0.000%
counter_functional 87.305 87.305 0 0.000%
dyn_create_destroy_apps 89.743 89.770 +0.026 +0.029%
file_upload 102.621 102.622 +0.001 +0.001%
function_memory_game 167.261 167.294 +0.033 +0.020%
function_router 350.257 350.357 +0.101 +0.029%
function_todomvc 161.972 161.969 -0.003 -0.002%
futures 226.658 226.658 0 0.000%
game_of_life 107.538 107.539 +0.001 +0.001%
inner_html 83.688 83.688 0 0.000%
js_callback 112.919 112.914 -0.005 -0.004%
keyed_list 195.098 195.100 +0.002 +0.001%
mount_point 86.281 86.280 -0.001 -0.001%
nested_list 115.914 115.913 -0.001 -0.001%
node_refs 90.524 90.522 -0.002 -0.002%
password_strength 1539.692 1539.692 0 0.000%
portals 97.262 97.257 -0.005 -0.005%
router 319.346 319.448 +0.103 +0.032%
simple_ssr 494.414 153.534 -340.880 -68.946%
ssr_router 425.838 394.871 -30.967 -7.272%
suspense 110.562 110.613 +0.052 +0.047%
timer 89.363 89.393 +0.029 +0.033%
todomvc 143.055 143.054 -0.001 -0.001%
two_apps 87.287 87.289 +0.002 +0.002%
web_worker_fib 153.531 153.539 +0.008 +0.005%
webgl 87.375 87.373 -0.002 -0.002%

⚠️ The following examples have changed their size significantly:

examples master (KB) pull request (KB) diff (KB) diff (%)
simple_ssr 494.414 153.534 -340.880 -68.946%
ssr_router 425.838 394.871 -30.967 -7.272%

github-actions[bot]
github-actions bot previously approved these changes Apr 30, 2022
@futursolo futursolo added A-examples Area: The examples A-yew Area: The main yew crate A-yew-macro Area: The yew-macro crate labels Apr 30, 2022
@futursolo futursolo requested a review from hamza1311 May 22, 2022 06:02
@futursolo
Copy link
Member Author

futursolo commented May 22, 2022

@hamza1311 Could you please give this pull request a review so the second part in #2649 can proceed forward?

hamza1311
hamza1311 previously approved these changes May 22, 2022
Copy link
Member

@hamza1311 hamza1311 left a comment

Choose a reason for hiding this comment

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

Sorry, it took me a little while to get to this. The implementation looks good. While not blocking, it would be nice to address these comments before merging

}

impl PreparedState {
// Async closure is not stable, so we rewrite it to clsoure + async block
Copy link
Member

Choose a reason for hiding this comment

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

Another case of #2681

@@ -29,6 +29,10 @@ thiserror = "1.0"

futures = { version = "0.3", optional = true }
html-escape = { version = "0.2.9", optional = true }
base64ct = { version = "1.5.0", features = ["std"], optional = true }
bincode = { version = "1.3.3", optional = true }
Copy link
Member

Choose a reason for hiding this comment

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

Not blocking but it would be nice if we could change the implementation, similar to how the global allocator can be changed. If one wants to use JSON, BSON, etc for this, they should have the option to do so

Copy link
Member

Choose a reason for hiding this comment

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

We should create a separate issue for this. Can you create one when merging this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure.

packages/yew/Cargo.toml Outdated Show resolved Hide resolved
packages/yew-macro/src/lib.rs Show resolved Hide resolved
@futursolo futursolo dismissed stale reviews from hamza1311 and github-actions via 1f0b5e1 May 23, 2022 09:53
github-actions[bot]
github-actions bot previously approved these changes May 23, 2022
github-actions[bot]
github-actions bot previously approved these changes May 23, 2022
github-actions[bot]
github-actions bot previously approved these changes May 23, 2022
github-actions[bot]
github-actions bot previously approved these changes May 23, 2022
@futursolo futursolo requested a review from hamza1311 May 23, 2022 12:21
github-actions[bot]
github-actions bot previously approved these changes May 23, 2022
github-actions[bot]
github-actions bot previously approved these changes May 23, 2022
Copy link
Member

@hamza1311 hamza1311 left a comment

Choose a reason for hiding this comment

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

Other than the error messages, we're good to go

@@ -0,0 +1,55 @@
error: expected `|`
Copy link
Member

Choose a reason for hiding this comment

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

Can we make this "expected closure, found <whatever>"?

Comment on lines 13 to 17
error: expected a second argument as dependency
--> tests/hook_macro/use_prepared_state-fail.rs:10:5
|
10 | use_prepared_state_with_closure!(|_| -> u32 { todo!() })?;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep this similar to rustc's missing argument error:
error: this hook takes 2 arguments but 1 argument were supplied
with the span pointing at the missing argument.

Example from rustc:

error[E0061]: this function takes 2 arguments but 1 argument was supplied
--> src/main.rs:4:5
|
4 |     hook("123")
|     ^^^^ ----- supplied 1 argument
|     |
|     expected 2 arguments

Copy link
Member Author

@futursolo futursolo May 23, 2022

Choose a reason for hiding this comment

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

I don't think this is possible with syn::Error as you cannot emit an error in which its span is beyond its input (the hook name itself) unless Span::call_site() is used.

@@ -29,6 +29,10 @@ thiserror = "1.0"

futures = { version = "0.3", optional = true }
html-escape = { version = "0.2.9", optional = true }
base64ct = { version = "1.5.0", features = ["std"], optional = true }
bincode = { version = "1.3.3", optional = true }
Copy link
Member

Choose a reason for hiding this comment

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

We should create a separate issue for this. Can you create one when merging this PR?

@futursolo futursolo merged commit b29b453 into yewstack:master May 24, 2022
@futursolo futursolo deleted the fc-prepared-state branch December 15, 2022 10:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-examples Area: The examples A-yew Area: The main yew crate A-yew-macro Area: The yew-macro crate
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants