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

Use reqwest by default #563

Merged
merged 15 commits into from
Apr 15, 2024
Merged

Use reqwest by default #563

merged 15 commits into from
Apr 15, 2024

Conversation

irevoire
Copy link
Member

@irevoire irevoire commented Apr 11, 2024

Pull Request

Related issue

Fixes #530 because we won’t depend on curl anymore

What does this PR do?

  • Get rid of isahc by default in favor of reqwest, which is way more used in the ecosystem

PR checklist

Please check if your PR fulfills the following requirements:

  • Create an example changing the default HTTP client
  • Simplify the HttpClient trait to only require one method to be implemented
  • Stores a basic reqwest::Client in the meilisearch_sdk::Client instead of re-creating it from scratch for every request
  • Double check it works with wasm
  • Remove all the unwraps in request
  • Do not use the User-Agent when in wasm as it is often blocked by the browser

@irevoire irevoire added the breaking-change The related changes are breaking for the users label Apr 11, 2024
@gibbz00
Copy link

gibbz00 commented Apr 13, 2024

It is as if #524 doesn't exist

@irevoire
Copy link
Member Author

irevoire commented Apr 13, 2024

I did look at it, but since I didn’t have much time to work on this, I preferred to rush something that works with the community. Sorry.
Also, I would like to get the WASM to work with reqwest; it's supposed to be supported 🤔

Once this is merged I would love to work with you on improving the SDK even more, thanks for your work

@irevoire irevoire force-pushed the use-reqwest-by-default branch 2 times, most recently from 3d82ce6 to 060d990 Compare April 14, 2024 23:35
@irevoire irevoire marked this pull request as ready for review April 15, 2024 13:22
Copy link
Member Author

@irevoire irevoire left a comment

Choose a reason for hiding this comment

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

bors merge

meili-bors bot added a commit that referenced this pull request Apr 15, 2024
563: Use reqwest by default r=irevoire a=irevoire

# Pull Request

## Related issue
Fixes #530 because we won’t depend on curl anymore

## What does this PR do?
- Get rid of `isahc` by default in favor of `reqwest`, which is way more used in the ecosystem

## PR checklist
Please check if your PR fulfills the following requirements:
- [x] Create an example changing the default HTTP client
- [x] Simplify the `HttpClient` trait to only require one method to be implemented
- [x] Stores a basic `reqwest::Client` in the `meilisearch_sdk::Client` instead of re-creating it from scratch for every request
- [x] Double check it works with wasm
- [x] Remove all the unwraps in `request`
- [x] Do not use the `User-Agent` when in wasm as it is often blocked by the browser


Co-authored-by: Tamo <[email protected]>
Copy link
Contributor

meili-bors bot commented Apr 15, 2024

This PR was included in a batch that successfully built, but then failed to merge into main. It will not be retried.

Additional information:

{"message":"At least 1 approving review is required by reviewers with write access.","documentation_url":"https://docs.github.com/articles/about-protected-branches"}

@curquiza
Copy link
Member

bors merge

Copy link
Contributor

meili-bors bot commented Apr 15, 2024

@irevoire irevoire merged commit 437649f into main Apr 15, 2024
7 checks passed
@irevoire irevoire deleted the use-reqwest-by-default branch April 15, 2024 13:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change The related changes are breaking for the users
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Library seems unusable on MacOS 14.0
3 participants