Closed strmer15 closed 1 year ago
@fastdivision @sethobey Not sure who can review this, can you point me in the right direction? Thanks!
There is a new advisory that affects the product now as well:
GHSA-p8p7-x288-28g6
I created an issue (#76), just in case... maybe somebody sees it :)
Also sent e-mail to taxjar support, hope that this will increase chances of this PR being merged
Hi @strmer15 - thank you so much for submitting this PR, and for the thorough description. We have reviewed it and are planning on merging/releasing it early next week! We really appreciate everyone's patience here.
Thanks for the update @smolentzov-stripe !
The current implemenation of
taxjar-node
usesrequest
under the hood, but the library has been deprecated for two years now. This change switches out usage ofrequest
andrequest-promise-native
for thenode-fetch
library.The main differences in the implementation are:
fetch
. This means that the baseapiUrl
, the path to the API endpoint, and the query params are all constructed using theURL
andURLSearchParams
constructors. Those APIs are only available in Node 10+, so this updates the minimum supported version of Node to 10.response.ok
, which is a convience property to check for a status code >= 200 and < 300.. When that is not set, I'm attempting to get the response body and parse into JSON for theproxyError
function to deal with. This should be functionally equivalent to what happens currently withrequest-promise-native
(see the Promise core plumbing at https://github.com/request/promise-core/blob/master/lib/plumbing.js#L68 and the error declarations in https://github.com/request/promise-core/blob/master/lib/errors.js).Additionally, I updated some types to use
Record<string, string>
which is what theURLSearchParams
expects for query params, and I updated all the dev dependencies to the latest. The newest version ofnock
specifically checks for invalid URLs (e.g. without a protocol) and throws an error, so I put anhttp://
in front of the testinvalidApiUrl
. Lastly, theinclude
check on one of the tests failed because the error message was longer but included the error string, so I changed to to callmatch
on theerr.message
using aRegExp
instead.