fix: manage backend authXService errors #3190
Conversation
| @@ -45,6 +46,17 @@ export const getFormHelpers = | |||
| } | |||
| } | |||
|
|
|||
| export const getFormHelpersWithError = <T>( | |||
| form: FormikContextType<T>, | |||
| error?: Error | unknown, | |||
There was a problem hiding this comment.
I'm surprised error is optional in this fn.
There was a problem hiding this comment.
Ha, yeah. This will eventually replace getFormHelpers, but we want to be able to replace it piecemeal so we had to give it a different name. It's called withError because the functionality it adds is unpacking an error (so that we don't have to repeat that code everywhere). A different name would be fine though!
| @@ -107,42 +97,44 @@ export const SignInForm: FC<SignInFormProps> = ({ | |||
| validateOnBlur: false, | |||
| onSubmit, | |||
| }) | |||
| const getFieldHelpers = getFormHelpers<BuiltInAuthFormValues>(form) | |||
| const getFieldHelpers = getFormHelpersWithError<BuiltInAuthFormValues>(form, authError) | |||
There was a problem hiding this comment.
Maybe I'm being dense :) but why do we call this method with authError and not methodsError?
There was a problem hiding this comment.
methodsError is not linked to the sign-in form. This method attributes form errors to specific fields in the form.
| )} | ||
| {methodsError && ( | ||
| <ErrorSummary error={methodsError} defaultMessage={Language.methodsErrorMessage} /> | ||
| )} |
There was a problem hiding this comment.
A small concern I have with this pattern is that JSX complexity will grow 1:1 with API/Error complexity (if I am understanding this right). Let's say we had a more complex form that made a couple of different API calls (like 4). Would we be passing 4 additional props and rendering 4 of the above fragments?
There was a problem hiding this comment.
Yeah, we were debating whether handling multiple errors in the same component made sense, or if we could just render a separate ErrorSummary component for each error. Since we were not sure how common it was to have many parallel API calls with a single form, we thought this would be fine for the time being. The use case might get clearer as we explore more of the code base.
There was a problem hiding this comment.
Yes, that's right @Kira-Pilot. My reason for not wanting to coalesce the errors into one ErrorSummary was aesthetic (I didn't think two error summaries stuck together would look good given that sometimes they have collapsible parts). But you raise a good point. It might be a good idea to make our next ticket one that can render a ton of errors on one page in order to find out up front if it's a problem and what the solution would be.
There was a problem hiding this comment.
Thanks for the explanation. Leaving for now seems reasonable to me! We can cross the complexity bridge when we get there.
| authErrorMessage={authErrorMessage} | ||
| methodsErrorMessage={getMethodsError} | ||
| authError={authState.context.authError} | ||
| methodsError={authState.context.getMethodsError as Error} |
There was a problem hiding this comment.
Why do we have to override the compiler here?
There was a problem hiding this comment.
getMethodsError does not really throw an error from the backend, but only a generic one. This was being done earlier (in one of the lines we deleted), so just wanted to keep it the same for sanity (like referencing the message string from the Error object).
Additionally, I don't know a good way to produce a getMethodsError, so couldn't reeally play around with it. @kylecarbs do you have any suggestions about how the error could be reproduced?
There was a problem hiding this comment.
I don't really think we do have to override it. I'm guessing that ended up in there originally to make it so we didn't have to add | unknown in a component prop type. I had hoped we could narrow all the error types in the XServices but it turns out we want to leave them alone until they reach ErrorSummary and getFormHelpers, so I think we're just stuck with Error | unknown and we can delete this. I like the comment explaining that this call never returns an ApiError though.
|
Couple of questions but this is looking sweet! I think it will bring a lot of clarity to our forms. |
|
The UI snapshots with errors are not showing the specific form field errors, because the field is not "touched". @presleyp @Kira-Pilot Any ideas on how we can simulate that? Example |
Oh yeah, I've run into that before! I don't think I fixed it though haha. But let's see if there's a programmatic way to make it be touched. It looks like there is a way: https://formik.org/docs/api/formik#initialtouched-formiktouchedvalues. I don't know how easy that will be to work with given our custom util function - let me know if you want to pair on it. |
@AbhineetJain The best solution I can think of (and maybe there is a better one) is to add a new boolean prop to your component called If you decide to use this strategy, you should make sure to add a comment so future devs know that your new prop is for testing only. :) |
|
@presleyp @Kira-Pilot Thank you so much for these amazing suggestions. I used |
This PR shows backend errors from authXService on relevant pages/forms. The following errors were considered in this PR:
Subtasks
ErrorSummarycomponent to display the general error messageThere are still some unused errors that are stored in the authXService context. This PR makes some progress towards #3088
Screenshots