-
Notifications
You must be signed in to change notification settings - Fork 320
typespec_client_core: New web_runtime to support wasm32-unknown-unknown
#2770
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
…` when target wasm The `typespec_client_core::sleep::sleep` is using the async_runtime's sleep implementation, which is complicated to support `wasm32`, and will panic now when targeting to `wasm32`. Instead of make the `standard_runtime` to support wasm32 sleep (I wonder if it's feasible), we can instead just introduce another `sleep` implementation to simply use the `gloo_timers::future::sleep`. Also, this PR adds a `cfg_attr` for conditionally remove the `Send` trait from the `async_trait` for the `RetryPolicy`, as the other parts of the code do. Additionally, the `getrandom` crate will enable `wasm_js` features when targetting to `wasm`, which is necessary.
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.
Pull Request Overview
This PR adds a WebAssembly-compatible sleep implementation using gloo_timers, adjusts async_trait attributes for WASM targets, and updates dependency declarations.
- Introduce a
wasm-onlysleepusinggloo_timers::future::sleep - Add
cfg_attrmacros to toggleasync_traitforRetryPolicyonwasm32 - Update
Cargo.tomlfiles to includegloo-timersand necessary features
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| sdk/typespec/typespec_client_core/src/sleep.rs | Split sleep into non-WASM and WASM variants using cfgs |
| sdk/typespec/typespec_client_core/src/http/policies/retry/mod.rs | Apply cfg_attr to async_trait for WASM vs non-WASM targets |
| sdk/typespec/typespec_client_core/Cargo.toml | Add gloo-timers to WASM target dependencies |
| Cargo.toml | Declare gloo-timers with the futures feature at workspace root |
Comments suppressed due to low confidence (1)
sdk/typespec/typespec_client_core/src/sleep.rs:36
- The WASM variant of
sleepis missing a doc comment; consider adding one to explain its behavior and any conversion caveats.
pub async fn sleep(duration: Duration) {
sleep when target wasmweb_runtime to support wasm32
web_runtime to support wasm32web_runtime to support wasm32-unknown-unknown
heaths
left a comment
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.
Better, but a couple lingering changes.
| /// | ||
| /// Uses a simple thread based implementation for sleep. A more efficient | ||
| /// implementation is available by using the `tokio` crate feature. | ||
| #[cfg_attr(target_arch = "wasm32", allow(unused_variables))] |
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.
Seems this line is no longer needed.
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.
If we want to do #2770 (comment), this is still needed to allow a user to select the StdRuntime even target to wasm32.
|
|
||
| fn create_async_runtime() -> Arc<dyn AsyncRuntime> { | ||
| #[cfg(not(feature = "tokio"))] | ||
| #[cfg(target_arch = "wasm32")] |
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.
Much better, but IMHO this should probably be tied to a specific feature which is the gloo runtime, unless you can assert safely that gloo is the runtime of choice for all wasm developers (I'm not familiar enough with wasm development to have an opinion here).
This is similar to how we've tied the tokio runtime to the tokio feature - because the tokio runtime is not a globally accepted async runtime, we hide its implementation behind a feature.
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.
@LarryOsterman The gloo-timers is provided by https://github.com/rustwasm, which is built on top of wasm-bindgen and js-sys, providing a more user friendly API. So if we really want to introduce a feature, then I'd prpose to name it "wasm-bindgen(as thespawnis based onwasm-bindgen-futures, and the sleepis based ongloo-timers, which in turn based on wasm-bindgen`).
Whilst, I believe even we use the target for this create_async_runtime(), it only means we provide a default runtime for the users. They are still able to call set_async_runtime() to choose the runtime of their choice. Otherwise, if we do:
#[cfg(feature = "wasm-bindgen")]
{
Arc::new(web_runtime::WasmBindgenRuntime) as Arc<dyn AsyncRuntime>
}
#[cfg(feature = "tokio")]
{
Arc::new(tokio_runtime::TokioRuntime) as Arc<dyn AsyncRuntime>
}
#[cfg(not(any(feature = "tokio", feature = "wasm-bindgen")))]
{
Arc::new(standard_runtime::StdRuntime)
}
For users who target to wasm32-unknown-unknown, without specifying the wasm-bindgen feature, then they'll get a StdRuntime by default, which will only panic in any case.
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 can totally get behind a wasm-bindgen feature name.
Thank you for your patience 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.
@LarryOsterman Sure, I've pushed a new commit to introduce that feature name, made the wasm dependencies optional, and only bring them by this new feature.
|
Kindly ping @LarryOsterman |
Thank you for the pin. I have to finish the July SDK release and after that, I'll get to this PR. Overall it looks great |
|
/azp run rust - pullrequest |
|
Azure Pipelines successfully started running 1 pipeline(s). |
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
|
There's a single outstanding CI error: If you could please fix this, I'll get this committed. |
|
Can we get a manual test checked in? Either a WASI that does something simple using our SDK (to make sure our SDK isn't elided)? It can be a manual test (example) ideally that returns 0 for success of anything else for an error, and we can automate it later. This is all looking pretty good at a high-level (I need to review more thoroughly), but unless we're actually exercising it we're only getting build validation. It'd be nice to have the option - at least manually run - to actually verify it works. |
|
@heaths Sure, I'll work on a test for this. |
|
@heaths and @LarryOsterman, the tests are ready at my own branch, which I've created another PR (since I can't push to this branch for some reason): #2838. Close this PR in favor of #2838 |
…nown (#2838) The `typespec_client_core::sleep::sleep` is using the async_runtime's sleep implementation, which is complicated to support `wasm32`, and will panic now when targeting to `wasm32`. Instead of make the `standard_runtime` to support wasm32 sleep (I wonder if it's feasible), we can instead just introduce another `sleep` implementation to simply use the `gloo_timers::future::sleep`. Also, this PR adds a `cfg_attr` for conditionally remove the `Send` trait from the `async_trait` for the `RetryPolicy`, as the other parts of the code do. Fix #2754 Supersedes: #2770 Co-authored-by: Larry Osterman <LarryOsterman@users.noreply.github.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Heath Stewart <heaths@microsoft.com>
The
typespec_client_core::sleep::sleepis using the async_runtime's sleep implementation, which is complicated to supportwasm32, and will panic now when targeting towasm32. Instead of make thestandard_runtimeto support wasm32 sleep (I wonder if it's feasible), we can instead just introduce anothersleepimplementation to simply use thegloo_timers::future::sleep.Also, this PR adds a
cfg_attrfor conditionally remove theSendtrait from theasync_traitfor theRetryPolicy, as the other parts of the code do.Additionally, thegetrandomcrate will enablewasm_jsfeatures when targetting towasm, which is necessary.Fix #2754