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: add ajv #408

Merged
merged 10 commits into from Jun 3, 2022
Merged

feat: add ajv #408

merged 10 commits into from Jun 3, 2022

Conversation

jsun969
Copy link
Contributor

@jsun969 jsun969 commented May 31, 2022

There are 2 problems need to be solved

  1. mjs file cannot be created when build. But I have add the build script.
  2. If field is required, user must add minLength: 1 in the schema. Does this need to be optimized?

close #309

@getTobiasNielsen
Copy link

If field is required, user must add minLength: 1 in the schema. Does this need to be optimized?

I'm fairly sure that is how ajv want it to work.

@bluebill1049
Copy link
Member

This is awesome! @jorisre would be able to help on the first issue since you are an expert on microbundle 😄

ajv/src/ajv.ts Outdated Show resolved Hide resolved
@jsun969
Copy link
Contributor Author

jsun969 commented Jun 1, 2022

If field is required, user must add minLength: 1 in the schema. Does this need to be optimized?

I'm fairly sure that is how ajv want it to work.

🤔 I think we can ignore the second issue. Just add some tips in documentation. What do you think? @bluebill1049

@bluebill1049
Copy link
Member

🤔 I think we can ignore the second issue. Just add some tips in documentation. What do you think? @bluebill1049

thanks, @jsun969 I wouldn't add any "magic" to make it work, and try to stay as native as possible with AJV, at the end of the day people who decided to choose and use AJV resolver should have a clear understanding of its mechanism. By the way, this feature is awesome! so many users have asked for it and it's finally coming into pieces.

@damien-duignan I may borrow your eyes here as well.

cc @Moshyfawn

@jsun969
Copy link
Contributor Author

jsun969 commented Jun 1, 2022

🤔 I think we can ignore the second issue. Just add some tips in documentation. What do you think? @bluebill1049

thanks, @jsun969 I wouldn't add any "magic" to make it work, and try to stay as native as possible with AJV, at the end of the day people who decided to choose and use AJV resolver should have a clear understanding of its mechanism. By the way, this feature is awesome! so many users have asked for it and it's finally coming into pieces.

@damien-duignan I may borrow your eyes here as well.

cc @Moshyfawn

I get it! I'll make Ajv native. btw, I can add some ajv tips to documentation after merge if necessary

@bluebill1049
Copy link
Member

thanks @jsun969 awesome to hear!

@bluebill1049 bluebill1049 requested a review from jorisre June 1, 2022 08:13
@jsun969
Copy link
Contributor Author

jsun969 commented Jun 1, 2022

I've fixed the first issue (build mjs error). Could you run the ci again 😄

@getTobiasNielsen
Copy link

getTobiasNielsen commented Jun 1, 2022

Just to let you know how brilliant the kid is, I pointed out to @jsun969 that none of the other builds produced mjs, and he figured out something had to make the mjs in the code.

const copySrc = () => {
// Copy .module.js --> .mjs for Node 13 compat.
fs.writeFileSync(
`${process.cwd()}/dist/resolvers.mjs`,
fs.readFileSync(`${process.cwd()}/dist/resolvers.module.js`),
);
};

Doesn't this solve the problem? (from microbundles docs)

image

I can see how having mjs, could make it more clear what type is it, and I guess you get more play with the node version.

@bluebill1049
Copy link
Member

Awesome stuff, I will review it soon and get this to release soon! existing stuff.

@bluebill1049 bluebill1049 self-requested a review June 1, 2022 22:45
Copy link
Member

@bluebill1049 bluebill1049 left a comment

Choose a reason for hiding this comment

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

Looks amazing to me 🚀 Let's wait for @jorisre to give a review as well.

@bluebill1049
Copy link
Member

hey @jorisre you are probably busy, I may merge and publish this feature this week. 🙏

@jorisre jorisre merged commit 0cad904 into react-hook-form:master Jun 3, 2022
@jorisre
Copy link
Member

jorisre commented Jun 3, 2022

Thanks a lot @jsun969 ! 🚀 👏🏻

@github-actions
Copy link
Contributor

github-actions bot commented Jun 3, 2022

🎉 This PR is included in version 2.9.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

AJV resolver
4 participants