Skip to content
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

[WIP] move from hyper to reqwest #125

Closed
wants to merge 1 commit into from

Conversation

alexkornitzer
Copy link

As per issue #110, I have decided to migrate rs-es from hyper to reqwest. Initially I started porting to the latest hyper version, but quickly found that we will just end up copying oneshot completion code from reqwest, so I just decided to take the level of abstraction one level higher. This migration was surprisingly easy due to the structure of rs-es. The benefits of reqwest apart from maintainability are that ability to build custom clients and pass these to rs-es, similar to that of the elastic rust library.

I have left this as WIP as I have not updated the documentation, or thoroughly tested.

@benashford
Copy link
Owner

Hello,

Yes, this is a good idea actually. It would still be a good idea for this library to go fully asynchronous in the fullness of time, as that seems to be where the wider Rust universe is going. But we can do that with Reqwest rather than lower-level Hyper code.

I'll have a fuller look into this over the next few days when I have some spare time.

Thanks!

@alexkornitzer
Copy link
Author

alexkornitzer commented Dec 11, 2018

Awesome, it should be easy to add an AsyncBuilder too, but for my cases atm I only need Sync so I didn't add that. I might have to move to elastic 6 soon, so I may start updating the library to handle that soon as well.

NOTE: The broken tests above are because I updated them to run on my ES5, so I will need to revert that as the library is not totally ES5 compatible.

@benashford
Copy link
Owner

Yes, we should leave making it async until a future change. Introducing Reqwest is a big enough change on its own and will solve a number of issues (e.g. SSL library compatibility issues).

@benashford benashford mentioned this pull request Jan 1, 2019
@benashford
Copy link
Owner

I'm going to close PR as the goal should have been achieved by #126. While this wasn't a direct merge of this PR, I used this as a reference and it was very helpful 👍

@benashford benashford closed this Jan 3, 2019
@alexkornitzer
Copy link
Author

Fair enough, thanks for finding time to update it :D Just one comment on #126, I would have kept the builder in as adding this at a later date will be another breaking change, and this gives the flexibility for users to easily modify their http client by passing a custom request client.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants