-
-
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
Separate Yew Platform #2893
Separate Yew Platform #2893
Conversation
Visit the preview URL for this PR (updated for commit 680590b): https://yew-rs-api--pr2893-platform-crate-ulfy6xeo.web.app (expires Wed, 26 Oct 2022 11:47:54 GMT) 🔥 via Firebase Hosting GitHub Action 🌎 |
Benchmark - SSRYew Master
Pull Request
|
Size Comparison
✅ None of the examples has changed their size significantly. |
packages/yew-platform/Cargo.toml
Outdated
@@ -0,0 +1,40 @@ | |||
[package] | |||
name = "yew-platform" |
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.
What if we dropped the yew-
prefix from here? It can be generic crate that can be used to pick an async runtime between tokio and wasm-bindgen-futures (JS promises)
Name idea: promise + tokio = prokio
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.
Sounds good. I have published the crate under name prokio
.
# Conflicts: # packages/yew/Cargo.toml
@@ -26,7 +26,6 @@ | |||
//! - `csr`: Enables Client-side Rendering support and [`Renderer`]. Only enable this feature if you | |||
//! are making a Yew application (not a library). | |||
//! - `ssr`: Enables Server-side Rendering support and [`ServerRenderer`]. | |||
//! - `tokio`: Enables future-based APIs on non-wasm32 targets with tokio runtime. |
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.
Both tokio
on wasi
with threading support and async-std
's spawn_local
are unstable.
tokio
and wasm-bindgen-futures
are going to be the only runtime that is supported by prokio
for the moment.
Hence, I feel it might be better to forego the tokio
feature until we need it.
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 like this this way. Tokio is hidden behind the pseudo-runtime (prokio) and doesn't need to be exposed here.
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 to me.
I'm in favor of the removed tokio feature. I would prefer if the new crates were placed under the yewstack
organization though, just like how implicit-clone
is
cargo clippy --release --no-default-features --features tokio -- --deny=warnings | ||
cargo clippy --release --no-default-features --features csr,tokio -- --deny=warnings | ||
cargo clippy --release --no-default-features --features default,tokio -- --deny=warnings | ||
cargo clippy --release --no-default-features --features csr,default,tokio -- --deny=warnings | ||
cargo clippy --release --no-default-features --features hydration,tokio -- --deny=warnings | ||
cargo clippy --release --no-default-features --features default,hydration,tokio -- --deny=warnings | ||
cargo clippy --release --no-default-features --features ssr,tokio -- --deny=warnings | ||
cargo clippy --release --no-default-features --features csr,ssr,tokio -- --deny=warnings | ||
cargo clippy --release --no-default-features --features default,ssr,tokio -- --deny=warnings | ||
cargo clippy --release --no-default-features --features csr,default,ssr,tokio -- --deny=warnings | ||
cargo clippy --release --no-default-features --features hydration,ssr,tokio -- --deny=warnings | ||
cargo clippy --release --no-default-features --features default,hydration,ssr,tokio -- --deny=warnings |
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 assume they move to prokio, correct?
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 is removed completely and does not present in prokio as well. As previously mentioned, the tokio feature is not very useful. I expect the current situation to stay years before it changes. So I guess it might be better to remove tokio feature and read it when we need it.
#[doc(inline)] | ||
pub use prokio::*; |
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 think you can put this in lib.rs to have the same effect:
#[doc(inline)]
pub use prokio as platform;
That way you can avoid duplicating the doc comments.
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 intentional. The documentation of yew::platform and prokio is slightly different. The wording is adjusted to mention Yew in the documentation.
@@ -26,7 +26,6 @@ | |||
//! - `csr`: Enables Client-side Rendering support and [`Renderer`]. Only enable this feature if you | |||
//! are making a Yew application (not a library). | |||
//! - `ssr`: Enables Server-side Rendering support and [`ServerRenderer`]. | |||
//! - `tokio`: Enables future-based APIs on non-wasm32 targets with tokio runtime. |
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 like this this way. Tokio is hidden behind the pseudo-runtime (prokio) and doesn't need to be exposed here.
[target.'cfg(target_arch = "wasm32")'.dependencies] | ||
wasm-bindgen-futures = "0.4" |
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 do we still need a direct dependency on this?
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 yew is using wasm_bindgen_futures::JsFuture in a couple places.
I have moved prokio to https://github.com/yewstack/prokio. |
Description
Fixes #2839.
prokio
(exported asyew::platform
) has been published separately.pinned
(previouslyyew::platform::pinned
) has been published separately.tokio
has been removed.Checklist