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

chore: created new type for API errors, created and implemented type predicates #6877

Merged
merged 10 commits into from
Oct 18, 2024

Conversation

dylanspyer
Copy link
Contributor

🎉 Thanks for submitting a pull request! 🎉

Summary

Fixes conversion to TypeScript

Previously, most invocations of the custom error function needed to have a TypeScript ignore comment due to the error_ object being passed to the function being of type unknown. Because the error function already takes error_ arguments that are of various different shapes, adding unknown to the parameter's type union allowed us to remove these TypeScript ignore comments without sacrificing type safety. Added a new type APIError and type predicates for custom type guarding of errors that are a result of an internal API call.


For us to review and ship your PR efficiently, please perform the following steps:

  • Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
  • Read the contribution guidelines 📖. This ensures your code follows our style guide and
    passes our tests.
  • Update or add tests (if any source code was changed or added) 🧪
  • Update or add documentation (if features were changed or added) 📝
  • Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

dylanspyer and others added 2 commits October 14, 2024 17:06
…predicates

Created APIError interface in `command-helpers.ts`.
Created two type predicates `isAPIError` and `errorHasStatus`.
Implemented predicates across the code base.

Co-authored-by: Ben Hancock <benhancock859@gmail.com>
Deleted remaining TSFIXME comments that were not needed anymore
due to adding `unknown` type to custom `error` function.

Co-authored-by: Ben Hancock <benhancock859@gmail.com>
@dylanspyer dylanspyer requested a review from a team as a code owner October 15, 2024 14:30
@dylanspyer dylanspyer changed the title Chore/error types chore: created new type for API errors, created and implemented type predicates Oct 15, 2024
Copy link
Contributor

@DanielSLew DanielSLew left a comment

Choose a reason for hiding this comment

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

I think this is a simpler version of adding the type and it's less for the users to remember to implement as they add more error catching.

Comment on lines 303 to 312
interface APIError extends Error {
status: number
message: string
}

export const isAPIError = (errorObject: unknown): errorObject is APIError =>
errorObject instanceof Error && 'status' in errorObject && 'message' in errorObject

export const errorHasStatus = (errorObject: unknown, statusCode: number) =>
isAPIError(errorObject) && errorObject.status === statusCode
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
interface APIError extends Error {
status: number
message: string
}
export const isAPIError = (errorObject: unknown): errorObject is APIError =>
errorObject instanceof Error && 'status' in errorObject && 'message' in errorObject
export const errorHasStatus = (errorObject: unknown, statusCode: number) =>
isAPIError(errorObject) && errorObject.status === statusCode
export interface APIError extends Error {
status: number
message: string
}

src/utils/dev.ts Outdated
Comment on lines 67 to 69
isAPIError(error_)
? error(`Failed retrieving addons for site ${chalk.yellow(site.id)}: ${error_.message}. ${ERROR_CALL_TO_ACTION}`)
: error(error_)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
isAPIError(error_)
? error(`Failed retrieving addons for site ${chalk.yellow(site.id)}: ${error_.message}. ${ERROR_CALL_TO_ACTION}`)
: error(error_)
error(`Failed retrieving addons for site ${chalk.yellow(site.id)}: ${(error_ as APIError).message}. ${ERROR_CALL_TO_ACTION}`)

src/utils/framework-server.ts Outdated Show resolved Hide resolved
src/utils/hooks/requires-site-info.ts Outdated Show resolved Hide resolved
dylanspyer and others added 4 commits October 17, 2024 12:13
Co-authored-by: Daniel Lew <51924260+DanielSLew@users.noreply.github.com>
Co-authored-by: Daniel Lew <51924260+DanielSLew@users.noreply.github.com>
Co-authored-by: Ben Hancock <benhancock859@gmail.com>
Co-authored-by: Ben Hancock <benhancock859@gmail.com>
@dylanspyer
Copy link
Contributor Author

@DanielSLew Changed to type assertions for APIError

@DanielSLew DanielSLew self-requested a review October 18, 2024 19:38
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.

3 participants