feat: use `teeny-request` instead of deprecated `request` dependency

googleapis / gax-nodejs

Google API Extensions for Node.js

Apache License 2.0

117 stars 88 forks source link

feat: use `teeny-request` instead of deprecated `request` dependency #1590

Closed wvanderdeijl closed 4 months ago

wvanderdeijl commented 5 months ago

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

[x] Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
[x] Ensure the tests and linter pass
[x] Code coverage does not decrease (if any source code was changed)
[x] Appropriate docs were updated (if necessary)

Fixes #1584

This is my suggested fix to #1584. Having a dependency on request is not wise as it is deprecated and has security issues. But it also poses an issue with bundlers (such as webpack) that will try to include request that is not installed in node_modules. Hence this solution with an explicit dependency to teeny-request

nicole0707 commented 4 months ago

Is anyone available to review the PR? @leahecole 🙏. It's blocking us for a while, Thanks!

leahecole commented 4 months ago

thanks for the ping! I want @sofisl to take a look at this first but Sofia, ping me if you want to chat about it

leahecole commented 4 months ago

Just had a brain blast, wasn't @danielbankhead doing work to migrate us from teeny-request to gaxios? Wondering if we may want to use that instead here.

danielbankhead commented 4 months ago

@leahecole yep! We will want to migrate away from both retry-request and teeny-request to gaxios.

sofisl commented 4 months ago

Since gaxios is used in auth, it won't make the package any bigger :) I think this is in the next phase of @danielbankhead's project so we should be working on it soon. As an FYI, I am planning on deprecating teeny and retry within the next few months, will put up a deprecation notice on the README to alert users long before we deprecate so that they can start preparing.

leahecole commented 4 months ago

Putting a do not merge on this given all of the shifting that's going on with teeny-request and gaxios right now, and because @danieljbruce may end up consolidating streamingRetryRequest into streaming.ts, rendering it obsolete. I'll make a note to look into this after that happens though - we do need to remove the request dependency!

leahecole commented 4 months ago

Closing because I'm going with a different strategy - removing what's not needed! 😄 see #1612