Support multiple errors messages #32

Paul-Vandell · 2020-07-19T14:05:06Z

100% test coverage for the yup resolver
Now errors have a types props with a Record key type and for values an array of error messages, if criteriaMode: "all"

At the same time:

Fix the abort early issue. It does not work because it let a yup resolver with an inner error of an empty array. So it always return any errors. Now we check if inner errors is an empty array or not. If no please return only one error at a time. Test has been done for this. it could help resolve issue from -> feat(context): inject context into yup context #30 (comment) where yup validation return anything.
Optimise the validate all field criteria props in the yup resolver script. The yup error path is always defined so i remove some useless code logic ( if i don't make a mistake )
The new props types is always defined it can be an empty array too.
Still compatible with the ErrorMessage component.
Add more types to the resolver script -> to be check im not an expert. I used types from rhf dist i think maybe it is not the right way :/

Improvements which can be done into

For an array, If we remove the transformToNestObject we will have at the same time array errors and all field errors inside this array.
Cons: errors duplication :

foo[0].test: {
      message: ...,
      type: ...,
      types: [ ] | {}
},
foo: [
  { 
    test: {
      message: ...,
      type: ...,
      types: [ ] | {}
    }
  },
  ...
]

Pros: it can be interesting to have it to get at the same time array errors and fields errors in.
We could maybe do : validateAllFieldCriteria ? call transformToNestObject
You can have example of use case here -> #26 (comment)
From issue -> #26

On first try to rhf we don't know where to place the context and where will be the changes !? We should remove the context option from the yup options by his types. It come in trouble when you expect your form validation to be used from cached context or from yup context options. From my issue -> Yup resolver validation seems to be unlinked with the form react-hook-form#2222. We could do this to force user to always use the context manage by rhf:

export const yupResolver = <TFieldValues extends Record<string, any>>(
  schema: Yup.ObjectSchema | Yup.Lazy,
  options: Omit<Yup.ValidateOptions, 'context'> = { // OMIT THE CONTEXT
    abortEarly: false,
  },

it could help issue like -> #30 (comment)

bluebill1049 · 2020-07-19T22:33:07Z

oh man! thanks very much for your amazing contribution, I will take a look at the PR. 🚀

pmaier983

LGTM! Great testing coverage!

bluebill1049 · 2020-07-20T01:17:28Z

Thanks @pmaier983 I will take a look at the PR tonight as well. solid effort @Vandell63 ❤️

kotarella1110 · 2020-07-20T03:52:47Z

src/yup.test.ts

+      expect(resolve.errors['password'].types).toMatchInlineSnapshot(`
+          Object {
+            "matches": Array [
+              "Special",
+              "Number",
+              "Uppercase",
+              "Lowercase",
+            ],
+            "min": Array [
+              "password must be at least 8 characters",
+            ],
+            "required": Array [
+              "password is a required field",
+            ],
+          }
+        `);
+    });


The structure I had in mind was:

expect(resolve.errors['password'].types).toMatchInlineSnapshot(` Object { "matches": Array [ "Special", "Number", "Uppercase", "Lowercase", ], "min": "password must be at least 8 characters", "required": "password is a required field", } `);

We should discuss whether all should be array message when validateAllFieldCriteria is true.

Thanks, @kotarella1110. Above is a breaking change, we should stick with

expect(resolve.errors['password'].types).toMatchInlineSnapshot(` Object { "matches": Array [ "Special", "Number", "Uppercase", "Lowercase", ], "min": "password must be at least 8 characters", "required": "password is a required field", } `);

@bluebill1049 I agree with you 👍

Yes, i was thinking if we keep this mix structure, i'm afraid that we have to handle the case where messages can be an array or a string in the code for every property in the schema. This is why i keep every messages in array format when validateAllCriteria is set to true. But you are in better place to take the right decision here :).

Thanks very much for understanding and putting this PR together!

Paul-Vandell · 2020-07-20T07:45:57Z

Thanks @pmaier983 I will take a look at the PR tonight as well. solid effort @Vandell63

Thx you . As much as the time you gave me when i was in trouble

kotarella1110 · 2020-07-20T15:07:35Z

src/__snapshots__/yup.test.ts.snap

+      "types": Object {
+        "required": "age is a required field",
+      },


If validateAllFieldCriteria is false, types field shouldn't be included.

kotarella1110 · 2020-07-20T15:08:15Z

src/__snapshots__/yup.test.ts.snap

+      "types": Object {
+        "min": "name is a min field",
+      },


Same as above

kotarella1110 · 2020-07-20T16:55:18Z

@Vandell63 I have created a PR (Paul-Vandell#1) to take my feedback comment above.
Please merge if no problem 🙏
After merging this PR, you are free to make changes.

Paul-Vandell · 2020-07-20T18:02:35Z

@kotarella1110 sry i was thinking that @bluebill1049 will make some changes too. So if i understood, you want the props types to be undefined if validateAllFieldCriteria is set to false. To resume :

Should i keep the yup resolver script that i made and then change it to match Your expected test result ? Or should i remove all my previous yup resolver changes and keep yours instead ?
Should i keep the mixed array and/or string types if validateAllFieldCriteria is set to true from your review @kotarella1110 ?
[Improvement] Should i remove the script transformToNestObject if validateAllFieldCriteria is set to true to get all fields error even if array have some ?
[Improvement] Should i force the user to inject his context in the rhf options instead of yup options with the Omit type ?
Should i re-add the script code to
Fix the abort early issue. It does not work because it let a yup resolver with an inner error of an empty array. So it always return any errors. Now we check if inner errors is an empty array or not
By checking if
Array.isArray(error.inner) && error.inner.length is true

kotarella1110 · 2020-07-21T03:31:31Z

Should i keep the yup resolver script that i made and then change it to match Your expected test result ? Or should i remove all my previous yup resolver changes and keep yours instead ?

I will commit the No.5 fix to the PR I created.
Array.isArray(error.inner) && error.inner.length is true
You can then merge.

Should i keep the mixed array and/or string types if validateAllFieldCriteria is set to true from your review @kotarella1110 ?

yes. I think we should keep mixed types.

[Improvement] Should i remove the script transformToNestObject if validateAllFieldCriteria is set to true to get all fields error even if array have some ?

What do @bluebill1049 think about this ?

[Improvement] Should i force the user to inject his context in the rhf options instead of yup options with the Omit type ?

This is under consideration. Please give me some time.

Should i re-add the script code to
Fix the abort early issue. It does not work because it let a yup resolver with an inner error of an empty array. So it always return any errors. Now we check if inner errors is an empty array or not
By checking if
Array.isArray(error.inner) && error.inner.length is true

we should fix this 👍

bluebill1049 · 2020-07-21T03:36:39Z

3. [Improvement] Should i remove the script transformToNestObject if validateAllFieldCriteria is set to true to get all fields error even if array have some ?

Yes totally 👍

bluebill1049 · 2020-07-21T03:38:26Z

Thanks @kotarella1110 for the review, sorry i was busy and haven't look close enough, i will make sure to do that tonight.

bluebill1049

Similar comments as @kotarella1110 thank for the PR 👍

bluebill1049 · 2020-07-21T04:03:46Z

src/yup.ts

+  Array.isArray(error.inner) && error.inner.length
+    ? error.inner.reduce<FieldErrors<TFieldValues>>(
+        (previous, { path, message, type }) => {
+          const previousPath = previous[path] as FieldErrors | undefined;


I think you can use this variable inline (save few bytes)

bluebill1049 · 2020-07-21T04:08:42Z

src/yup.ts

+                type,
+                types: previousTypes,
+              }),
+              types: {


types should be optional.

kotarella1110 · 2020-07-21T04:20:45Z

@Vandell63 @bluebill1049 please review this PR (Paul-Vandell#1).

Paul-Vandell · 2020-07-21T07:56:12Z

@Vandell63 @bluebill1049 please review this PR (Vandell63#1).

Done @kotarella1110

…ple-errors-message

Paul-Vandell · 2020-07-21T11:21:45Z

Thank @bluebill1049 @kotarella1110 merge done.
I will work on #32 (comment) and make the test for tonight.
And I wait for the point 4 with @kotarella1110 message : This is under consideration. Please give me some time.

Paul-Vandell · 2020-07-21T11:31:00Z

@bluebill1049 @kotarella1110 Oh i maybe forget this. In yup errors the path seems to be never undefined so should i remove path logic from the code. Or should i keep it and make a test on it ?

Paul-Vandell · 2020-07-21T11:35:45Z

I finally added test and #32 (comment) logic.
Pending for my previous messages.

bluebill1049 · 2020-07-21T11:38:28Z

[Improvement] Should i force the user to inject his context in the rhf options instead of yup options with the Omit type ?

I think it would be better to centralize the context at useForm, that way our API is consistent through other resolvers as well. what do you think @kotarella1110?

bluebill1049 · 2020-07-21T11:39:19Z

@bluebill1049 @kotarella1110 Oh i maybe forget this. In yup errors the path seems to be never undefined so should i remove path logic from the code. Or should i keep it and make a test on it ?

actually, from memory they do get undefined in certain circumstance, I can't remember when :(

kotarella1110 · 2020-07-21T11:58:28Z

[Improvement] Should i force the user to inject his context in the rhf options instead of yup options with the Omit type ?

I think it would be better to centralize the context at useForm, that way our API is consistent through other resolvers as well. what do you think @kotarella1110?

@bluebill1049 Totally agree 👍
@Vandell63 Let's remove the context option from yupResolver option.

Paul-Vandell · 2020-07-21T13:17:31Z

So @bluebill1049 @kotarella1110 should you prefer:

add the Omit typescript: Omit<Yup.ValidateOptions, 'context'>
delete options.context
Add a console warn to the user if context is passed ( for non typescript dev ) to yup options on non-production build ?

It can be all of them.
I let you me know what you prefer and then :

I will add the missing test statement if yup path is undefined.
Refactor context to be only the rhf context based of your choices.
Test the logic based of your choices.

kotarella1110 · 2020-07-21T14:14:14Z

#32 (comment)

@Vandell63 Does option 2 mean this?

 values:  await schema.validate(values, {
-   context,
    ...options,
+   context,
  }) as any,

I prefer all of them.
Let's wait for @bluebill1049 's opinion.

Paul-Vandell · 2020-07-21T14:33:49Z

Oh sorry @kotarella1110 , no in the code it could be

try {
  if(options.context){
    console.warn("please use rhf context option instead")
    delete options.context
  }
  process()
} ...

Remove context property from yup option if user is not in typescript it could still add context props into. Just to be sure.

bluebill1049 · 2020-07-22T03:15:00Z

shall we do 1 and 3?

for TS user just a type
for JS user just a console.warn for dev only

if (process.env.NODE_ENV ==='development') {
 console.warn('xxx');
}

kotarella1110 · 2020-07-22T03:24:23Z

@bluebill1049 I agree 👍
@Vandell63 Also, we prefer the context passed from useForm over the options.context of yupResolver, so I think it should be fixed as I described in this #32 (comment).

Paul-Vandell · 2020-07-22T10:50:27Z

@bluebill1049 @kotarella1110 ok for me i will propose you a final commit with test as soon as i can .

bluebill1049 · 2020-07-22T10:52:18Z

@bluebill1049 @kotarella1110 ok for me i will propose you a final commit with test as soon as i can .

Thank you very much. no rush whenever you are free.

Paul-Vandell · 2020-07-22T10:53:37Z

@bluebill1049 a pleasure for me no worry

Paul-Vandell · 2020-07-22T18:52:21Z

@bluebill1049 @kotarella1110 there we go.

100% test coverage
Remove context type from the yup options
Add warn log if yup context is defined
Maybe you will have to change the message, im bit crappy in english :/ .
Tell me if you want a special squash commit with a rebase to finish it.

kotarella1110

LGTM 🎉

bluebill1049 · 2020-07-23T06:53:30Z

Let's go! thanks @kotarella1110 's review and @Vandell63's hard work! do you have a twitter account? let's tweet this new feature.

Paul-Vandell · 2020-07-23T11:01:14Z

Yess it was so cool thx . let's go this new fresh twitter -> https://twitter.com/Paul_Vandell.
Thank you again for your high availability @bluebill1049 @kotarella1110

sngreg · 2020-07-23T12:16:48Z

@Vandell63 @bluebill1049 @kotarella1110 - THANK you so much this code looks great. Sorry I went MIA for a couple of days, I was out of the office on vacation.

This feature is a GREAT add to this repo!

Paul-Vandell added 2 commits July 19, 2020 16:34

support-multiple-errors-message

4e149bf

rebase rhf/resolver master

dba4bb9

Paul-Vandell force-pushed the feature/support-multiple-errors-message branch from c0977e0 to dba4bb9 Compare July 19, 2020 14:35

Paul-Vandell changed the title ~~support-multiple-errors-messages~~ Support multiple errors messages Jul 19, 2020

Paul-Vandell mentioned this pull request Jul 19, 2020

Question: How to do multiple checks for password validation with Yup and RFH? #26

Closed

bluebill1049 requested review from bluebill1049, kotarella1110 and pmaier983 July 19, 2020 22:32

pmaier983 approved these changes Jul 20, 2020

View reviewed changes

kotarella1110 reviewed Jul 20, 2020

View reviewed changes

take feedback comment

97d43af

kotarella1110 mentioned this pull request Jul 20, 2020

take feedback comment Paul-Vandell/resolvers#1

Merged

kotarella1110 added 2 commits July 21, 2020 12:40

improve parseErrorSchema

9b91c0a

fix early issue

8367d6a

bluebill1049 reviewed Jul 21, 2020

View reviewed changes

refactor parseErrorSchema

1ce4bbd

remove true because message is non undefined

fabc1ab

kotarella1110 added 2 commits July 21, 2020 18:10

improve unit test

1836657

reduce code size by a few bytes

a43f559

Merge branch 'kotarella1110-take-feedback' into feature/support-multi…

d720aa2

…ple-errors-message

feat(yup): add validate all field criteria logic on parsed errors

e33e7f2

feat(context): prevent user to not used the yup option context

e998e69

Paul-Vandell force-pushed the feature/support-multiple-errors-message branch from b08757d to e998e69 Compare July 22, 2020 18:55

kotarella1110 approved these changes Jul 23, 2020

View reviewed changes

bluebill1049 merged commit bafb36d into react-hook-form:master Jul 23, 2020

Support multiple errors messages #32

Support multiple errors messages #32

Conversation

Paul-Vandell commented Jul 19, 2020 • edited Loading

bluebill1049 commented Jul 19, 2020

pmaier983 left a comment

Choose a reason for hiding this comment

bluebill1049 commented Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

bluebill1049 Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Paul-Vandell commented Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kotarella1110 commented Jul 20, 2020

Paul-Vandell commented Jul 20, 2020 • edited Loading

kotarella1110 commented Jul 21, 2020

bluebill1049 commented Jul 21, 2020

bluebill1049 commented Jul 21, 2020

bluebill1049 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kotarella1110 commented Jul 21, 2020

Paul-Vandell commented Jul 21, 2020 • edited Loading

Paul-Vandell commented Jul 21, 2020 • edited Loading

Paul-Vandell commented Jul 21, 2020

Paul-Vandell commented Jul 21, 2020

bluebill1049 commented Jul 21, 2020

bluebill1049 commented Jul 21, 2020

kotarella1110 commented Jul 21, 2020

Paul-Vandell commented Jul 21, 2020 • edited Loading

kotarella1110 commented Jul 21, 2020 • edited Loading

Paul-Vandell commented Jul 21, 2020

bluebill1049 commented Jul 22, 2020 • edited Loading

kotarella1110 commented Jul 22, 2020

Paul-Vandell commented Jul 22, 2020

bluebill1049 commented Jul 22, 2020

Paul-Vandell commented Jul 22, 2020

Paul-Vandell commented Jul 22, 2020 • edited Loading

kotarella1110 left a comment

Choose a reason for hiding this comment

bluebill1049 commented Jul 23, 2020

Paul-Vandell commented Jul 23, 2020

sngreg commented Jul 23, 2020

Paul-Vandell commented Jul 19, 2020 •

edited

Loading

bluebill1049 commented Jul 20, 2020 •

edited

Loading

bluebill1049 Jul 20, 2020 •

edited

Loading

Paul-Vandell commented Jul 20, 2020 •

edited

Loading

Paul-Vandell commented Jul 20, 2020 •

edited

Loading

Paul-Vandell commented Jul 21, 2020 •

edited

Loading

Paul-Vandell commented Jul 21, 2020 •

edited

Loading

Paul-Vandell commented Jul 21, 2020 •

edited

Loading

kotarella1110 commented Jul 21, 2020 •

edited

Loading

bluebill1049 commented Jul 22, 2020 •

edited

Loading

Paul-Vandell commented Jul 22, 2020 •

edited

Loading