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: Conform Validator Middleware #666

Merged
merged 18 commits into from
Jul 31, 2024
Merged

Conversation

uttk
Copy link
Contributor

@uttk uttk commented Jul 23, 2024

Added a validator that allows validation using conform.
Although the processing content is highly abstract and does not do much, it can increase the affinity with Hono RPC.

Copy link

changeset-bot bot commented Jul 23, 2024

🦋 Changeset detected

Latest commit: 8775f57

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@hono/conform-validator Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@yusukebe
Copy link
Member

Hi @uttk !

Cool! I've tried it the feeling of usage is good. Also, It's great the implementation is simple. Before I review the details, I would like to know @chimame 's thoughts.

@chimame
Copy link
Contributor

chimame commented Jul 24, 2024

Before I write my thoughts, let me first say thank you.
This middleware will be very important for Hono to use Form with JSX. I am grateful for creating such a great middleware. 👍

If I were to create this middleware, I would create it with the same interface. I'm glad we're on the same page.

import { conformValidator } from '@hono/conform-validator'

conformValidator((formData) => parseWithZod(formData, { schema })),
// parseWithZod or parseWithYup or parseWithValibot

This allows middleware users to choose which Validator to use to validate form data, and makes it possible to validate form data with minimal code.

The process of returning errors is defined by the user each time, but how about a method to customize it by allowing an optional hook function to be passed as the second argument of the conformValidator function, just like other validators? If there is no hook function, a process to return the default error content defined by the middleware is necessary.

if (hook) {
const hookResult = hook(result, c)
if (hookResult instanceof Response || hookResult instanceof Promise) {
return hookResult
}
}
if (!result.success) {
return c.json(result, 400)
}
const data = result.output as InferOutput<T>
return data

Also, although it may be limited to React, I think it would be a good idea to return the Conform SubmissionResult type, that is, submission.reply(), to the client to pass server-side errors to the client's form data.

const submission = c.req.valid('form')

if (submission.status !== 'success') {
  const res = c.json({ success: false, result: submission.reply() }, 400)
  throw HTTPException(400, { res })
}

@uttk
Copy link
Contributor Author

uttk commented Jul 24, 2024

@yusukebe @chimame

Thank you for reviews !

The process of returning errors is defined by the user each time, but how about a method to customize it by allowing an optional hook function to be passed as the second argument of the conformValidator function, just like other validators?

The optional Hook function is a great idea !
I would definitely like to implement it !

If there is no hook function, a process to return the default error content defined by the middleware is necessary.

As for the behavior when the hook function is omitted, I think it would be best to perform only validation, as with the current process, to prevent unintentional transmission of confidential information to the outside.

Also, I think error handling should be done carefully and should be implemented by the implementer himself, rather than leaving it to another library.

@uttk
Copy link
Contributor Author

uttk commented Jul 25, 2024

Hi @yusukebe .

I added hook option to conformValidator(). There is room for discussion regarding error handling and what happens when the hook is omitted, but I think the basic processing has been implemented.

cc: @chimame

@chimame
Copy link
Contributor

chimame commented Jul 25, 2024

Thank you for the great implementation.

I think that when there is a validation error in Conform, the default behavior should be return submission.reply(), as stated in the Conform documentation. If users of the middleware want to change this behavior, I think it would be a good idea to use the Hook function you implemented.

https://conform.guide/#the-gist

I think it would be best to perform only validation, as with the current process, to prevent unintentional transmission of confidential information to the outside.

I was thinking about what kind of assumptions you made in your opinion. I thought that this might be the case depending on the validation schema implementation of middleware users, but if that's the case, I think the same thing could happen with existing validation middleware.

I would like to respect your opinion, so I would like you to tell me in what kind of situations this could occur. I think it will also be useful for yusukebe's decision-making.

@uttk
Copy link
Contributor Author

uttk commented Jul 25, 2024

@chimame

I was thinking about what kind of assumptions you made in your opinion. I thought that this might be the case depending on the validation schema implementation of middleware users, but if that's the case, I think the same thing could happen with existing validation middleware.

That's true.

On second thought, I realized that since the other validator middleware already returns error responses by default, it would be more natural for this middleware to match the behavior.

I would like to modify the default behavior so that if a validation error occurs, the default behavior is to return an error response.

Thanks.

@uttk
Copy link
Contributor Author

uttk commented Jul 25, 2024

Fixed it.

Copy link
Contributor

@chimame chimame left a comment

Choose a reason for hiding this comment

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

Thank you for thinking about the API design together with me.

I think there's no problem with this implementation of the API for this middleware. Since it handles FormData, I wonder if we can make good use of hono/validator's FormData processing.

@yusukebe
I think there's no problem with this as a middleware design, so would you mind reviewing it in detail?

@yusukebe
Copy link
Member

@uttk Great! I'll do it.

packages/conform-validator/test/valibot.test.ts Outdated Show resolved Hide resolved
packages/conform-validator/package.json Outdated Show resolved Hide resolved
.github/workflows/ci-conform-validator.yml Outdated Show resolved Hide resolved
@yusukebe
Copy link
Member

Hey @uttk

I've left some comments. Please check them!

@chimame Thank you for your reviews!

@uttk
Copy link
Contributor Author

uttk commented Jul 27, 2024

@yusukebe

Thank you for reviews!
I guess I was able to fix the issues you pointed out.

packages/conform-validator/package.json Outdated Show resolved Hide resolved
.changeset/pretty-eels-prove.md Outdated Show resolved Hide resolved
@yusukebe
Copy link
Member

Hi @uttk

Thank you for some fixes. I've left two comments. There will be last. Check them!

@uttk
Copy link
Contributor Author

uttk commented Jul 28, 2024

@yusukebe

Thank you for your kind review.
I have corrected it.

@uttk uttk requested a review from yusukebe July 30, 2024 14:42
Copy link
Member

@yusukebe yusukebe left a comment

Choose a reason for hiding this comment

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

LGTM!

@yusukebe
Copy link
Member

@uttk @chimame Thank you!

Let's gooo! I'll merge and release it.

@yusukebe yusukebe merged commit d4a6913 into honojs:main Jul 31, 2024
1 check passed
@github-actions github-actions bot mentioned this pull request Jul 31, 2024
@uttk uttk deleted the feat-conform-middleware branch July 31, 2024 14:57
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.

3 participants