search

ramsayleung / rspotify

Spotify Web API SDK implemented on Rust
MIT License
632 stars 121 forks source link

Support wasm32-unknown-unknown architecture #458

Closed gelendir closed 6 months ago

gelendir commented 6 months ago

Description

Modifications needed to compile rspotify for a WebAssembly runtime. (i.e. cargo build --target wasm32-unknown-unknown)

Motivation and Context

I want to build an app that helps me better manage music when using spotify to DJ. I'd like to use rust + WASM to build it, however none of the maintained spotify crates support WASM.

@shiguma127 made an attempt in https://github.com/ramsayleung/rspotify/pull/293, but the PR was closed. This PR improves on what he started.

This is my most "complex" rust endeavor so far and I would be happy to get feedback on better ways of refactoring or rearchitecturing.

The code is now littered with a bunch of architecture conditionals that might be hard to maintain. I'm open to trying better alternatives if the maintainers don't like this.

One idea would be to make it easier to use the rspotify-http traits. One could implement the BaseHttpClient trait outside of this crate and then build a SpotifyClient<CustomHttpClient>. Feature flags would need to be refactored since this crate forces choosing a http client through the client-reqwest or client-ureq features.

Dependencies

The other dependencies are managed through Cargo.toml

Type of change

How has this been tested?

The #[wasm_bindgen_test] attribute has been added to all tests. They all pass when running wasm-pack test --node. The attribute doesn't conflict with #[test] (but it does add an extra dev-dependency)

wasm-pack depends on a WASM runtime to run tests. The 3 currently supported are:

NodeJS is the default, so that's what's configured in the CI job

Client ID and Client secret

Some of the tests depend on environment variables RSPOTIFY_CLIENT_ID and RSPOTIFY_CLIENT_SECRET. However, WASM does not support the concept of environment variables. As a workaround the variables are imported at compile time only when running tests.

Is this change properly documented?

I wasn't sure if wasm support warrants documentation or not. I'm happy to add documentation as part of this PR

ramsayleung commented 6 months ago

Thanks for your contribution, everything looks good to me now, is it ready to merge?

Furthermore, are you planning to add a section in the CHANGELOG to illustrate the support for wasm?

gelendir commented 6 months ago

Good to hear ! Yes I can add some docs and notes to the changelog. It's getting late over here but will push the docs to this PR tomorrow ?

gelendir commented 6 months ago

Documentation and CHANGELOG have been added. I didn't figureout how you manage version increments, so I guessed that this would be consifered a new feature and added version 1.3.0.

If the docs look good then I don't have any other changes planned and you can merge.

ramsayleung commented 6 months ago

LGTM, but some CI tasks failed.

gelendir commented 6 months ago

It seems to be a transient error with the spotify API, the test received a HTTP 504. I just tried running the CI on my repo and everything passes: https://github.com/gelendir/rspotify/actions/runs/8091405577

Could you try re-running the CI ?

ramsayleung commented 6 months ago

Merged :)