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 request with needle #350

Merged
merged 1 commit into from
Mar 14, 2018
Merged

Replace request with needle #350

merged 1 commit into from
Mar 14, 2018

Conversation

cktang88
Copy link
Contributor

Closes #340

@springmeyer
Copy link
Contributor

Looks good, thanks for putting this together. Only test that is failing is unrelated. For the sake of our future selves, can you share why you chose needle? After that I'll plan to merge.

@cktang88
Copy link
Contributor Author

cktang88 commented Mar 10, 2018

@springmeyer I choose needle for the following reasons (in no particular order):

  1. Had similar configuration and functionality to request, so that few changes to /lib/install.js would be needed (specifically, needed to support proxy and HTTP headers)
  2. Seems to be widely used, reliable, and actively developed (1.28 million downloads/month)
  3. Very lightweight, few dependencies

@ovaar
Copy link

ovaar commented Mar 12, 2018

What about got

@LinusU
Copy link

LinusU commented Mar 12, 2018

Name Downloads 1 Bundle size 2
simple-get 1,747,443 80kB
got 11,009,114 156kB
needle 1,231,722 556kB
request 39,390,626 ??? 3

1: from npmjs.com
2: from bundlephobia.com
3: it timed out, so probably quite large 😄

I personally prefer got because it strikes a good balance between size and popularity, is maintained by some of the most known people within the node ecosystem, and has an excellent api!

@cktang88
Copy link
Contributor Author

If you consider actual disk size, on my Win10 (NTFS file system), needle is 635kb, while got is 564kb, so a difference of only 71kb. Although I admit simple-get is a lot smaller. Also, for what it's worth, needle only installs 5 packages, while got installs 44.

@cktang88
Copy link
Contributor Author

cktang88 commented Mar 12, 2018

Hmmm.... if we are going to have a discussion about all the possible replacement options, I believe axios should also get a mention. It's only 14kb according to bundlephobia.com which beats all three options mentioned thus far by a long shot, and has 5.8mil downloads/month.

@LinusU
Copy link

LinusU commented Mar 12, 2018

The problem with adding axios and superagent to my list is that they have an alternative browser implementation, which makes there size "lie" for our use case... Listing sizes from bundlephobia.com is actually not a good idea at all, it was just the first tool I found 😄

The best would be to try and install them in a clean directory and then measuring disk size, hopefully I'll get some time for that this evening

@LinusU
Copy link

LinusU commented Mar 12, 2018

Okay, here is a list using actual install sizes, which is a much better measurement for our use case:

Name Downloads 1 Installed size 2
axios 5,802,061 588kB
got 11,009,114 1260kB
needle 1,231,722 888kB
request 39,390,626 5800kB
simple-get 1,747,443 140kB
superagent 6,061,245 1852kB

1: from npmjs.com
2: from running echo {} > package.json; npm i $NAME; du -sk . in an empty directory

@ovaar
Copy link

ovaar commented Mar 12, 2018

@LinusU are these production installs?

@LinusU
Copy link

LinusU commented Mar 12, 2018

@ninox92 yes 👍

@springmeyer
Copy link
Contributor

Thanks for the extra details and discussion here. I like the idea of going out with needle now since it ranks well for size but does not require code changes.

@springmeyer springmeyer merged commit 07e7ee5 into mapbox:master Mar 14, 2018
@springmeyer
Copy link
Contributor

v0.9.0 is now published with this PR included.

springmeyer pushed a commit to mapbox/mason-js that referenced this pull request Mar 28, 2018
millzpaugh pushed a commit to mapbox/mason-js that referenced this pull request Mar 29, 2018
* move deps to devDeps that are only needed when testing

* follow the lead on mapbox/node-pre-gyp#350

* fix needle usage
hyj1991 pushed a commit to X-Profiler/node-pre-gyp that referenced this pull request Jun 16, 2023
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.

4 participants