feat(standard-validator): Add standard schema validation #887
Conversation
🦋 Changeset detectedLatest commit: 986c34f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
muningis
changed the title
|
great!! |
|
Hi @muningis This is awesome! The code below actually works! import { Hono } from 'hono'
import { sValidator } from '@hono/standard-validator'
import { type } from 'arktype'
import * as v from 'valibot'
import { z } from 'zod'
const aSchema = type({
agent: 'string',
})
const vSchema = v.object({
slag: v.string(),
})
const zSchema = z.object({
name: z.string(),
})
const app = new Hono()
app.get(
'/:slag',
sValidator('header', aSchema),
sValidator('param', vSchema),
sValidator('query', zSchema),
(c) => {
const headerValue = c.req.valid('header')
const paramValue = c.req.valid('param')
const queryValue = c.req.valid('query')
return c.json({ headerValue, paramValue, queryValue })
}
)
const res = await app.request('/foo?name=foo', {
headers: {
agent: 'foo',
},
})
console.log(await res.json())
/**
{
headerValue: {
agent: "foo",
},
paramValue: {
slag: "foo",
},
queryValue: {
name: "foo",
},
}
*/I'll review this. First, can you add the CI like this: https://github.com/honojs/middleware/blob/main/.github/workflows/ci-hello.yml |
|
Hy! Sure, will do it later today once I'm back from work. |
|
Hey @fabian-hiller ! Can you review this? |
|
Yes, I will review it later. Make sure to add Hono to the Standard Schema ecosystem list once this is merged. 😎 |
fabian-hiller
left a comment
fabian-hiller
left a comment
There was a problem hiding this comment.
Once Standard Schema is accepted by the JS community, Hono could try to implement it natively to remove the adapter altogether.
|
Hey @fabian-hiller. This looks good to me. Can you approve this? |
| >( | ||
| target: Target, | ||
| schema: Schema, | ||
| hook?: Hook<StandardSchemaV1.InferOutput<Schema>, E, P, Target> |
There was a problem hiding this comment.
Should this be Hook<Schema, E, P, Target> instead? At least the Valibot validator middleware implementation is using the schema object and not its output. But maybe this is a bug in the Valibot validator middleware.
There was a problem hiding this comment.
Valibot and Zod has different Hook implementation, this one is based on Zod.
There was a problem hiding this comment.
@yusukebe should this be uniformed?
It's better to unify the implementation. Following Zod Validator is good. The current implementation in this PR is okay, but we should change the implementation of the Valibot Validator.
| } | ||
|
|
||
| if (result.issues) { | ||
| return c.json({ data: value, error: result.issues, success: false }, 400) |
There was a problem hiding this comment.
This is probably not a problem in this implementation, but the Valibot validator middleware uses output instead of data for the output value. Could it be that the Valibot implementation should be updated?
There was a problem hiding this comment.
Hm. Does valibot transform values if it fails to validate? Personally, I think, ideally, we should return whatever was passed, before transformations and defaults, in case of failed validation.
There was a problem hiding this comment.
Yes, Valibot always provides an output that can contain transformations when using safeParse or safeParseAsync. I have no opinion on the implementation. I just noticed it.
There was a problem hiding this comment.
Working with original data IMO has slightly more better handling if there’s something you need to do what’s not available/possible with schema and/or want to use it for observability. Would be nice to hear what @yusukebe has to say on this.
There was a problem hiding this comment.
It's okay with @muningis's implementation for this Standard Validator. It's helpful to see the original data. Regarding the Validabot Validator, I think both updating and not are okay.
There was a problem hiding this comment.
Would probably be good/nice to update Valibot Validator, but that would be ugly breaking change :/
|
Anything else we should do, or this is good to go? |
|
This is good! Let's land it. @muningis Thank you for the great work. Standardizing the schema libs for the validator will be a big thing for Hono and the TS/JS ecosystem! @fabian-hiller Thank you for the review! |
|
Does it work with OpenAPI like @hono/zod-openapi or is it planned to? |
|
@Eghizio - From my understanding, |
|
Thanks @muningis 🙇 |
5 participants
Group of different Schema Validation libraries authors has agreed on a standard schema, so other libraries, don't have even to care what validation library you're using, as typings and output must match spec.