search

braintree / braintree_node

Braintree Node.js library
https://developer.paypal.com/braintree/docs/start/overview
MIT License
334 stars 104 forks source link

add keepalive agent #164

Closed blugavere closed 2 years ago

blugavere commented 4 years ago

Summary

add keepalive agent

Checklist

blugavere commented 3 years ago

bump

hollabaq86 commented 3 years ago

👋 @blugavere, thanks for your patience while we review this PR. After discussing with our infrastructure teams to confirm we can support keepalive as a default setting, we have concerns about how this could expose our API.

As an alternative, the SDK could be modified to accept a custom http agent. If you'd like to PR that work we'd be happy to review! Otherwise, we don't have an ETA on when this will get added to the SDK, but we'll make an issue in this repo so it stays on our radar.

James1x0 commented 2 years ago

Hi there, this pull has been updated to the current lib version. Ready for re-review.

James1x0 commented 2 years ago

Conflict resolved. Bump.

cgdibble commented 2 years ago

@James1x0 there are still outstanding requested changes from @hollabaq86 here.

👋 @blugavere, thanks for your patience while we review this PR. After discussing with our infrastructure teams to confirm we can support keepalive as a default setting, we have concerns about how this could expose our API.

As an alternative, the SDK could be modified to accept a custom http agent. If you'd like to PR that work we'd be happy to review! Otherwise, we don't have an ETA on when this will get added to the SDK, but we'll make an issue in this repo so it stays on our radar.

To re-iterate, the SDK should be modified to accept an agent that is already configured and no longer include these Agents within the SDK. That agent should then be used, if present, on the Http client. Does that make sense?

jaxdesmarais commented 2 years ago

Hello @James1x0 and @blugavere -

We've released version 3.9.0 of the SDK that allows passing a fully custom HTTP Agent in the configuration object. This allows you to take full control over any defaults if you want or allows the SDK to use the global Node agent with Node defined defaults.

We preferred this approach as it leaves the developer/merchant to pass any default they desire and removes the SDK from having to define this on behalf of a developer/merchant. Thanks again for this suggestion!