search

goshippo / shippo-node-client

Shipping API Node.js library (USPS, FedEx, UPS and more)
https://goshippo.com/docs
MIT License
136 stars 54 forks source link

Node.js v13+ default timeout changed #55

Closed jonchurch closed 4 years ago

jonchurch commented 4 years ago

Hey there, wanted to point out that under Node.js v13+ the default timeout has changed from 2 minutes to 0. This has the effect of never timing out.

In your current implementation, under Node.js v13, your requests will never timeout on the client side.

The change would need to be made here: https://github.com/goshippo/shippo-node-client/blob/b215c5af69211febfb26955db868b07a248d5b60/lib/shippo.js#L7

You can hardcode the previous value of 120000 if you want to preserve the behavior of previous Node.js versions (this value has been 120000 since it was introduced to Node.js, until the recent change).

I opened #56 with the proposed change.

I've been searching Github for codebases that rely on this timeout value and manually opening issues/PRs like this. My intent is to bring your attention to this subtle change and let you decide if it's necessary to address ❤️

epistemancer commented 4 years ago

@jonchurch: thanks for the heads-up! We're going to hardcode it to 30s instead of 2m -- that's a better ceiling for our API calls -- so I'll do that in a new PR instead of merging yours, but appreciate your contribution.