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 deprecated "request" library #29

Closed
NicolasMassart opened this issue Oct 10, 2020 · 14 comments · Fixed by #43
Closed

Replace deprecated "request" library #29

NicolasMassart opened this issue Oct 10, 2020 · 14 comments · Fixed by #43
Assignees
Labels
dependencies Pull requests that update a dependency file help wanted

Comments

@NicolasMassart
Copy link
Contributor

NicolasMassart commented Oct 10, 2020

Request is deprecated and will never be updated or fixed.
Migrating to a new supported library is required for long term.

Does anyone have thoughts on which one we should choose?
Would the core node.js http be enough?

Note that we only need to be able to do HEAD and GET requests. We need to be able to pass auth and any additional headers, get HTTP codes, body and response headers.

@NicolasMassart NicolasMassart self-assigned this Oct 10, 2020
@NicolasMassart NicolasMassart added dependencies Pull requests that update a dependency file help wanted hacktoberfest labels Oct 10, 2020
@ccsCoder
Copy link

Hi. This looks interesting.
I went through the attached issue. Have we settled on the library of choice ?
If yes, I am interested in taking this up.

@NicolasMassart
Copy link
Contributor Author

NicolasMassart commented Oct 13, 2020

For what I investigated, maybe node's default http api could be enough. We only do head or get requests with a few headers, and we don't process the response body.
In the YAGNI spirit I would be tempted to start with this, simple.
If we need to implement more complex things later, we'll see but there's no need to load the project with a complex dependency if we only use 1% of it.
Let me know if that's something you agree with to start your work, if not, feel free to propose something and explain your choice.

@nschonni
Copy link
Contributor

The case for using a library is that it will also handle proxies, which are likely a common need for a library like this that is used a lot in CI setups

@ccsCoder ccsCoder removed their assignment Oct 14, 2020
@ccsCoder
Copy link

Sorry guys. Official workload increased. unassigning myself

@NicolasMassart
Copy link
Contributor Author

@nschonni do we currently handle proxies?
@ccsCoder 😢 take care, come back whenever you can.

@nschonni
Copy link
Contributor

request handles it under the hood already https://github.com/request/request#controlling-proxy-behaviour-using-environment-variables

@NicolasMassart
Copy link
Contributor Author

NicolasMassart commented Oct 14, 2020

@nschonni What lib would you suggest?

@tcort
Copy link
Owner

tcort commented Oct 15, 2020 via email

@NicolasMassart
Copy link
Contributor Author

NicolasMassart commented Oct 15, 2020

Migration from request to Needle also seems very easy.
I think we could go with Needle and anyway if we have issues we can still change.

@ThunderSon
Copy link

I am not the best person in node dev, but if no one is working on this, I can try building something up.
I never used needle (used axios and request), but I don't mind trying this out.

Let me know if I can be of help.

@NicolasMassart
Copy link
Contributor Author

You are very welcome to help! I assign issue this to you and move it to "in progress".

@ThunderSon
Copy link

Should I be using promises, or traditional callbacks?
If you want we can have this staged.
First we go with callbacks to ensure smooth migration, then we implement promises if it's something that benefits the project.

@NicolasMassart
Copy link
Contributor Author

I agree with callback first for compatibility and then promises. That seems the wise choice.

@NicolasMassart NicolasMassart self-assigned this Jan 13, 2021
@NicolasMassart
Copy link
Contributor Author

hi @ThunderSon did you already start working on this?
If not, I have something that works with Needle and that helps solving another issue.
I will tell you when I have a PR ready.

NicolasMassart added a commit to NicolasMassart/link-check that referenced this issue Jan 13, 2021
update debug dep as the lower version shipped with Mocha is vulnerable

replace deprecated use of legacy Node URL by new WHATWG URL API
See https://nodejs.org/api/url.html#url_legacy_url_api

update tests as timeout and redirect return changes with Needle

fixes tcort#29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file help wanted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants