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

feat: implement requiredIf rules #42

Merged
merged 4 commits into from
Mar 13, 2024
Merged

Conversation

thetutlage
Copy link
Contributor

The requiredIf rules offer an alternate API to the vine.union which is less type-safe but also simpler to write and express conditions.

Here's a quick example between the vine.union and the requiredIf rules.

With vine.union

const emailOrPhone = vine.group([
  vine.group.if((data) => 'email' in data, {
    email: vine.string().email()
  }),
  vine.group.if((data) => 'phone' in data, {
    phone: vine.string().mobile()
  }),
])

const loginForm = vine.object({
  password: vine
    .string()
    .minLength(6)
    .maxLength(32)
})
.merge(emailOrPhone)

The output type will be a union as well.

type LoginForm = {
  password: string
} & ({
  email: string
} | {
  phone: string
})

With requiredIfMissing

const loginForm = vine.object({
  email: vine.string().email().optional().requiredIfMissing('phone'),
  phone: vine.string().mobile().optional().requiredIfMissing('email'),
  password: vine
    .string()
    .minLength(6)
    .maxLength(32)
})

The output will have both email and phone as optional even though one of them will be defined at runtime.

type LoginForm = {
  email: string | undefined,
  phone: string | undefined,
  password: string
})

The requiredIfMissing rules are helpful when you do not care much about the output data-types and you do not want to narrow them. For example, validate data and store it as a JSON blob in the database

@Julien-R44
Copy link
Member

requiredIfExistsAny and requiredIfMissingAny sound weird to me. wouldn't requiredIfAnyExists and requiredIfAnyMissing be more appropriate?

maybe it's my English that's lacking here, so tell me if i am wrong

otherwise, looks perfect. cool and simpler alternative to unions 👌

@thetutlage
Copy link
Contributor Author

requiredIfExistsAny and requiredIfMissingAny sound weird to me. wouldn't requiredIfAnyExists and requiredIfAnyMissing be more appropriate?

Yeah, your variation seems more natural to speak. Let's see if @RomainLanz thinks the same and then I can update the method names.

@RomainLanz
Copy link
Member

I agree with @Julien-R44, requiredIfAnyExists and requiredIfAnyMissing sounds more natural. 👍🏻

* the validation options, we can safely copy them by reference.
*/
protected cloneValidations(): Validation<any>[] {
return this.validations.map((validation) => {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it would be better to directly use structuredClone()?

return structuredClone(this.validations)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh did not know about that. Will do in a separate PR

@thetutlage thetutlage merged commit 893d378 into develop Mar 13, 2024
12 checks passed
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.

None yet

3 participants