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

Replace reqwest with ureq for less dependencies and smaller file size #547

Merged
merged 2 commits into from
Oct 13, 2021

Conversation

dnaka91
Copy link
Contributor

@dnaka91 dnaka91 commented Oct 12, 2021

First of all great project, although not mainly a JS/TS developer I use it quite often and really like the ease of use of it and compability with different shells 🎉.

I saw that fnm is using reqwest with the blocking feature enabled, which has a big impact on the dependency count and file size. The reason is that reqwest depends on the tokio runtime and enabling the blocking feature internally runs a tokio runtime with a non-async interface over the usual API. This leads to many dependencies and by using a different popular HTTP client library (namely ureq but there are several others) , both dep count and file size can be well improved.

ureq is a non-async HTTP client library and has a very small dependency count. Below are the results with Before when using reqwest and After when using ureq:

Value Before After Difference
Dependencies 208 151 -27.4%
File size (MacOS) 7.7MB 4.9MB -33.8%

I added a dependency on url as well, as I saw that it's used in many places (re-exported from reqwest) and wanted to keep it that way as I thought you might not want to replace all spots with a simple &str or String as the Url type gives you the guarantee of having a valid URL wherever you use it.

@vercel
Copy link

vercel bot commented Oct 12, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/schniz/fnm/5uKbSqRGDMzE7V5D7bgLNj89sBHv
✅ Preview: https://fnm-git-fork-dnaka91-ureq-schniz.vercel.app

@Schniz
Copy link
Owner

Schniz commented Oct 12, 2021

Wow, pretty straight forward but with significant file size change!
Thank you!

My concern is regarding compression which I am not sure that is supported in ureq: we currently enable brotli support for downloading the Node.js artifacts. It's not a small JSON file, but a prebuilt executable with Node libraries. I wonder if that affects timings. Needs to be benchmarked

@Schniz
Copy link
Owner

Schniz commented Oct 12, 2021

I am sure there are ways to overcome that. We can add a "accept-encoding" header and extract manually

@dnaka91
Copy link
Contributor Author

dnaka91 commented Oct 12, 2021

Oh that's a good point that I missed. I saw handling of gzip and tar.xz files so I assumed there is already some handling done but didn't check in detail.

I'll look for the parts where artifacts are downloaded and add gzip, deflate and brotli handling 🙇

@Schniz
Copy link
Owner

Schniz commented Oct 12, 2021

I'll look for the parts where artifacts are downloaded and add gzip, deflate and brotli handling

awesome! I think we can focus on brotli for now (send accept-encoding: br header and wrap the response with brotli decompression), because this is the only reqwest feature we are using right now :) we can always support more, but it can be postponed

@Schniz Schniz added the PR: New Feature A new feature was added label Oct 12, 2021
@dnaka91
Copy link
Contributor Author

dnaka91 commented Oct 12, 2021

Okay just added it. There are actually just 2 specific locations where we make HTTP calls.

  • For the node distribution
  • For the index JSON which contains information about all available distributions.

First I thought it's not necessary but I saw the JSON is about 250kb uncompressed and probably growing in the future. So for the index file only I added the brotli decompression. Not needed for the distributions themselves as they're already packed in an archive, I would say.

@Schniz
Copy link
Owner

Schniz commented Oct 13, 2021

Not needed for the distributions themselves as they're already packed in an archive, I would say

Yup, looking at the server responses it seems that the files are not brotli'd in return:

$ curl --head https://nodejs.org/dist/index.json -H 'Accept-Encoding: br'
HTTP/2 200 
...
content-encoding: br
$ curl --head https://nodejs.org/dist/latest-v16.x/node-v16.11.1-win-x64.zip -H 'Accept-Encoding: br'
HTTP/2 200
...
[no content-encoding header]

@dnaka91
Copy link
Contributor Author

dnaka91 commented Oct 13, 2021

Even if it would support brotli for archives as well, it wouldn't make much sense as an already compressed file doesn't benefit from being compressed again. It would bring additional computational overhead with nearly 0 benefit in file size.

Anyways, if you'd like any additional adjustments just let me know 👍

@dnaka91
Copy link
Contributor Author

dnaka91 commented Oct 13, 2021

Hm, Windows related actions failed. It doesn't allow me to see the details, could you have a look @Schniz ?

@Schniz
Copy link
Owner

Schniz commented Oct 13, 2021

@dnaka91 I'm rerunning the entire workflow. GitHub Actions was down for Windows runners: https://twitter.com/githubstatus/status/1448242741067005952

@Schniz Schniz changed the title Replace reqwest with ureq Replace reqwest with ureq for less dependencies and smaller file size Oct 13, 2021
@Schniz Schniz merged commit 9c2e360 into Schniz:master Oct 13, 2021
@Schniz
Copy link
Owner

Schniz commented Oct 13, 2021

Thanks! 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: New Feature A new feature was added
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants