-
Notifications
You must be signed in to change notification settings - Fork 4
(ts-api-react-actions) Enhance error handling in API actions and hooks with user-facing error mapping #1616
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
base: 11-19-refactor_api_methods_to_remove_unnecessary_async_await_and_improve_return_handling
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
| import { useApiAction } from "./useApiAction"; | ||
|
|
||
| /** | ||
| * Props for the `ApiAction` component that exposes an async submit handler. |
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.
Nitpick: low value comment
| * Props for the `ApiAction` component that exposes an async submit handler. | ||
| */ | ||
| export interface ApiActionProps< | ||
| Params extends object, |
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.
Off-scope for this PR, but extends object is a footgun as it doesn't really mean anything since everything except primitives are objects. (I.e. it's not like saying something is a Dict in Python.)
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.
There was some good reason for this, but can't remember anymore. Was probably something to do with how the ts-api fetch function arguments work
|
|
||
| // As of this moment ApiActions sole purpose is to gracefully handle errors from API calls | ||
| /** | ||
| * Component that wraps an API endpoint in a stable submit callback with mapped errors. |
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.
Nitpick: maybe it's just me but I have hard time grasping this comment.
| import { ApiEndpointProps } from "@thunderstore/thunderstore-api"; | ||
|
|
||
| /** | ||
| * Configuration for wiring `useApiAction` to a specific API endpoint. |
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.
Nitpick: generally speaking I don't know if "FooArgs" or "BarProps" benefit that much for having a docstring. "Foo" and "Bar" are more natural places to comment on what the thing actually does (if any extra clarification is needed).
| props: ApiEndpointProps<Params, QueryParams, Data> | ||
| ): Promise<Awaited<Return>> => { | ||
| const result = await apiCall(props); | ||
| return result as Awaited<Return>; |
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.
According to my IDE this as assertion is not required, and you should avoid unnecessary assertions as they can hide bugs.
|
|
||
| interface UseStrongFormProps< | ||
| Inputs, | ||
| type IsExact<A, B> = (<T>() => T extends A ? 1 : 2) extends <T>() => T extends B |
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.
Now here I would welcome a good docstring explaining IsExact, RefinerRequirement, and ErrorMapperRequirement.
|
|
||
| export function useStrongForm< | ||
| /** | ||
| * Configuration for wiring a StrongForm instance to refiners, submitters and lifecycle hooks. |
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.
Nitpick: but instead I get "here be props" and "here be return type" level comments 😔 A hard-knock life.
| return mapApiErrorToUserFacingError(error); | ||
| }; | ||
|
|
||
| const mapError: (error: unknown) => SubmissionError = |
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.
My IDE flags this for type error, and I don't know why this variable is even used since it's called in only one place, and why it's defined here since the call site comes way later.
| const mapError: (error: unknown) => SubmissionError = | ||
| props.errorMapper ?? defaultErrorMapper; | ||
|
|
||
| useEffect(() => { |
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.
The whole strongForm concept confuses me to the level that I don't even attempt to review it with my fried Friday brain.
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.
You know, it confuses me nowadays too. But it does work
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.
Could be even better though
| formatUserFacingError, | ||
| } from "@thunderstore/thunderstore-api"; | ||
|
|
||
| export type UseFormToasterArgs<OnSubmitSuccessDataType, OnSubmitErrorDataType> = |
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.
Off-scope but my gut feeling is that the whole useFormToaster is overengineered on the concept level and quite confusing if the point is to just show toasts.
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.
You are not incorrect, but I don't have a solution off the top of my head, probably a worthy non-mission-critical task
06e02e3 to
e40ef67
Compare
cd40580 to
5ac0d36
Compare
…s with user-facing error mapping
…ing error mapping
5ac0d36 to
de2572f
Compare
e40ef67 to
ee03845
Compare

(ts-api-react-actions) Enhance error handling in API actions and hooks with user-facing error mapping
Refactor loader functions to enhance error handling and move to different file
Enhance error handling in StrongForm and useFormToaster with user-facing error mapping