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

Replacing the request module #48

Open
RENOMIZER opened this issue Feb 3, 2024 · 3 comments
Open

Replacing the request module #48

RENOMIZER opened this issue Feb 3, 2024 · 3 comments

Comments

@RENOMIZER
Copy link

Hello.

The request module has been deprecated so I have made a fork that replaces it with axios.
All tests are passing and during the practical use no issues were spotted.
So I wanted to know whether or not you'd allow such a change to your original module so that I would know should I open a pull request or not?

@RENOMIZER RENOMIZER changed the title Replacing the request Replacing the request module Feb 3, 2024
@vot
Copy link
Member

vot commented Feb 4, 2024

Hey @RENOMIZER,

That's definitely a change worth making - there's no real reason for keeping request around. I've been a bit on the fence about the backwards breaking nature of fully promise-based axios for a while... This is a very no-thrills module where "it just works" is the main idea but the promises API has been well supported in Node for many years at this point so it might be the time to replace request.

Go ahead and raise a PR, I just took a look at your fork and it seems pretty much OK.
One thing I did notice is that we're losing the runningTotal += data.length bit which is used for periodical update of the download progress. I think we can get that same functionality by using onDownloadProgress callback from Axios. You can make that change either before or after opening the PR.

This change will need to be released under 1.2.x and there's a couple of small issues I'd like to still resolve in 1.1.x.
Just to give you a heads up: it'll take about a month or so before your PR gets merged and published but it is very much welcome - go ahead with the PR when you have a moment.

@RENOMIZER
Copy link
Author

RENOMIZER commented Feb 4, 2024

Ok, got you.

@shpingalet007
Copy link
Contributor

shpingalet007 commented Feb 9, 2024

Hey @vot,

Just in case if you consider to use Fetch API, there is node-fetch-progress NPM for such functionality. However Fetch returns ReadableStream which fulfills onDownloadProgress natively.

Example on how the mentioned above module solves that...

https://github.com/Prioe/node-fetch-progress/blob/master/src/index.js#L20-L28

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

No branches or pull requests

3 participants