-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
use_prepared_state
& use_transitive_state
#2650
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
Conversation
# Conflicts: # packages/yew/src/html/component/scope.rs
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 🌎 |
Size Comparison
|
@hamza1311 Could you please give this pull request a review so the second part in #2649 can proceed forward? |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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 } |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure.
1f0b5e1
There was a problem hiding this 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 `|` |
There was a problem hiding this comment.
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>"?
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!() })?; | ||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 } |
There was a problem hiding this comment.
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?
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