elastic-rs / elastic

An Elasticsearch REST API client for Rust
Apache License 2.0
253 stars 40 forks source link

Custom backends support #405

Open ColonelThirtyTwo opened 4 years ago

ColonelThirtyTwo commented 4 years ago

Allows external crates to write their own Sender implementations and use them with Client.

This involved refactoring a decent amount of the crate, and probably has some backwards-incompatible changes. I didn't change any of the usual APIs though; most code that just does querying should still work.

The primary impetus of this change is getting this crate to work in WebAssembly. In that environment, requests must be made via the XMLHttpRequest API, and this crate must return JS promises. I have written such a backend, which serves as an example; code is here: https://gitlab.com/colonelthirtytwo/elastic-stdweb-rs

Major changes off the top of my head, see commits for more details:

Fixes #281

ColonelThirtyTwo commented 4 years ago

FYI I haven't tested this too hardly yet, but I'm planning on working on a project that makes use of this crate on WASM, so that should exercise all the functionality.

mwilliammyers commented 4 years ago

Wow! Thanks for the PR! This is pretty big. So does this allow implementing/creating your own Sender as discussed in #281?

I tried updating to futures 0.3 yesterday and it is a doozy... I wonder if this could help? Would it be crazy if we just remove sync sender now that async/await is stable? I digress and I'll open a different issue to discuss further.

ColonelThirtyTwo commented 4 years ago

Yes, you can implement your own sender in your own crate. Example for wasm using stdweb: https://gitlab.com/colonelthirtytwo/elastic-stdweb-rs

IMO I'd rather keep syncsender around; tokio and the async stuff is a bit heavy and I would not be surprised if people would not want to include all that.

mwilliammyers commented 4 years ago

Any chance you can rebase this onto master so we can run the tests? I assume they might need to be updated too?

ColonelThirtyTwo commented 4 years ago

Could do. Just wanted a look-over of the code beforehand in case I had to change anything.

ColonelThirtyTwo commented 4 years ago

One issue I've noticed is that since send is now implemented on RequestBuilder, there is no documentation for send on the request type's pages, which isn't very helpful.

ColonelThirtyTwo commented 4 years ago

~...and now with the new version, cargo keeps pulling in reqwests even with default-features: false set... humm~ Nevermind my fault with a dependency.

mwilliammyers commented 4 years ago

I am just now starting to look more at this and I have a few initial questions:

  1. Am I to understand that when Generic Associated Types stabilizes we won't need both the Sender & TypedSender<TReqInner> traits? It will just be the Sender trait with an associated type that indicates the type of the sender (WASM vs async etc.)
  2. Is it worth adding your WASM implementation into this crate behind an off by default feature? It doesn't look that big/complicated and might be nice to have...?
  3. Should we make not including sync-sender the default and just make async-sender the default? reqwest is going this route with their next breaking change release (see: https://github.com/seanmonstar/reqwest/commit/7e3c1bc461dd51a9b59ca6b06b82e1699cc1a1ce). Our next release v0.21.0 will have a few breaking changes in it already. Granted this would be a much bigger breaking change but might be worth it? I guess it doesn't pull in any extra dependencies so it might not matter?

I'll add more to my review asap...

ColonelThirtyTwo commented 4 years ago
  1. Correct. The Sender trait could have a type InProgressResponse<Response>;, which is Result<Response, Error> for SyncSender, Pending<Response> for AsyncSender, and Promise<Response, Error> for StdwebSender. Then typed_send could be moved to the Sender trait and return Self::InProgressResposne<SomeResponseType>.
  2. I'd rather polish the stdweb sender a bit more then include it in a future PR. There are some things I'd like to change with it.
  3. Huh, didn't know that reqwests was turning off the blocking stuff by default now. Doesn't the async stuff require setting up an executor of some sort? Won't that be inconvenient for simple programs? Or did that change with async stabilization?
mwilliammyers commented 4 years ago
  1. Nice! That seems very clean.
  2. Makes perfect sense. 👌
  3. The alpha crate is setup like that and I would assume the final release will be too; I haven't seen any Indication otherwise. Yeah, it is kinda annoying to setup (although it should be as easy as a #[tokio::main] or #[runtime::main]—assuming the prerequisite dependencies are all downloaded and in order) and then using async/await. I bet their thinking is that while it is a bit more annoying, it kinda forces the ecosystem into using async I/O which is a benefit in performance and utility in the long run...? But I can't speak for the reqwest devs of course.