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.

Sign up for GitHub

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

Jump to bottom

refactor: replace reqwest with ureq #844

Merged
merged 2 commits into from Jan 19, 2020
Merged

refactor: replace reqwest with ureq #844

merged 2 commits into from Jan 19, 2020

Conversation

davidkna
Copy link
Member

@davidkna davidkna commented Jan 17, 2020 •
edited

Description

Motivation and Context

reqwest is quite a heavy dependency in terms of file size and pulls in a lot of async related dependencies (tokio) despite starship only using the blocking API.
By replacing reqwest with ureq binary size on windows release builds is reduced by ~30%.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Screenshots (if appropriate):

How Has This Been Tested?

  • I have tested using MacOS
  • I have tested using Linux
  • I have tested using Windows

Checklist:

  • I have updated the documentation accordingly.
  • I have updated the tests accordingly.

jonstodle
jonstodle reviewed Jan 18, 2020
View reviewed changes
Copy link
Contributor

@jonstodle jonstodle left a comment

Choose a reason for hiding this comment

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

This looks good to me.

@davidkna
Merge remote-tracking branch 'upstream/master' into ureq
47395d2 
* upstream/master:
  build(deps): bump toml from 0.5.5 to 0.5.6
  fix: Fix incorrect prefix usage in cmd_duration module (#832)
  ci: Update CI dependencies (#850)
  docs: Fix indentation and missing modules in README (#847)
@matchai matchai merged commit d869b3a into starship:master Jan 19, 2020
@matchai
Copy link
Member

matchai commented Jan 19, 2020

Thank you for your PR, David! 😊

@q66
Copy link

q66 commented Feb 1, 2020

This breaks starship on any architecture other than x86 and ARM by pulling in ring (via rustls, which is needed for the tls feature) which is assembly-only without portable fallbacks, and doesn't seem to have any plans to change this, or accept additional platform support. It'd be great if you could reconsider this in favor of some other, better solution.

matchai added a commit that referenced this pull request Feb 1, 2020
@matchai
revert: "refactor: replace reqwest with ureq (#844)"
8ce1a6c 
This reverts commit d869b3a.
@matchai matchai mentioned this pull request Feb 1, 2020
Merged
8 tasks
matchai added a commit that referenced this pull request Feb 1, 2020
@matchai
revert: "refactor: replace reqwest with ureq (#844)"
fdf86a6 
This reverts commit d869b3a.
matchai added a commit that referenced this pull request Feb 4, 2020
@matchai
revert: "refactor: replace reqwest with ureq (#844)"
4e27119 
This reverts commit d869b3a.
matchai added a commit that referenced this pull request Feb 6, 2020
@matchai
revert: "refactor: replace reqwest with ureq (#844)" (#895)
0908c12
matchai added a commit that referenced this pull request Feb 6, 2020
@matchai
revert: "refactor: replace reqwest with ureq (#844)" (#895)
8a7f925 
Former-commit-id: 0908c12
matchai added a commit that referenced this pull request Feb 6, 2020
@matchai
revert: "refactor: replace reqwest with ureq (#844)" (#895)
21f2947 
Former-commit-id: 0908c12
@q66 q66 mentioned this pull request Feb 12, 2020
Closed
This was referenced Mar 5, 2020
Closed
Merged
dagbrown pushed a commit to dagbrown/starship that referenced this pull request Oct 22, 2021
@matchai @dagbrown
revert: "refactor: replace reqwest with ureq (starship#844)" (starshi…
5997a95 
…p#895)

Former-commit-id: 0908c12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonstodle jonstodle jonstodle approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@davidkna @matchai @q66 @jonstodle