search

sugyan / atrium

Rust libraries for Bluesky's AT Protocol services.
MIT License
218 stars 26 forks source link

refactor: remove async_trait crate due to increased MSRV #234

Closed oestradiol closed 2 months ago

oestradiol commented 2 months ago

This PR is due to #233 and should only be merged after that one is merged

oestradiol commented 2 months ago

I wonder if there's a better way to do this without having to repeat the method definitions for every async trait method. I'm not particularly fond of that, but it seems that you cannot add a #[cfg] directive inside a definition... However I imagine there's good reason for the Send trait to be relaxed for wasm, so I decided to keep it like it was to avoid breaking changes. @sugyan, if it is okay for me to ask why exactly is it done like that? Is there any specific difference on how Rust handles that on wasm?

sugyan commented 2 months ago

I don't know much about it, but I understand that JsValue is not-Send, so wasm_bindgen can't make it Send, and we need to switch it with target_arch, as in the current implementation. ref: https://github.com/rustwasm/wasm-bindgen/issues/2833

So I'm wondering if we could do something similar by using the trait-variant crate (I haven't implemented and tested it properly yet.) https://blog.rust-lang.org/2023/12/21/async-fn-rpit-in-traits.html#async-fn-in-public-traits https://crates.io/crates/trait-variant

Since there should be no need to have both a Sendable version and not, I think ATrium could still be implemented using target_arch, with Send only when it is not wasm32, as follows.

#[cfg_attr(not(target_arch = “wasm32”), trait_variant::make(Send))]
oestradiol commented 2 months ago

Force-pushed to resync the branch with #233.

oestradiol commented 2 months ago

Force-pushed again to re-do the commit for the wasm implementation, and again to format it.

@sugyan your suggestion worked wonderfully, trait-variant is really nice! I expanded the macros with cargo-expand and it did exactly what we needed. This helped clean up a lot of code. The only issue happened on send_xrpc. Self has to be Sync, and unfortunately Rust does not support #[cfg] on where clauses 😢. I don't know if there's much we can do about this one, though. But it shouldn't be a big problem considering that I refactored out the default implementation to an external method, which means that any wrong change to the signature of send_xrpc in XrpcClient should cause a compile-time error.

sugyan commented 2 months ago

It was nice to have a cleaner code!

As for send_xrpc, sure... As for methods that provide default implementations like this, I guess I'll have to treat them specially. I think your current implementation seems good enough.

oestradiol commented 2 months ago

Ok! I'll proceed with turning this into a proper PR when #233 is merged then.