search

sreeise / graph-rs-sdk

Microsoft Graph API Client And Identity Platform Client in Rust
MIT License
114 stars 30 forks source link

remove unused `tokio` features #452

Closed smndtrl closed 12 months ago

smndtrl commented 12 months ago

I discovered that graph-rs pulls in full tokio features yet doesn't really make use of any of those. This currently blocks usage in wasm32-wasi targets amongst some other crates I'm working with.

I ran build and test and all seems fine. CI will yell when I was wrong :)

sreeise commented 11 months ago

@smndtrl

I don't know that you were necessarily completely wrong... at least in the sense that it could be done. If someone were to just use the blocking part then you can get away with it on wasm, given that you change the features to align with that in this crate.

I would probably be concerned about what type of changes would have to be made to feature gate this correctly such that it would not cause a non-backwards compatible change and that it won't cause normal features that are used to no longer work without a feature being enabled. The non-backward compatible changes im ok with as long as the whole crate is bumped a major version but im not sure its worth a major version bump for just that. Possibly as part of other merges that would also cause a major version bump.

That being said, you can use target cfg flags to set whats disabled on wasm platform and set what is enabled on non-wasm platforms. Take a look at the reqwest crate cargo.toml for an example of this.

[target.'cfg(not(target_arch = "wasm32"))'.dependencies]
tokio = ...

As far as the features we are using heres what I could think of off the top of my head:

tokio features in dependencies: task, sync, fs

tokio features in dev-dependencies: rt, macros, time, test-utils

reqwest also enables certain features but they are already feature/cfg target gated for wasm like I mentioned.

If you look at the tokio docs it says that, at least for the dependencies. you can use task and sync and still be on wasm. But fs would cause a failure.

If you want to do the work to add target cfg scopes to this then feel free to reopen this.

BTW, the tests are not failing because of what you did. They are failing because your branch is a fork and GitHub doesn't allow forks access to repository secrets. Its an unfortunate issue that I wish GitHub had a solution for because it makes it more difficult for me as I have to run everyones PRs locally to test them on the full test suite.