-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Polled SSR Stream #2824
Polled SSR Stream #2824
Conversation
# Conflicts: # packages/yew/src/virtual_dom/vlist.rs
Visit the preview URL for this PR (updated for commit a78f0b3): https://yew-rs-api--pr2824-polled-stream-x9xginht.web.app (expires Sun, 11 Sep 2022 02:17:22 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
Benchmark - SSRYew Master
Pull Request
|
Size Comparison
✅ None of the examples has changed their size significantly. |
# Conflicts: # packages/yew/src/server_renderer.rs
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.
Looks good, sorry it took me a while to get to it
tracing = "0.1.36" | ||
pin-project = "1.0.11" |
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.
pin-project-lite
maybe?
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.
However, if you already have proc-macro related dependencies in your crate’s dependency graph, there is no benefit from using this crate.
The primary benefit of #[pin_project]
is that rustfmt will continue to work for the struct.
state: BufStreamState, | ||
|
||
// This type is not send or sync. | ||
_marker: PhantomData<Rc<()>>, |
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.
Why not do:
impl !Send for Inner {}
impl !Sync for Inner {}
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.
This is due to that negative impls is not stable.
See: https://doc.rust-lang.org/unstable-book/language-features/negative-impls.html
where | ||
F: Future<Output = ()>, |
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.
Perhaps remove the bound from here? That would mean that everywhere this type is named doesn't require the bound - only the usages do
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 cannot remove this as this bound is declared on the MaybeDone type itself.
@@ -1,103 +0,0 @@ | |||
//! This module contains types for I/O functionality. |
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.
It seems that this file was moved, rather than deleted. git mv
would be better in that case since that preserves the git history
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.
fmt
is a complete rewrite of the buffer and not connected to the previous implementation.
I tried to interactively rebase the changes and is having conflicts with every commits after the commit that removed io.rs.
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, I didn't see the review notifications. Looks good!
Description
This pull request introduces a polled SSR stream instead of using channels.
Compared to channels, Polled Streams:
Checklist