Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot read properties of undefined (reading 'defaultPort') when used with nock #675

Open
TiddoLangerak opened this issue Mar 4, 2024 · 9 comments

Comments

@TiddoLangerak
Copy link

TiddoLangerak commented Mar 4, 2024

SDK you're using (please complete the following information):

  • Version 5.1.0

Describe the bug
When using Nock to mock requests to Xero then error responses result in a Cannot read properties of undefined (reading 'defaultPort') error triggered in the ApiError class.

This appears to be caused by xero-node using an undocumented http.ClientRequest.agent property to fetch the port, which doesn't exist when using Nock (and possibly other mocking frameworks, too).

To Reproduce

  1. Setup a test using nock to generate some kind of error response. For example:
     nock(/xero/)
         .get(/Accounts/)
         .reply(429);
     client.accountingApi.getAccounts(/*...*/);
    
  2. Observe that this triggers the above error, instead of throwing an ApiError

Expected behavior
I expect an ApiError to be thrown, instead of a cannot read properties of undefined

Screenshots

Additional context
http.ClientRequest.agent is an undocumented property, it's likely not intended as a public facing API. There's a documented alternative available to get the port: request.socket.localPort. So replacing

port: axiosError.request.agent.defaultPort,

with

port: axiosError.request.socket.localPort

should give the same information, using a documented public api, and compatible with Nock.

Copy link

github-actions bot commented Mar 4, 2024

PETOSS-396

Copy link

github-actions bot commented Mar 4, 2024

Thanks for raising an issue, a ticket has been created to track your request

@TiddoLangerak
Copy link
Author

Have you had the chance to consider this issue? I'd be happy to submit a PR if you agree with the suggested change?

@QuentinLemCode
Copy link

Same issue here

@QuentinLemCode
Copy link

@TiddoLangerak The PR can maybe accelerate the fix :)

@QuentinLemCode
Copy link

@sangeet-joy-tw Can we have update on this please ?

@c4c1us
Copy link

c4c1us commented Apr 8, 2024

Hey, @sangeet-joy-tw Could we have an update on this, please? Thanks in advance 🙏

@QuentinLemCode
Copy link

QuentinLemCode commented Apr 9, 2024

Hello @BenGDev @GraceYBR @JamesColemanXero @MattR-Api @MJMortimer @TheRegan
This issue blocks us to update the xero-node dependency. We rely on nock to test our business services using Xero API.

Can you review and approve this PR please ?
#682

@QuentinLemCode
Copy link

@manishT72 @manishT72x
Could you take a look at this issue and the PR linked, please ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants