gax-nodejs icon indicating copy to clipboard operation
gax-nodejs copied to clipboard

Published 20 hours ago • verified publisher icon googleapis

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

Open wvanderdeijl opened this issue 10 months ago • 2 comments

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

wvanderdeijl avatar Apr 24 '24 11:04 wvanderdeijl

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

nicole0707 avatar May 15 '24 01:05 nicole0707

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 avatar May 15 '24 14:05 leahecole

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.

leahecole avatar May 20 '24 21:05 leahecole

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

danielbankhead avatar May 20 '24 21:05 danielbankhead

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.

sofisl avatar May 20 '24 21:05 sofisl

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 avatar May 21 '24 13:05 leahecole

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

leahecole avatar Jun 06 '24 14:06 leahecole