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

Feature/nope resolver #18

Closed
wants to merge 4 commits into from

Conversation

kysley
Copy link

@kysley kysley commented Jul 7, 2020

Hey @bluebill1049, big fan ;)

I made this to use on a new project of mine, and figured I should put up a pr.

Let me know what you guys think!

@bluebill1049
Copy link
Member

oh wow! nice! thank you very much for this.

@JeromeDeLeon
Copy link

This is looking great! However, I believe that this could also be put up on a website. The README is starting to be bloated with examples. I think it makes sense to include a list of examples and link them to it.

@JeromeDeLeon
Copy link

JeromeDeLeon commented Jul 7, 2020

Haven't seen any other libraries aside from the first three on the README. This kinda a great to experiment. Thank you. Really appreciated it.

id: 'test',
},
};
expect(await nopeResolver(Article)(data)).toMatchSnapshot();
Copy link

Choose a reason for hiding this comment

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

I think you should check error value. Because I think snapshot test should be used only when comparing value is difficult.

return {
...acc,
...(fieldName
? acc[fieldName] && validateAllFieldCriteria
Copy link

Choose a reason for hiding this comment

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

I think you should test here too. (the case validateAllFieldCriteria is false)

const parseNopeSchema = (error: any, validateAllFieldCriteria: boolean) => {
const errors = Object.keys(error);

return Array.isArray(errors)
Copy link

Choose a reason for hiding this comment

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

I think errors is always Array type. Is it wrong?

Copy link
Author

Choose a reason for hiding this comment

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

Nope! This is the signature:

export interface ShapeErrors {
    [key: string]: string;
}

@keiya01
Copy link

keiya01 commented Jul 8, 2020

@kysley
Thank you very much for sending the PR🙏
I reviewed your code.

import { appendErrors, transformToNestObject, Resolver } from 'react-hook-form';
import NopeObject from 'nope-validator/lib/umd/NopeObject';

const parseNopeSchema = (error: any, validateAllFieldCriteria: boolean) => {
Copy link
Member

Choose a reason for hiding this comment

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

Is this possible to avoid any?

import { ShapeErrors } from 'nope-validator/lib/umd/types';

const parseNopeSchema = (error: ShapeErrors, validateAllFieldCriteria: boolean) => {

Copy link
Author

Choose a reason for hiding this comment

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

It might be more complicated than casting it to ShapeErrors, please see my comment below!

@kysley
Copy link
Author

kysley commented Jul 8, 2020

Hey guys, I'm new to using Typescript in more complex situations, and would love a little guidance here.

Any NopePrimitive can be validated, changing the return signature.

Nope.string() //String, Number, Date, Boolean
  .test(a => (a === '42' ? undefined : 'Must be 42'))
  .validate('41'); 

// string => 'Must be 42'

If NopeObject has shape({...}), then ShapeErrors is returned.

const schema = Nope.object().shape({
  name: Nope.string()
    .atMost(15)
    .required(),
  email: Nope.string()
    .email('Please provide a valid email')
    .required(),
});

const errors = schema.validate({
  name: 'Test',
  email: 'invalidemail',
});

// ShapeErrors => { email: 'Please provide a valid email', }

I think to keep consistent, we should require a NopeObject WITH the shape signature. But I am unsure how to write a type signature for this scenario.

@bluebill1049
Copy link
Member

shall we get this boy in and sort out the type afterword? ship it first :)

@kysley
Copy link
Author

kysley commented Jul 14, 2020

shall we get this boy in and sort out the type afterword? ship it first :)

There seems to be an issue with nope. It doesn't validate nested objects within a schema. I think we should wait? Since RHF users may have nested form fields.

If you don't think that is critical, I'm more than happy to get this sorted out and ship it :)

ftonato/nope-validator#288

@bluebill1049
Copy link
Member

Thanks @kysley for the update, let's wait for nope validator author to response on the above :) 🙏

import React from 'react';
import { useForm } from 'react-hook-form';
import { nopeResolver } from '@hookform/resolvers';
import Nope from 'nope-resolver';

Choose a reason for hiding this comment

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

Should this import nope-validator instead?

@firebluetom2
Copy link

I would love to see this library supported, would want to replace my yup with something smaller

@bluebill1049
Copy link
Member

I would love to see this library supported, would want to replace my yup with something smaller

use Zod instead.

@jorisre jorisre mentioned this pull request Apr 16, 2021
@jorisre jorisre closed this Apr 17, 2021
@jorisre
Copy link
Member

jorisre commented Apr 17, 2021

I close this one because the nope resolver is released

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.

8 participants