-
Notifications
You must be signed in to change notification settings - Fork 45
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
feat: log CONNECT error response body #139
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request is neither linked to an issue or epic nor labeled as adhoc!
src/resolve-protocol.ts
Outdated
Below is the first 100 bytes of the proxy response body: | ||
${head.toString('utf8', 0, 100)} | ||
`)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
Connected to https://github.com/apify/apify-proxy/issues/823.
On an internal proxy error (such as trying to access an invalid or unresolvable URL - e.g.
https://example.comundefined
), the proxy returns a response with the error message.got-scraping
only logs the length of the message, which is quite useless for most applications.