-
Notifications
You must be signed in to change notification settings - Fork 124
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
Support wasm32-unknown-unknown architecture #458
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,7 +36,7 @@ base64 = "0.21.2" | |
chrono = { version = "0.4.19", features = ["serde"] } | ||
dotenvy = { version = "0.15.0", optional = true } | ||
futures = { version = "0.3.17", optional = true } | ||
getrandom = "0.2.3" | ||
|
||
log = "0.4.14" | ||
maybe-async = "0.2.6" | ||
serde = { version = "1.0.130", default-features = false } | ||
|
@@ -46,10 +46,23 @@ thiserror = "1.0.29" | |
url = "2.2.2" | ||
webbrowser = { version = "0.8.0", optional = true } | ||
|
||
[target.'cfg(target_arch = "wasm32")'.dependencies] | ||
getrandom = { version = "0.2.3", features = ["js"] } | ||
|
||
[target.'cfg(not(target_arch = "wasm32"))'.dependencies] | ||
getrandom = "0.2.3" | ||
|
||
[dev-dependencies] | ||
env_logger = { version = "0.11.0", default-features = false } | ||
tokio = { version = "1.11.0", features = ["rt-multi-thread", "macros"] } | ||
futures-util = "0.3.17" | ||
wasm-bindgen-test = "0.3.34" | ||
|
||
[target.'cfg(not(target_arch = "wasm32"))'.dev-dependencies] | ||
tokio = { version = "1.11.0", features = ["rt-multi-thread", "macros"] } | ||
|
||
[target.'cfg(target_arch = "wasm32")'.dev-dependencies] | ||
tokio = { version = "1.11.0", features = ["rt", "macros"] } | ||
dotenvy_macro = { version = "0.15.7" } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Used to read There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does it mean Shouldn't we add a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct, From my understanding of cargo the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For comparison, here's the output with wasm:
|
||
|
||
[features] | ||
default = ["client-reqwest", "reqwest-default-tls"] | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,8 @@ | |
use super::{BaseHttpClient, Form, Headers, Query}; | ||
|
||
use std::convert::TryInto; | ||
|
||
#[cfg(not(target_arch = "wasm32"))] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added to remove a warning on unused imports |
||
use std::time::Duration; | ||
|
||
use maybe_async::async_impl; | ||
|
@@ -56,6 +58,7 @@ pub struct ReqwestClient { | |
client: reqwest::Client, | ||
} | ||
|
||
#[cfg(not(target_arch = "wasm32"))] | ||
impl Default for ReqwestClient { | ||
fn default() -> Self { | ||
let client = reqwest::ClientBuilder::new() | ||
|
@@ -67,6 +70,17 @@ impl Default for ReqwestClient { | |
} | ||
} | ||
|
||
#[cfg(target_arch = "wasm32")] | ||
impl Default for ReqwestClient { | ||
fn default() -> Self { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. reqwest doesn't implement the |
||
let client = reqwest::ClientBuilder::new() | ||
.build() | ||
// building with these options cannot fail | ||
.unwrap(); | ||
Self { client } | ||
} | ||
} | ||
|
||
impl ReqwestClient { | ||
async fn request<D>( | ||
&self, | ||
|
@@ -109,7 +123,8 @@ impl ReqwestClient { | |
} | ||
} | ||
|
||
#[async_impl] | ||
#[cfg_attr(target_arch = "wasm32", async_impl(?Send))] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm wondering is there any way to encapsulate these There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was wondering the same thing. I found in the rust docs that the I've never written custom macros yet but could experiment if you think it's a good idea. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Custom macros is not an ideal idea for me, it will also increase complexity and mental effort to maintain the codebase. |
||
#[cfg_attr(not(target_arch = "wasm32"), async_impl)] | ||
impl BaseHttpClient for ReqwestClient { | ||
type Error = ReqwestError; | ||
|
||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This stumped me the most. The async runtime in wasm doesn't support At first I tried to extract the trait bounds into "aliases" with the goal of annotating them with |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
//! Asynchronous implementation of automatic pagination requests. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need a separate There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like my previous comment dissapeared, I explained it here: #458 (comment) Basically I didn't find a way of encapsulating the |
||
|
||
use crate::{model::Page, ClientResult}; | ||
|
||
use std::pin::Pin; | ||
|
||
use futures::{future::Future, stream::Stream}; | ||
|
||
/// Alias for `futures::stream::Stream<Item = T>`, since async mode is enabled. | ||
pub type Paginator<'a, T> = Pin<Box<dyn Stream<Item = T> + 'a>>; | ||
|
||
pub type RequestFuture<'a, T> = Pin<Box<dyn 'a + Future<Output = ClientResult<Page<T>>>>>; | ||
|
||
/// This is used to handle paginated requests automatically. | ||
pub fn paginate_with_ctx<'a, Ctx: 'a, T, Request>( | ||
ctx: Ctx, | ||
req: Request, | ||
page_size: u32, | ||
) -> Paginator<'a, ClientResult<T>> | ||
where | ||
T: 'a + Unpin, | ||
Request: 'a + for<'ctx> Fn(&'ctx Ctx, u32, u32) -> RequestFuture<'ctx, T>, | ||
{ | ||
use async_stream::stream; | ||
let mut offset = 0; | ||
Box::pin(stream! { | ||
loop { | ||
let request = req(&ctx, page_size, offset); | ||
let page = request.await?; | ||
offset += page.items.len() as u32; | ||
for item in page.items { | ||
yield Ok(item); | ||
} | ||
if page.next.is_none() { | ||
break; | ||
} | ||
} | ||
}) | ||
} | ||
|
||
pub fn paginate<'a, T, Fut, Request>(req: Request, page_size: u32) -> Paginator<'a, ClientResult<T>> | ||
where | ||
T: 'a + Unpin, | ||
Fut: Future<Output = ClientResult<Page<T>>>, | ||
Request: 'a + Fn(u32, u32) -> Fut, | ||
{ | ||
use async_stream::stream; | ||
let mut offset = 0; | ||
Box::pin(stream! { | ||
loop { | ||
let request = req(page_size, offset); | ||
let page = request.await?; | ||
offset += page.items.len() as u32; | ||
for item in page.items { | ||
yield Ok(item); | ||
} | ||
if page.next.is_none() { | ||
break; | ||
} | ||
} | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -108,6 +108,21 @@ | |
//! the [`.env` file](https://github.com/ramsayleung/rspotify/blob/master/.env) | ||
//! for more details. | ||
//! | ||
//! ### WebAssembly | ||
//! | ||
//! RSpotify supports the `wasm32-unknown-unknown` target in combination | ||
//! with the `client-reqwest` feature. HTTP requests must be processed async. | ||
//! Other HTTP client configurations are not supported. | ||
//! | ||
//! [Spotify recommends][spotify-auth-flows] using [`AuthCodePkceSpotify`] for | ||
//! authorization flows on the web. | ||
//! | ||
//! Importing the Client ID via `RSPOTIFY_CLIENT_ID` is not possible since WASM | ||
//! web runtimes are isolated from the host environment. The client ID must be | ||
//! passed explicitly to [`Credentials::new_pkce`]. Alternatively, it can be | ||
//! embedded at compile time with the [`std::env!`] or | ||
//! [`dotenv!`](https://crates.io/crates/dotenvy) macros. | ||
//! | ||
//! ### Examples | ||
//! | ||
//! There are some [available examples on the GitHub | ||
|
@@ -442,11 +457,13 @@ impl OAuth { | |
} | ||
|
||
#[cfg(test)] | ||
mod test { | ||
pub mod test { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
use crate::{alphabets, generate_random_string, Credentials}; | ||
use std::collections::HashSet; | ||
use wasm_bindgen_test::*; | ||
|
||
#[test] | ||
#[wasm_bindgen_test] | ||
fn test_generate_random_string() { | ||
let mut containers = HashSet::new(); | ||
for _ in 1..101 { | ||
|
@@ -456,6 +473,7 @@ mod test { | |
} | ||
|
||
#[test] | ||
#[wasm_bindgen_test] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A naive question, what does There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The regualar
I believe the macro exports the tests into a separate WASM module to make it easier to run in a browser. |
||
fn test_basic_auth() { | ||
let creds = Credentials::new_pkce("ramsay"); | ||
let headers = creds.auth_headers(); | ||
|
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.
Strictly speaking tokio isn't needed for running wasm tests, but to removing it would have made the
#[cfg...
attributes on tests more complex