search

nchaulet / node-geocoder

nodejs geocoding library
http://nchaulet.github.io/node-geocoder/
MIT License
926 stars 213 forks source link

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

Closed p-kuen closed 2 years ago

p-kuen commented 2 years ago

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 commented 2 years ago

@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 commented 2 years ago

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 commented 2 years ago

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: https://github.com/CartoDB/node-geocoder/commit/01862dd956e84ef02d3c2eb3c43bc55ee783ea10

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 commented 2 years ago

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 https://github.com/nchaulet/node-geocoder/pull/332)

nchaulet commented 2 years ago

@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 .