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.

Sign up for GitHub

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

feat: log CONNECT error response body #139

Merged
merged 3 commits into from
May 22, 2024
Merged

feat: log CONNECT error response body #139

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
30d4924
feat: log CONNECT error response body
barjin May 14, 2024
c9610ec
feat: throw ProxyError with `cause`
barjin May 16, 2024
7211b57
chore: bump `got`, fix build
barjin May 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/resolve-protocol.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,11 @@ const connect = async (proxyUrl: string, options: tls.ConnectionOptions, callbac

request.once('connect', (response, socket, head) => {
if (response.statusCode !== 200 || head.length > 0) {
reject(new Error(`Proxy responded with ${response.statusCode} ${response.statusMessage}: ${head.length} bytes`));
reject(new Error(`Proxy responded with ${response.statusCode} ${response.statusMessage}: ${head.length} bytes.

Below is the first 100 bytes of the proxy response body:
${head.toString('utf8', 0, 100)}
`));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems weird to include this kind of formatting in the message of an Error, but we probably don't have any other options, do we?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could squish it in cause, but the intuition (and MDN doc) tells me that the message is for human readable errors and cause is for structured data / original errors in case of rethrowing.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, sounds reasonable. Still, the newlines freak me out 😄

Could we also include the whole response body somewhere in cause?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multiline error messages are quite common, we use those in several places already, also playwright and puppeteer do the same (and much worse, e.g. 10+ lines error messages, namely those about a missing browser executable). you can attach metadata to the error instance too, but its better to have a custom error class for that so its discoverable. we also use this approach (but we don't add any metadata there, just custom error prototypes).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to bump got to have sindresorhus/got#2327 , which caused the build to break... fixed by the tsconfig.json update. Happy to get feedback on that, it fixes the build, but might not be the best way around it.

socket.destroy();
return;
}
Expand Down
Loading