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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

fixed useActionData overriding error clearing #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

88 changes: 88 additions & 0 deletions src/hook/index.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,18 @@ import {
} from "@testing-library/react";
import { RemixFormProvider, useRemixForm, useRemixFormContext } from "./index";
import React from "react";
import * as remixRun from "@remix-run/react";

const submitMock = vi.fn();
vi.mock("@remix-run/react", () => ({
useSubmit: () => submitMock,
useActionData: () => ({}),
}));

const mockUseActionData = vi
.spyOn(remixRun, "useActionData")
.mockImplementation(() => ({}));

describe("useRemixForm", () => {
it("should return all the same output that react-hook-form returns", () => {
const { result } = renderHook(() => useRemixForm({}));
Expand Down Expand Up @@ -116,4 +121,87 @@ describe("RemixFormProvider", () => {

expect(spy).toHaveBeenCalled();
});

it("should merge useActionData error on submission only", async () => {
const mockError = {
userName: { message: "UserName required", type: "custom" },
};

const enum Value_Key {
USERNAME = "userName",
SCREEN_NAME = "screenName",
}

const defaultValues = {
[Value_Key.USERNAME]: "",
[Value_Key.SCREEN_NAME]: "",
};

const { result, rerender } = renderHook(() =>
useRemixForm({
mode: "onSubmit",
reValidateMode: "onChange",
submitConfig: {
action: "/submit",
},
defaultValues,
})
);

// Set useActionData mock after initial render, to simulate a server error
mockUseActionData.mockImplementation(() => mockError);

act(() => {
result.current.setValue(Value_Key.SCREEN_NAME, "priceIsWrong");
});

act(() => {
result.current.handleSubmit();
});

// Tests that error message is merged.
await waitFor(() => {
expect(result.current.formState.errors[Value_Key.USERNAME]?.message).toBe(
mockError[Value_Key.USERNAME].message
);
});

act(() => {
result.current.setValue(Value_Key.USERNAME, "Bob Barker");
// Simulates revalidation onChange
result.current.clearErrors(Value_Key.USERNAME);
});

rerender();

// This test that error is cleared after state change and not reemerged from useActionData
await waitFor(() => {
expect(result.current.getValues(Value_Key.USERNAME)).toBe("Bob Barker");
});

await waitFor(() => {
expect(
result.current.formState.errors[Value_Key.USERNAME]?.message
).toBeUndefined();
});

const newScreenName = "CaptainJackSparrow";

act(() => {
result.current.setValue(Value_Key.SCREEN_NAME, newScreenName);
});

// This test that other state changes do not reemerged from useActionData
await waitFor(() => {
expect(result.current.getValues(Value_Key.SCREEN_NAME)).toBe(
newScreenName
);
});

await waitFor(() => {
expect(
result.current.formState.errors[Value_Key.USERNAME]
).toBeUndefined();
});
});
});
14 changes: 12 additions & 2 deletions src/hook/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from "react";
import React, { useState } from "react";
import { SubmitFunction, useActionData, useSubmit } from "@remix-run/react";
import {
SubmitErrorHandler,
Expand Down Expand Up @@ -35,9 +35,11 @@ export const useRemixForm = <T extends FieldValues>({
const submit = useSubmit();
const data = useActionData();
const methods = useForm<T>(formProps);
const [formSubmitted, setFormSubmitted] = useState(false);

// Submits the data to the server when form is valid
const onSubmit = (data: T) => {
setFormSubmitted(true);
submit(createFormData({ ...data, ...submitData }), {
method: "post",
...submitConfig,
Expand All @@ -62,7 +64,15 @@ export const useRemixForm = <T extends FieldValues>({
isLoading,
} = formState;

const formErrors = mergeErrors<T>(errors, data?.errors ? data.errors : data);
const onMerge = () => {
setFormSubmitted(false);
};

// Will only merge data from useActionData if form was just submitted and only make
// formSubmitted false if data exist to useActionData to account for multiple renders
const formErrors = formSubmitted
? mergeErrors<T>(errors, data, onMerge)
: errors;

return {
...methods,
Expand Down
7 changes: 5 additions & 2 deletions src/utilities/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,9 +169,10 @@ The function recursively merges the objects and returns the resulting object.
*/
export const mergeErrors = <T extends FieldValues>(
frontendErrors: Partial<FieldErrorsImpl<DeepRequired<T>>>,
backendErrors?: Partial<FieldErrorsImpl<DeepRequired<T>>>
backendErrors?: Partial<FieldErrorsImpl<DeepRequired<T>>>,
onMerge?: () => void
) => {
if (!backendErrors) {
if (!backendErrors || (onMerge && Object.keys(backendErrors).length === 0)) {
Copy link
Author

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 });
  }
 ...

Copy link
Author

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

Copy link
Contributor

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

return frontendErrors;
}

Expand All @@ -189,5 +190,7 @@ export const mergeErrors = <T extends FieldValues>(
}
}

onMerge && onMerge();
Copy link
Contributor

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.

Copy link
Author

@Centerworx Centerworx Aug 7, 2023 •
edited
Loading

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.

Copy link
Contributor

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)

Copy link
Author

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;


return frontendErrors;
};