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

Update status code error types #72

Merged
merged 9 commits into from
Dec 14, 2023
Merged

Conversation

camden11
Copy link
Contributor

@camden11 camden11 commented Dec 12, 2023

Description and Context

A few months ago, we switched from using request and request-promise-native to axios, but never updated the types for http related errors. This PR removed the StatusCodeError type associated with request and updates instances of it to instead use AxiosError

TODO

Should make sure to test this thoroughly. To my knowledge, the nothing currently being used by the CLI threw StatusCodeErrors so this should be relatively safe. I've tested with my upcoming PR that did rely on StatusCodeError HubSpot/hubspot-cli#964

Who to Notify

@brandenrodgers

@camden11 camden11 marked this pull request as ready for review December 13, 2023 19:36
@@ -13,19 +9,7 @@ function isSystemError(err: BaseError) {
}

function debugErrorAndContext(error: BaseError, context?: ErrorContext): void {
if (error.name === 'StatusCodeError') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does removing this result in more content being logged to the console? Is this filtering out some of the extra content that gets included on the error object for status code errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

errors_DEPRECATED is only used with the old config_DEPRECATED to make sure the logging was identical. Config doesn't make any requests so there won't be any StatusCodeErrors thrown

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah thanks for explaining. I didn't notice the file that this was in 👍

@camden11 camden11 merged commit fcdd90e into main Dec 14, 2023
1 check passed
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

Successfully merging this pull request may close these issues.

2 participants