-
-
Notifications
You must be signed in to change notification settings - Fork 204
V2 - TypeScript/Tests improvements + Vest fix #110
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
Conversation
98633da to
2721382
Compare
3d3d46d to
e4c750a
Compare
|
Size Change: +88 B (+1%) Total Size: 14.3 kB
ℹ️ View Unchanged
|
|
@bluebill1049 PR ready for review ;) |
| birthYear: 'birthYear', | ||
| }; | ||
|
|
||
| const result = await joiResolver(schema)(data, undefined, 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.
just a quick note, going to change this true to an object.
{
isValidateAllFieldCriteria: boolean,
field: {
name: string,
},
// potential more
}
maybe call it formContext, not sure if that's going to be too confusing, but we figure out a good name later.
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.
Ok, sound good to me. I'll change it in another PR.
Does field contains ref too ?
Do you think we have to add an option to allow users to validate only the current instead all field ? (except on submit event)
Or users have to create a custom resolver ?
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.
Does field contains ref too ?
Yes.
Do you think we have to add an option to allow users to validate only the current instead all field ? (except on submit event)
Yes, That would be awesome. I am not sure how easy it is going to be for all resolvers.
|
|
||
| const result = await joiResolver(schema)(data); | ||
|
|
||
| expect(result).toMatchSnapshot(); |
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.
Minor: (don't have to change as well) maybe be specific with the error? I normally use snapshots for a successful responses.
bluebill1049
left a comment
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.
I love those:
- Tests
- Type improvement
- Structure improvement
Awesome stuff!! 🎖
| } from 'react-hook-form'; | ||
| import type { AsyncValidationOptions, Schema } from 'joi'; | ||
|
|
||
| export type Resolver = <T extends Schema>( |
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.
here is the type in the react-hook-form repo:
I think maybe it's better to leave that at react-hook-form and centralize there?
export type Resolver<
TFieldValues extends FieldValues = FieldValues,
TContext extends object = object
> = (
values: UnpackNestedValue<TFieldValues>,
context?: TContext,
validateAllFieldCriteria?: boolean,
) => Promise<ResolverResult<TFieldValues>> | ResolverResult<TFieldValues>;
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.
It's not the same type
From react-hook-form:
export type Resolver<
TFieldValues extends FieldValues = FieldValues,
TContext extends object = object
> = (
values: UnpackNestedValue<TFieldValues>,
context?: TContext,
validateAllFieldCriteria?: boolean,
) => Promise<ResolverResult<TFieldValues>> | ResolverResult<TFieldValues>;And from resolvers:
export type Resolver = <T extends Schema>(
schema: T,
options?: AsyncValidationOptions,
) => <TFieldValues extends FieldValues, TContext>(
values: UnpackNestedValue<TFieldValues>,
context?: TContext,
validateAllFieldCriteria?: boolean,
) => Promise<ResolverResult<TFieldValues>>;The generic type isn't placed at the same point. This update improve resolver's return type. But I'm open for a better idea 😉
I tried to centralized but it breaks severals things: react-hook-form/react-hook-form#3821
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.
all good, I realize that after reviewing the rest of the resolvers.
| } from 'react-hook-form'; | ||
| import { validate, Struct } from 'superstruct'; | ||
|
|
||
| type Options = Parameters<typeof validate>[2]; |
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.
oh, I see why you need to leave those type here, that's fine as well. probably need to include that in the doc, so user don't get confused where the type is, perhaps we should remove that type from the core repo.
| }; | ||
|
|
||
| export const vestResolver = <TFieldValues extends FieldValues>( | ||
| schema: ICreateResult, |
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.
oops
| _: any = {}, | ||
| export const vestResolver: Resolver = (schema, _ = {}) => async ( | ||
| values, | ||
| _context, |
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 PR is included in version 2.0.0-beta.4 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Last step before starting new features :)
validateAllFieldCriteriabug in Vest resolveruseForm— related to Type problems after update resolver to 1.3.0 #97Resolvertypes ⬇️Before

After
