Custom error from createConnection makes it hard to get the response status/code

[ckcr4lyf]

Currently, if a CONNECT to the proxy fails, the error returned is using a vanilla Error with string templating to add the status code

hpagent/index.js

Line 44 in cc90c2b

callback(new Error(`Bad response: ${response.statusCode}`), null)

This means that from within the axios logic to send a request, if the proxy connection fails, it is not as trivial to extract the response status. Example with wrong credentials, using axios + hpagent:

For axios errors, the statusCode is typically in response.status, but since this is a custom error, that field is undefined. Since got, axios etc. have their own ways of handling errors, specifically what the field for the statusCode is called, maybe an hpagent extension to the error object, containing the code, and a boolean, isHpagentError: true ?

Otherwise currently I need to extract the response code from the string. I would be happy to make a PR for this if we can align on a way to pass this information

[delvedor]

Hello! I don't like the approach of adding a custom boolean to identify the error.
If we have to do this, it should follow Node.js's error conventions.

It could be something like:

class HPAError extends Error {
  constructor (message, statusCode) {
    super(message)
    this.name = 'HPAError'
    this.code = 'HPA_ERR_STATUS'
    this.statusCode = statusCode
  }
}

I fear this change would require a breaking change tho.

[ckcr4lyf]

That's also a good way! I just stole the boolean thing from axios. Would it still be a breaking change if we kept the error "message" same?

…

________________________________ From: Tomas Della Vedova ***@***.***> Sent: Thursday, May 12, 2022 5:42:05 PM To: delvedor/hpagent ***@***.***> Cc: Raghu Saxena ***@***.***>; Author ***@***.***> Subject: Re: [delvedor/hpagent] Custom error from createConnection makes it hard to get the response status/code (Issue #70) Hello! I don't like the approach of adding a custom boolean to identify the error. If we have to do this, it should follow Node.js's error conventions. It could be something like: class HPAError extends Error { constructor (message, statusCode) { super(message) this.name = 'HPAError' this.code = 'HPA_ERR_STATUS' this.statusCode = statusCode } } I fear this change would require a breaking change tho. — Reply to this email directly, view it on GitHub<#70 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABS7AJ3WASONLALNY2B5PRTVJTG63ANCNFSM5VWZCCPA>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[delvedor]

I'm not sure, the following doesn't throw:

const assert = require('assert')

class HPAError extends Error {
  constructor (message, statusCode) {
    super(message)
    this.name = 'HPAError'
    this.code = 'HPA_ERR_STATUS'
    this.statusCode = statusCode
  }
}

const err = new HPAError('Bad response: 407', 407)
assert(err instanceof Error)
assert(err instanceof HPAError)
assert(err.message === 'Bad response: 407')

The issue could happen here:

assert(err.name === 'Error') // now `.name` is 'HPAError'

[ckcr4lyf]

Ah right, this would mean changing the name property as well...

[mbargiel]

Have you considered replaying the proxy error response onto the socket, e.g. like it's done with https-proxy-agent? That allows the client to be completely unaware of proxies at all, so it goes ahead, sends its headers (ideally on a dummy, not-connected socket) and then reads the response status and headers back.

[mbargiel]

I was going to open an issue for this, in fact - we would like our client to handle an HTTP response 407, not have to deal with knowing about hpagent as we intend to use it only indirectly through an HTTP library. (For context, I'm considering integrating hpagent into axios instead of using https-proxy-agent as I initially planned.)

[mbargiel]

@delvedor I think I could open a PR for this. Would you take it?