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

Migrate from request to node-fetch #54

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tamaracha
Copy link

Hi, this package looks interesting for my purposes, but the request deprecation warning is nasty. Could it be a possible approach to take smaller steps and migrate to a lighter HTTP client? I have migrated from request to node-fetch in my fork. This gets rid of about 40 dependencies, the tests run 2 seconds faster, and node-fetch supports redirects.

@sporto
Copy link

sporto commented Dec 19, 2022

@avh4 any chance you merge this soon?

#65 is a big issue for a couple of important Elm related npm packages that use this package (elm-format and elm-json).

Otherwise, here is a list of other possible alternatives request/request#3143
I could make a PR if you prefer to use some of the others.

Thanks

Copy link
Owner

@avh4 avh4 left a comment

Choose a reason for hiding this comment

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

Thanks for the first draft!

Do you happen to know if this will follow redirects? And does it also honor proxy settings from the shell environment?

reject("Error communicating with URL " + url + " " + error);
return;
}
if (response.statusCode == 404) {
Copy link
Owner

Choose a reason for hiding this comment

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

According to https://www.npmjs.com/package/node-fetch#handling-exceptions

3xx-5xx responses are NOT exceptions, and should be handled in then()

Doesn't that mean we still need to check for 404 or any other 300-599 status codes?

.pipe(gunzip)
.pipe(untar);
return
Copy link
Owner

Choose a reason for hiding this comment

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

Don't we need to return the stream or the promise or whatever so that the caller will be able to wait for the operation to fully complete? (Or is that happening somehow as this is?)

.then(
function () {
console.log("OKAY: " + displayUrl);
return
Copy link
Owner

Choose a reason for hiding this comment

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

We still need to check for statusCode != 200 here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants