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

fix/improve setError & clearError #1907

Merged
merged 12 commits into from Jun 19, 2020
Merged

fix/improve setError & clearError #1907

merged 12 commits into from Jun 19, 2020

Conversation

bluebill1049
Copy link
Member

@bluebill1049 bluebill1049 commented Jun 18, 2020

setError

setError will focus one error at a time and remove confusing set multiple errors, behavior change.

  • setError will persis an error if it's not part of the form, which requires manual remove with clearError
  • setError error will be removed by validation rules, rules always take over errors

Reason

should focus an error at a time just like the name suggested, refine the API and remove multiple errors set.

- setError('test', 'test', 'test')
+ setErrror('test', { type: 'test, message: 'bill'})

clearError

Rename to clearErrors

Reason

This function allow remove multiple errors, so make sense to rename it to clearErrors

- clearError()
+ clearErrors()

fix #1908

@bluebill1049 bluebill1049 added the status: working in progress working in progress label Jun 18, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Jun 18, 2020

Size Change: -597 B (0%)

Total Size: 70.5 kB

Filename Size Change
dist/react-hook-form.es.js 14.3 kB -119 B (0%)
dist/react-hook-form.ie11.js 18.1 kB -117 B (0%)
dist/react-hook-form.js 14.3 kB -134 B (0%)
dist/react-hook-form.min.es.js 9.18 kB -77 B (0%)
dist/react-hook-form.umd.js 14.6 kB -150 B (1%)

compressed-size-action

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jun 18, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit b09faa9:

Sandbox Source
ecstatic-napier-uzytp Configuration
React Hook Form - Set Error / Clear Error Issue #1908
React Hook Form - Set Error / Clear Error Issue #1908

@bluebill1049 bluebill1049 requested review from kotarella1110, keiya01, stramel and JeromeDeLeon and removed request for kotarella1110 June 19, 2020 00:02
@bluebill1049 bluebill1049 removed the status: working in progress working in progress label Jun 19, 2020
@JeromeDeLeon
Copy link
Contributor

I think we could go back to this and setValue when users wanted setValues and setErrors to be implemented without the cause of extra re-renders, but I think it's fine for to make consistency around the API. 👍

@bluebill1049
Copy link
Member Author

I think we could go back to this and setValue when users wanted setValues and setErrors to be implemented without the cause of extra re-renders, but I think it's fine for to make consistency around the API. 👍

Totally agree, otherwise the API name is very confusing against its functionality.

['xxx', 'tss'].map(setError) should do the job

if users complains about the multiple re-render, we can then consider introducing setValues setErrors, ideally to avoid.

@bluebill1049 bluebill1049 changed the title fix setError fix/improve setError & clearError Jun 19, 2020
types: MultipleFieldErrors;
}
| {
message?: Message;
Copy link
Contributor

Choose a reason for hiding this comment

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

curious tho why is it message is not required?

Copy link
Member Author

Choose a reason for hiding this comment

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

just like register({ required: true })

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh make sense. 😆

src/useForm.ts Outdated
| {
message?: Message;
type: string;
},
Copy link
Contributor

Choose a reason for hiding this comment

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

could we create type ErrorOption or Error to reduce redundancy in type?

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds good to me 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

will do that after work :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I could do it for you. 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

commit please! 💋

Copy link
Member Author

Choose a reason for hiding this comment

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

Screen Shot 2020-06-19 at 3 16 27 pm

first-time see 8 in months...

Copy link
Contributor

@keiya01 keiya01 left a comment

Choose a reason for hiding this comment

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

I'm sorry for the delay in my review🙏

src/useForm.ts Outdated
shouldRender: true,
});
}
setInternalError({
Copy link
Contributor

Choose a reason for hiding this comment

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

setInternalError method is used inside only setError method. I think you can expand setInternalError method to setError method.

Copy link
Member Author

Choose a reason for hiding this comment

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

oh man! totally!

Copy link
Contributor

Choose a reason for hiding this comment

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

Man! didn't notice it before! 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @keiya01
image

we are 8 again. lol

Copy link
Member Author

Choose a reason for hiding this comment

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

I miss when the lib was 2k tho lol

Copy link
Contributor

Choose a reason for hiding this comment

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

It just means it becoming useful to others because they want to stick with it and push some new features to us. 😄

@@ -28,10 +28,7 @@ export default function shouldRenderBasedOnError<
const currentFieldError = get(error, name);
Copy link
Contributor

Choose a reason for hiding this comment

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

isManual is used inside unit test. I think you had better remove this variables from unit test.

@JeromeDeLeon
Copy link
Contributor

Haha you got me first @bluebill1049

@bluebill1049
Copy link
Member Author

Haha you got me first @bluebill1049

Thanks for your help @JeromeDeLeon 🙏

@JeromeDeLeon
Copy link
Contributor

Nah. It was nothing compared to you. My laptop stopped responding when I was about to test locally 🤯🤯

Copy link
Contributor

@keiya01 keiya01 left a comment

Choose a reason for hiding this comment

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

It's nice to have less code :)

}
}
};

function setError(name: FieldName<TFieldValues>, error: ErrorOption): void {
Copy link
Contributor

Choose a reason for hiding this comment

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

very nice!

@bluebill1049
Copy link
Member Author

@keiya01 @JeromeDeLeon happy to approve?

@bluebill1049 bluebill1049 merged commit acd6214 into master Jun 19, 2020
@bluebill1049 bluebill1049 deleted the feature/fix-setError branch June 19, 2020 06:37
@bluebill1049
Copy link
Member Author

Thanks guys for the review.

@2DSharp
Copy link

2DSharp commented Jun 19, 2020

I think that would make things clearer, thanks @bluebill1049

errorsRef.current = {};
reRender();
await callback(transformToNestObject(fieldValues), e);
} else {
errorsRef.current = fieldErrors;
errorsRef.current = {
Copy link

Choose a reason for hiding this comment

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

why need to merge the old error state?

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.

Inconsistent behavior of setError
5 participants