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

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

Closed
sngreg opened this issue Jul 13, 2020 · 85 comments
Closed
Labels
enhancement New feature or request

Comments

@sngreg
Copy link

sngreg commented Jul 13, 2020

I am wondering if there is a way with yup and yupResolver with RHF to do something similar to this:

https://codesandbox.io/s/eager-bush-dfl5t

Basically using this code when a user starts typing into the password field you get separated booleans for each validation criteria which allows you to build an interactive component such as:

Screen Shot 2020-07-13 at 7 40 26 PM

Code that I have been playing around with:

....
import { yupResolver } from '@hookform/resolvers';

const passwordSchema = yup.object().shape({
  password: yup
    .string()
    .required()
    .min(8)
    .matches(RegExp('(.*[a-z].*)'), 'Lowercase')
    .matches(RegExp('(.*[A-Z].*)'), 'Uppercase')
    .matches(RegExp('(.*\\d.*)'), 'Number')
    .matches(RegExp('[!@#$%^&*(),.?":{}|<>]'), 'Special'),
});

  const { register, handleSubmit, errors, formState, getValues } = useForm({
    resolver: yupResolver(
      passwordSchema
    ),
    criteriaMode: 'all',
    reValidateMode: 'onChange',
    mode: 'onChange',
  });
....

Which results in:

Screen Shot 2020-07-13 at 7 47 39 PM

I was hoping to have types returned like the coding sample so I can flag the realtime critera.

Any help or things in yup would be super helpful.

Thanks!

@sngreg sngreg changed the title Password Validation, multiple checks Question: How to do multiple checks for password validation with Yup and RFH? Jul 13, 2020
@bluebill1049
Copy link
Member

we do actually support validateAllFieldCriteria https://github.com/react-hook-form/resolvers/blob/master/src/yup.ts otherwise it would be a bug if it's not working.

@sngreg
Copy link
Author

sngreg commented Jul 14, 2020

Thanks @bluebill1049 for the quick reply. Going to try that quickly now to see if that is what I am looking for. Will post here and close out if it works :)

@sngreg
Copy link
Author

sngreg commented Jul 14, 2020

I am pretty new to RFH and Yup looking at the types, I am not seeing validateAllFieldCriteria

// Not Correct
 resolver: yupResolver(
      passwordSchema,
      { validateAllFieldCriteria: true }
    ),

This is what it is expecting:

export declare const yupResolver: <TFieldValues extends Record<string, any>>(schema: Yup.ObjectSchema, options?: Yup.ValidateOptions) => Resolver<TFieldValues, object>;

export interface ValidateOptions {
    /**
     * Only validate the input, and skip and coercion or transformation. Default - false
     */
    strict?: boolean;
    /**
     * Return from validation methods on the first error rather than after all validations run. Default - true
     */
    abortEarly?: boolean;
    /**
     * Remove unspecified keys from objects. Default - false
     */
    stripUnknown?: boolean;
    /**
     * When false validations will not descend into nested schema (relevant for objects or arrays). Default - true
     */
    recursive?: boolean;
    /**
     * Any context needed for validating schema conditions (see: when())
     */
    context?: object;
}

I will do some more digging / googling, but just wanted. to let you know what I was trying.

@Mathius17
Copy link

Mathius17 commented Jul 14, 2020

Hey @sngreg! I think if you add abortEarly: false to the options when declaring the resolver it might solve your problem. It is true by default.

resolver: yupResolver(passwordSchema, { abortEarly: false })

I haven't actually tested it but it seems like the solution to me.

@sngreg
Copy link
Author

sngreg commented Jul 14, 2020

@Mathius17 - Thanks for the suggestion. I did set it to false and am experiencing the same result above, where the password errors will trigger, but only shows one of each "type" so for reference: I have 4 different .matches regex but it only fires the first one.

Screen Shot 2020-07-13 at 7 47 39 PM

Also. for reference - it looks like the YupResolver defaults aboutEarly:false already - https://github.com/react-hook-form/resolvers/blob/master/src/yup.ts#L45

@Paul-Vandell
Copy link
Contributor

Paul-Vandell commented Jul 14, 2020

Hi @sngreg so yes for the validate all criteria it is the criteriaMode need to be 'all'.
If you abortEarly to true you will only get the first error Property in your schema
If you abortEarly to false you will get all yup errors from every properties.
So your question is more about many errors for only one props, and after investigate it yup only send you back the first error property you got from your schema, and if it successfull, it will take the next one ( the next yup validation of the same property ) .
And so yup don't this feature that give you all props errors in an array.
So it is more a yup next waiting feature.
Infortunetly -> jaredpalmer/formik#243 there is no change today for this :/.

@sngreg
Copy link
Author

sngreg commented Jul 14, 2020

Thanks @Vandell63 - I was afraid this might be the case after all the digging I was doing. I appreciate everyone jumping into this discussion.

@Paul-Vandell
Copy link
Contributor

You welcome, i think you can close it, and as soon as yup want to add this feature, i'm pretty sure that @bluebill1049 will be already on :) .

@sngreg
Copy link
Author

sngreg commented Jul 14, 2020

@Vandell63 @bluebill1049 - Thank you again for the really quick responses.

Vandell63 - I opened an issue on yup (jquense/yup#975) which it looks like this is not a problem in yup. Here is a very quick sample that shows ALL the errors when we call yup.validate.

https://codesandbox.io/s/nice-brown-61nzg

Notice in the console:

Error - password is a required field,password must be at least 8 characters,Lowercase,Uppercase,Number,Special

I believe this something with how RHF or the resolver handles these errors and makes the object to return

Thoughts?

@Paul-Vandell
Copy link
Contributor

@sngreg Im so sorry to lead you to the bad way. Effectively yup return an array of all errors ( log on codesandbox an Error class lead me to a weird log ).
I continue investigate into the yup resolver code.
I already started this -> https://codesandbox.io/s/yup-resolver-with-errors-array-pvph7?file=/src/resolver.js

@sngreg
Copy link
Author

sngreg commented Jul 14, 2020

@Vandell63 no worries at all! Glad we are able to narrow this down. Thanks for starting that, i have narrowed it down to the parseErrorSchema method. It looks like that is where the "BUG" or "FEATURE REQUEST" would be. It is grouping and only saving one of each "type" in the "types" node.

EX:

Parse Inner Reduce ----
PATH: password MESSAGE: password is a required field TYPE: required
PATH: password MESSAGE: password must be at least 8 characters TYPE: min
PATH: password MESSAGE: Lowercase TYPE: matches
PATH: password MESSAGE: Uppercase TYPE: matches
PATH: password MESSAGE: Number TYPE: matches
PATH: password MESSAGE: Special TYPE: matches

This is the start of the inner.reduce, but after what the function returns is:

Screen Shot 2020-07-13 at 7 47 39 PM

Which only has one of the "Matches" TYPES

@sngreg
Copy link
Author

sngreg commented Jul 15, 2020

I believe this is where the error is happening, it only appends the latest error of a type:

https://github.com/react-hook-form/resolvers/blob/master/src/yup.ts#L15

Looking at what this does https://github.com/react-hook-form/react-hook-form/blob/173e0c1740108d23101681d79000708c5a2b7778/src/logic/appendErrors.ts#L21

It overwrites the type match with the latest error in the array.

I tested this out adding a random number to type in the resolver and it. "solved" the issue.

I am not sure the best approach to where a permeant should live, is appendError in RFH functioning as intended (not to duplicate types) or is this something we should fix in the resolver?

Thoughts?

@bluebill1049
Copy link
Member

thanks for the invitation @sngreg i haven't got time to look too deep on this one, do you have any fix in mind since you already traveled this deep into the code.

@sngreg
Copy link
Author

sngreg commented Jul 15, 2020

Thanks Bill! I don’t have a fix in mind. I know why it is broken but not sure how or where to fix. I know it is the appendError in RHF code.

One thought if we are fixing in resolvers can we build a unique list before appending them, or build our own error list.

@Paul-Vandell
Copy link
Contributor

Paul-Vandell commented Jul 15, 2020

@sngreg Yes this is the case for the appendError, it replace the previous one every time. In my code sandbox link you can see in the resolver file that i have remove the appendError to return the first error if validateAllCriteria is set to firstError. And now i will work on the all from validate all criteria to show you instead a list of error with the same shape of the firstError.

[path]: validateAllFieldCriteria
                  ? // RETURN AN ARRAY OF MESSAGE AND TYPE INSTEAD if it is TRUE
                    [...(previous[path] || []), { message, type }]
                  : { ...previous[path], message, type }

In the code if you switch the criteriaMode: "all", -> in log you will get the Array of you errors that you needed.
So if you switch to all you will get the error list directly. But i have to investigate the goal of the function transformToNestObject to make it working properly you got it ?

@bluebill1049
Copy link
Member

Thanks, @Vandell63 for all the support and investigation. I am really appreciated it here.

@Paul-Vandell
Copy link
Contributor

@bluebill1049 is totally normal, you are really active and we all like this. And i have to switch from formik to this library ( performance reason ). And no way for me i stay here.
I need to know how react-hook-form handle the yup resolver behind because instead Object error he will get an array of error so it should impact the react-hook-form behind. I continue

@Paul-Vandell
Copy link
Contributor

Paul-Vandell commented Jul 15, 2020

@bluebill1049 & @sngreg I got a working example of @sngreg needs on

                [path]: validateAllFieldCriteria
                  ? [...(previous[path] || []), { message, type }]
                  : { ...previous[path], message, type }

Et voilà. But im pretty sure that it will work for this issue. I don't know how it could impact into the react-hook-form library. To be tested.

@bluebill1049
Copy link
Member

yes i think we can remove: transformToNestObject. can you create a PR and let's do some further testing and see if this impact other stuff or not.

@Paul-Vandell
Copy link
Contributor

@bluebill1049 same here i will give a try :)

@sngreg
Copy link
Author

sngreg commented Jul 15, 2020

@Vandell63 - Thank you so much, taking a look now to see what you made and trying it out in my application.

@sngreg
Copy link
Author

sngreg commented Jul 15, 2020

@bluebill1049 @Vandell63 This is exactly what I needed. Things are looking good in my application, but I think the way we have this now will introduce a breaking change for people that have coded for the old type:

Now all the fields in the schema return an array of errors (GREAT!) but prior to this change their was the top level error exposed and then an object called types to. see the "partial / broken" list

BEFORE:
Screen Shot 2020-07-13 at 7 47 39 PM

AFTER: (Note the screenshots are not 1-1 in regards to the same schema, but you understand the thought behind it)
Screen Shot 2020-07-15 at 8 41 47 AM

So if anyone is display label / field messages for their form it will break because errors.lastName.message now has to be errors.lastName[0].message

One thought to enhance this change and not introduce it as a breaking change would be an object like this

{
  email:
    message: 'Email  is Required'
    type: 'min'
    errors: [
      {
        message: 'Email is Required'
        type: 'required'
      },
      {
        message: 'Email is not valid format'
        type: 'email'
      }
    ]
}

@bluebill1049
Copy link
Member

Thanks guys. I think we need to keep the same object shape, otherwise, it's a breaking change not just the resolve but he core as well:
image

@sngreg
Copy link
Author

sngreg commented Jul 15, 2020

@bluebill1049 - So with the thoughts of keeping the object the same, I agree.

We run into issues using the type as the key in types object. Do you think we can keep the object the same and then add on just an errors array or would your thought be to make the keys unique in the types key / value pair like this:

matches-1: 'Special'
matches-2: ' Number'
...

@Paul-Vandell
Copy link
Contributor

Hey after a diving session into react hook form i didnt find any deps with the props types from the resolver into the core so it could be used easily. It was not documented and maybe could be a soft breaking change for any user which used the types props in errors ( i don't think many of them ).
I have updated the code -> https://codesandbox.io/s/yup-resolver-with-errors-array-pvph7?file=/src/App.js
And it still do the job. What you think about this @bluebill1049 @sngreg ?

@sngreg
Copy link
Author

sngreg commented Jul 15, 2020

@Vandell63 - this looks great to me! I like that we keep the same object type and just repurpose the types array with “all” the validation criteria.

I am new to this repo, but I think you should make a PUll request with this code and see what @bluebill1049 thinks!

I appreciate all the time, intelligent discussion, and collaboration we all put towards this feature.

@Paul-Vandell
Copy link
Contributor

@sngreg thx same feeling.
I can propose at the same time the context at the second argument which is not inject into resolver when user fill the object context into the resolver object ( and not in the yupResolver option ) -> react-hook-form/react-hook-form#2222 (comment) sounds good to you @bluebill1049 with the code change ?

@bluebill1049
Copy link
Member

yea totally! thanks for all the investigations. looking forward to your PRs, this is going to help some many developers.

@sngreg
Copy link
Author

sngreg commented Jul 16, 2020

Thanks @Vandell63 - Bill's always one step ahead of me!

@Paul-Vandell
Copy link
Contributor

Paul-Vandell commented Jul 16, 2020

@sngreg i think we have to wait for a decision and the PR too to be sync on . And take in mind that the ui do not refresh on new yup errors from the same types. It work when message was a Array. So the current example refresh the ui if one of the key appear or disappear base on yup errors.
Wating here your final though and decision @bluebill1049 @kotarella1110 :)

@sngreg
Copy link
Author

sngreg commented Jul 16, 2020

Thanks @Vandell63 - In my use case, I will need the UI to refresh to give live feedback on the password criteria. I agree, lets see what Bill and Kotarella1110 has to say in RFH and then work on our solution

@sngreg
Copy link
Author

sngreg commented Jul 17, 2020

@bluebill1049 @Vandell63 - looks like the PR is merged in RHFs (thanks for doing that) -

Bill - would you like us to submit a PR to this repo now with the final structure that we talked about using types and message arrays? I still think this resolver should return an array for every message in the types key (like we were going to do for messages)

@bluebill1049
Copy link
Member

Screen Shot 2020-07-17 at 9 21 27 pm

let's follow this, unless if there is anything that we are missing.

@sngreg
Copy link
Author

sngreg commented Jul 17, 2020

@Vandell63 okay that works, so basically, the type is the key and if there is only one message it is

[type]: Message

But if multiple messages it is

[type]: Message[]

That works for me, I just like to be consistent as possible with value types but works for my scenario

@Paul-Vandell
Copy link
Contributor

@sngreg I saw that the types are merge to rhf but for this repo nothing has change yet right ? Or am i missing something
@bluebill1049 @kotarella1110 thx for your time :)

@sngreg
Copy link
Author

sngreg commented Jul 17, 2020

@Vandell63 based on looking at open / closed PRa nothing has changed. That being said not sure if bill or kota are working in it?

@kotarella1110
Copy link
Member

@sngreg please create this PR 👍
We will review the PR you created.

@sngreg
Copy link
Author

sngreg commented Jul 17, 2020

@Vandell63 do you have a PR started, if not I can look into making one. (I don’t want to duplicate the efforts)

@bluebill1049
Copy link
Member

looking forward for the PR. thanks 👍

@Paul-Vandell
Copy link
Contributor

@sngreg sorry not yet, do want me to give a try ?

@sngreg
Copy link
Author

sngreg commented Jul 18, 2020

@Vandell63 if you have time today sure. I looked at the code sandbox and I think we are almost there. I am using the latest code sandbox and was working as intended.

@Paul-Vandell
Copy link
Contributor

Paul-Vandell commented Jul 18, 2020

@sngreg @bluebill1049 @kotarella1110 i let you a video instead of hundred of wording.
https://www.loom.com/share/d561dd07ce9948e489d881d2bd62e1c5

Codesandbox link -> https://codesandbox.io/s/yup-resolver-with-errors-array-0js7s?file=/src/Inputs/Object.js
I will start the pull request tomorrow if it sounds good to you.

@sngreg
Copy link
Author

sngreg commented Jul 18, 2020

@Vandell63 thanks for making the video and the code! This looks great to me. Looking forward to the PR!

@Paul-Vandell
Copy link
Contributor

@bluebill1049 @sngreg there we go -> #32

@Paul-Vandell
Copy link
Contributor

@sngreg are you happy with the new release 0.1.0 ? If yes we should maybe close this issue ?

@sngreg
Copy link
Author

sngreg commented Jul 26, 2020

@Vandell63 Yes! Everything works great! Thanks @Vandell63 / @bluebill1049 / @kotarella1110 for the quick work and the release. Closing this issue out.

@codemaster115
Copy link

codemaster115 commented Apr 13, 2021

@sngreg, @Vandell63, @bluebill1049
Seems like this issue is happened again in new V2. Actually I can't get all errors from yup validation.
It just returns the last error of matches, not all match errors.
Any idea about it?

@codemaster115
Copy link

I think it's because of fe53179#diff-1c9dbc45c6880a5a6b5aef346c662825aa881277d91717d88cd8aae0cd35bcc4.
Looking forwards for your update.

@jorisre
Copy link
Member

jorisre commented Apr 13, 2021

Hi @codemaster115

Can you share a codesanbox with your problem please ?

@codemaster115
Copy link

codemaster115 commented Apr 13, 2021 via email

@jorisre
Copy link
Member

jorisre commented Apr 13, 2021

Thanks for the codesandbox @codemaster115
I've opened a new issue, I'll fix it soon

The issue: #155

@codemaster115
Copy link

codemaster115 commented Apr 13, 2021

Thank you, @jorisre
Looking forwards for your update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

7 participants