XeroAPI / xero-node

Xero Node SDK for OAuth 2.0 generated from XeroAPI/Xero-OpenAPI
http://developer.xero.com/
MIT License
192 stars 155 forks source link

Add optional chaining to ensure compatibility with mocking library #682

Closed QuentinLemCode closed 1 week ago

QuentinLemCode commented 2 months ago

Solve #675

TiddoLangerak commented 2 months ago

I'm not quite sure if this is desirable, as this changes the contract (thus is a breaking change). I wouldn't expect the values to be undefined in tests (or anywhere else for that matter). I think the minimal change needed to make this work with nock is to replace port: axiosError.request.agent.defaultPort, with port: axiosError.request.socket.localPort - without optional chaining. This preserves the contract, and thus is a non-breaking change that addresses the issue.

QuentinLemCode commented 2 months ago

I'm not quite sure if this is desirable, as this changes the contract (thus is a breaking change). I wouldn't expect the values to be undefined in tests (or anywhere else for that matter). I think the minimal change needed to make this work with nock is to replace port: axiosError.request.agent.defaultPort, with port: axiosError.request.socket.localPort - without optional chaining. This preserves the contract, and thus is a non-breaking change that addresses the issue.

You are right, my change made the type invalid (in fact, it was already), but I'd rather update the contract. I'm not sure the object socket is always present. Do you have documentation about that?

TiddoLangerak commented 2 months ago

https://nodejs.org/api/http.html#requestsocket

QuentinLemCode commented 2 months ago

@TiddoLangerak Thanks, but I'm still unsure that this object is always present There can be cases where the request has not been done yet : request cancellation, request interceptor failed, payload transform error ...

The request object is marked as optional in the Axios type definition. We should reflect this in the ApiError type definition.

Still, I have updated the code to use axiosError.request.socket.localPort first

sangeet-joy-tw commented 1 week ago

The issue has been addressed in our latest xero-node v7.0.0. Hence closing this PR.