-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
fixed useActionData overriding error clearing #15
Conversation
@@ -189,5 +190,7 @@ export const mergeErrors = <T extends FieldValues>( | |||
} | |||
} | |||
|
|||
onMerge && onMerge(); |
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.
maybe I am missing something but wouldn't this trigger a re-render right after merging errors and remove the invalid state right away? I think the real solution is to allow you to remove the errors when you want via a reset function instead.
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.
React-hook-form does this already, but useActionData will re-add it on the state change with the previous implementation.
This is by design, as remix holds the useActionData in a props and not state, so you can't remove it. The only way to clear the useActionData is to resubmit the action.
React-hook-forms clears the error, which triggers a re-render, so the error is re-add to the UI, because a new action was not triggered. So the previous data errors persist in the newly generated fieldErrors.
To answer your question, this implementation will not clear the errors that where added since they are already on fieldError prop and added to the react-hook-form error state. So on the re-render fieldErrors is past back, containing all of the front and backend errors.
This ends up being more efficient as the merge function is on run till data is sent back from the backend. If the backend doesn't send back an anything or there is no data, it doesn't matter if submitted state is still true. Since there is nothing to add to the fieldErrors. MergeErrors does an early return anyway. This implementation also address the potential problem with multiple actions triggering and returning values.
The last scenario is that the action has redirected the user away, which makes the form irrelevant.
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.
This should not be our decision to make I think, I would augment the reset form handler only, so instead of it reseting the form only it also sets the flag to false, and the be errors will be passed and merged only when the flag is true (which happens from the moment you successfully submit to the moment you reset the form)
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 reset the form
What do you mean by the above? Do you mean call reset?
Note, The above configuration restores the expected behavior of react-hook-form. I agree that we should add a method to allow the user to restore the default behavior of remix-run useActionData though, so how about we add restorePersistedActionData: boolean;
) => { | ||
if (!backendErrors) { | ||
if (!backendErrors || (onMerge && Object.keys(backendErrors).length === 0)) { |
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.
Do you think the above change would block 2nd level object errors form being added?
eg. errors: { userName: { message: "Email is required", type: "custom" } }
This will not add.
export const action = async ({ request }: ActionArgs) => {
const { errors, data } = await getValidatedFormData<FormData>(
request,
resolver
);
if (errors) {
return json({ errors, ok: true });
}
...
It has to be changed to the below, to log errors:
export const action = async ({ request }: ActionArgs) => {
const { errors, data } = await getValidatedFormData<FormData>(
request,
resolver
);
if (errors) {
return json({ ...errors, ok: true });
}
...
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.
Needs some changes closing pr for now
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.
well that as well now that I look at it, this will be called recursively and will stop errors from being added as soon as the onMerge is hit. the mergeErrors utility is not the problem at all I think but rather the fact that the errors are not ignored
Fixes issue #12
The reseting of submit state needs to happen in the mergeErrors function so that it will not clear the state till after the data has been added to the error.
Note: Something weird happened with the updated version. I had to spread errors in the action to get the errors to show up on the form. Did that change recently?