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

chore: replace request and node-fetch with undici #331

Closed
wants to merge 1 commit into from

Conversation

p-kuen
Copy link

@p-kuen p-kuen commented Feb 9, 2022

This temporarily fixes #294 by replacing request, request-promise and node-fetch with undici.
Undici is backed by node itself and has a fetch method in experimental state, which should do the things needed for this package.

This could be a solution in the short time for the deprecation problems in this package.

Tests were successful for me.

Update: I just recognized undici has a node support for node >= 12.18, this package has a node support for node >= 6. Therefore this could be a problem and would require a major version upgrade.

@nchaulet
Copy link
Owner

nchaulet commented Feb 9, 2022

@p-kuen instead of relying on another library I think it will make more sense to wait on fetch to be in a node LTS version and not experimental.

@p-kuen
Copy link
Author

p-kuen commented Feb 9, 2022

Okay, I understand this strategy and to not want to use experimental features (although experimental in this case just means that some breaking changes are maybe coming in the next updates). I also want to point out that the fetch included in nodejs in the next versions is just the fetch from undici merged into nodejs core - makes almost no difference in the end.

Nevertheless we could at least replace request and request-promise with undici. The request function is not experimental at all. We would replace a long deprecated package.

@amiedes
Copy link
Contributor

amiedes commented Feb 10, 2022

The dependency on request-response library is breaking part of our instrumentation, so since we were on a hurry I decided to give a try to replacing request-response by node-fetch to avoid introducing new dependencies: CartoDB@01862dd

Then I saw that the RequestAdapter was deprecated 2 years ago, so maybe it makes sense to just remove the whole requestadapter.js altogether?

UPDATE: As far as I'm understanding there are adapters to several HTTP libraries (one of those already being node-fetch), so keeping in the codebase an adapter for the request-response library which is not using request-response under the hood kind of seems to defeat the purpose.

@nchaulet
Copy link
Owner

nchaulet commented Feb 13, 2022

Then I saw that the RequestAdapter was deprecated 2 years ago, so maybe it makes sense to just remove the whole requestadapter.js altogether?

Yes I think it's time to remove it, going to come with a PR for that and publish a new version as soon as possible (here is the PR #332)

@nchaulet
Copy link
Owner

@p-kuen Thanks for submitting that PR I removed the dependency on request and request-promise and publish a new major version of the library.

I am not oppose to move forward and replace node-fetch by undici in the future, but I would love the support for node-fetch to be not experimental, let reopen a PR when this will be more stable and supported in all active nodejs LTS .

@nchaulet nchaulet closed this Feb 15, 2022
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.

warning node-geocoder > request@2.88.2: request has been deprecated
3 participants