Update HTTP timeout error messaging #1419
Conversation
camden11
requested review from
brandenrodgers,
kemmerle and
joe-yeager
as code owners
| const config = getConfig(); | ||
| const defaultTimeout = config?.httpTimeout; | ||
|
|
||
| if (error.timeout && defaultTimeout === error.timeout) { |
There was a problem hiding this comment.
Checking this to make sure the message doesn't get displayed in cases where we explicitly set a timeout
There was a problem hiding this comment.
Should we still have some sort of error override, even if we manually adjusted the timeout?
There was a problem hiding this comment.
Good question, I was thinking we'd just handle that on a case by case basis anywhere where we suspect it could be an issue (which is few or zero places as far as i know)
There was a problem hiding this comment.
Ah I see what you mean. My guess is that it would be fine to open this up to apply to all timeouts, but what you have is an improvement regardless. You can make the call to keep or remove the extra check
There was a problem hiding this comment.
Ohh I was just trying to avoid the case where we manually set a timeout somewhere and then a user tries to fix an error with hs config set but it doesn't work. I'll add a generic message for when that's the case
There was a problem hiding this comment.
I didn't think about that. Then yeah +1 to keeping this in 👌
There was a problem hiding this comment.
Might be worth adding a comment so we don't forget that years from now lol
Description and Context
HTTP timeout errors on the CLI are usually easy to solve by increasing timeout with
hs config set, but we don't give users any guidance towards doing that. This adds a helpful message for all timeout errors that are triggered by the default timeout.Relies on this LDL PR: https://github.com/HubSpot/hubspot-local-dev-lib
Issue: https://git.hubteam.com/HubSpot/hubspot-cli-issues/issues/803
Screenshots
Before (unhelpful)
After (helpful)
Who to Notify
@brandenrodgers @joe-yeager @kemmerle