Skip to content

Conversation

gmg137
Copy link

@gmg137 gmg137 commented Apr 2, 2020

  • Compatible with async-std.
  • Fix test errors.

Note: reqwest is pure rust, isahc relies on curl.

- Fix test errors.
@Empty2k12
Copy link
Collaborator

Hello! Thanks for openig this PR! I think it makes a lot of sense to support all runtimes available.

DO you think, however, we could support async-std only with a feature-flag and use reqwest when users do not opt into async-std. I think we can select the http client dynamically via the feature flag. Tests in CI would nbeed to run twice, once with each http library.

@gmg137
Copy link
Author

gmg137 commented Apr 6, 2020 via email

@Empty2k12
Copy link
Collaborator

reqwest is pure rust, while isahc is dependent on the OS. I think that should be opt-in :) Interms of complexity, maybe a common HTTP Abstraction for the library could be provided which takes care of sending requests with isahc or reqwest depending on the feature-flag.

@gmg137
Copy link
Author

gmg137 commented Apr 6, 2020 via email

} else {
ReqwestClient::new().post(url)
isahc::post_async("", url.as_str().to_owned()).await
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same order of arguments here too according to my IDE

Copy link

@MTRNord MTRNord left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works on my PC :)

@Empty2k12
Copy link
Collaborator

@gmg137 How do you think should the global feature design be changed? I thought it might be possible to add a feature flag for toggling between async-std/tokio

@gmg137
Copy link
Author

gmg137 commented May 12, 2020

@Empty2k12 Agreed, adding a feature flag is by far the most common practice.

@Empty2k12
Copy link
Collaborator

This is now implemented in #72. Thanks for your interest in the project.

@Empty2k12 Empty2k12 closed this Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants