Skip to content

feat(standard-validator): Add standard schema validation - #887

Merged
yusukebe merged 12 commits into
honojs:mainfrom
muningis:main
Feb 6, 2025
Merged

feat(standard-validator): Add standard schema validation#887
yusukebe merged 12 commits into
honojs:mainfrom
muningis:main

Conversation

@muningis

@muningis muningis commented Dec 13, 2024

Copy link
Copy Markdown
Contributor

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.

@changeset-bot

changeset-bot Bot commented Dec 13, 2024

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 986c34f

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

This PR includes changesets to release 1 package
Name Type
@hono/standard-validator Minor

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 muningis changed the title draft: feat(standard-validator): Add standard schema validation feat(standard-validator): Add standard schema validation Dec 13, 2024
@EdamAme-x

Copy link
Copy Markdown
Contributor

great!!

@yusukebe

Copy link
Copy Markdown
Member

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

Comment thread packages/standard-validator/test/__schemas__/valibot.ts Outdated
Comment thread packages/standard-validator/package.cjs.json Outdated
@muningis

Copy link
Copy Markdown
Contributor Author

Hy! Sure, will do it later today once I'm back from work.

@yusukebe

Copy link
Copy Markdown
Member

Hey @fabian-hiller ! Can you review this?

@fabian-hiller

Copy link
Copy Markdown
Contributor

Yes, I will review it later. Make sure to add Hono to the Standard Schema ecosystem list once this is merged. 😎

@fabian-hiller fabian-hiller left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Once Standard Schema is accepted by the JS community, Hono could try to implement it natively to remove the adapter altogether.

Comment thread .changeset/modern-bugs-search.md Outdated
Comment thread packages/standard-validator/README.md Outdated
Comment thread packages/standard-validator/package.json
Comment thread packages/standard-validator/package.json Outdated
Comment thread packages/standard-validator/package.json Outdated
Comment thread packages/standard-validator/src/index.ts Outdated
Comment thread packages/standard-validator/src/index.ts
Comment thread .github/workflows/ci-standard-validator.yml Outdated
Comment thread packages/standard-validator/package.json

@yusukebe yusukebe left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM!

@yusukebe

yusukebe commented Feb 2, 2025

Copy link
Copy Markdown
Member

Hey @fabian-hiller. This looks good to me. Can you approve this?

Comment thread packages/standard-validator/package.json
Comment thread packages/standard-validator/tsconfig.json
Comment thread packages/node-ws/package.json

@fabian-hiller fabian-hiller left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rest LGTM

>(
target: Target,
schema: Schema,
hook?: Hook<StandardSchemaV1.InferOutput<Schema>, E, P, Target>

@fabian-hiller fabian-hiller Feb 3, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Valibot and Zod has different Hook implementation, this one is based on Zod.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@yusukebe should this be uniformed?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Would probably be good/nice to update Valibot Validator, but that would be ugly breaking change :/

@muningis

muningis commented Feb 6, 2025

Copy link
Copy Markdown
Contributor Author

Anything else we should do, or this is good to go?

@yusukebe

yusukebe commented Feb 6, 2025

Copy link
Copy Markdown
Member

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!

@yusukebe
yusukebe merged commit f77d7ba into honojs:main Feb 6, 2025
@github-actions github-actions Bot mentioned this pull request Feb 6, 2025
@Eghizio

Eghizio commented Feb 9, 2025

Copy link
Copy Markdown

Does it work with OpenAPI like @hono/zod-openapi or is it planned to?

@muningis

Copy link
Copy Markdown
Contributor Author

@Eghizio - @hono/zod-openapi depeneds on @asteasolutions/zod-to-openapi.

From my understanding, @asteasolutions/zod-to-openapi relies on internals of zod (which differs from Valibot, Arktype or Effect Schema). What they have in common, is output and typings of that. Since this supports zod, it should be fine with @hono/zod-openapi when using just zod.

@Eghizio

Eghizio commented Feb 11, 2025

Copy link
Copy Markdown

Thanks @muningis 🙇
It would be amazing to have all of the standard schema compliant validators allow for generating Open API spec & Swagger out of the box in Hono ✨

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.

5 participants