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

chore: Update valibot to version 0.31.0 #553

Merged
merged 4 commits into from
Jun 9, 2024

Conversation

chimame
Copy link
Contributor

@chimame chimame commented May 30, 2024

Overview

Update valibot to version 0.31.0.
0.31.0 will lose backward compatibility due to many breaking changes.

0.31.0 has not yet been officially released. Therefore, it is a draft PR.

Details

Changes have been made to correspond to the changes in 0.31.0.

From 0.31.0, the pipe function, which was previously built-in, is now defined as an independent function. This means that each API was able to perform consecutive validations on the second and third arguments, but from 0.31.0 validations will be performed using an independent pipe function.

fabian-hiller/valibot#502

// 0.30.0 or lower
number("required", [minValue(1, "min"), maxValue(10, "max")])
// 0.31.0 or higher
pipe(number("required"), minValue(1, "min"), maxValue(10, "max"))

Also, from 0.31.0 onwards there will be a change in the type definition name.

https://valibot.dev/guides/migrate-from-v0.30.0/

Copy link

changeset-bot bot commented May 30, 2024

🦋 Changeset detected

Latest commit: 877538b

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

This PR includes changesets to release 1 package
Name Type
@hono/valibot-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

@yusukebe
Copy link
Member

Hi @chimame

0.31.0 has not yet been officially released. Therefore, it is a draft PR.

Okay! Could you ping me if the PR is ready? Thanks!

@chimame
Copy link
Contributor Author

chimame commented Jun 1, 2024

valibot will officially release 0.31.0 this week or next. I'll let you know at that time!

@fabian-hiller
Copy link
Contributor

Thank you @chimame! Your changes look good to me.

@@ -56,6 +56,6 @@ export const vValidator = <
return c.json(result, 400)
Copy link
Contributor

@fabian-hiller fabian-hiller Jun 2, 2024

Choose a reason for hiding this comment

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

@yusukebe would it be better for the developer experience to return result.issues instead of result?

Copy link
Member

Choose a reason for hiding this comment

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

Hi @fabian-hiller

I think result is not bad. And if they do not prefer it, they can change it themselves with the hook.

@chimame Do you have any thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understand that the intention of fabian-hiller's comment is as follows.

If validation fails, wouldn't it be fine to set the HTTP status to 400 and return only the error details in JSON? I think that is what he is trying to suggest.

If validation is successful, you will get result.output, in other words, the JSON that passed validation. However, if validation fails, the entire result JSON will be returned to the client. The type of result is as follows. The type has been simplified for ease of understanding.

type SafeParseResult = 
  | {
      readonly typed: true;
      readonly success: true;
      readonly output: InferOutput<TSchema>; // <-- Contains objects that have been successfully validated.
      readonly issues: undefined; // <-- Since the validation was successful, there are no issues.
    }
  | {
      readonly typed: false;
      readonly success: false;
      readonly output: unknown; // <-- Contains the original data that failed validation.
      readonly issues: [InferIssue<TSchema>, ...InferIssue<TSchema>[]]; // <-- This contains the details of the errors for each item that failed validation.
    };

const result: SafeParseResult = await safeParseAsync(schema, value);

/*
example error result
{
  typed: false,
  success: false,
  output: { name: 'Superman', age: '20' },
  issues: [
    {
      kind: 'schema',
      type: 'number',
      input: '20',
      expected: 'number',
      received: '"20"',
      message: 'Invalid type: Expected number but received "20"',
      requirement: undefined,
      path: [Array],
      issues: undefined,
      lang: undefined,
      abortEarly: undefined,
      abortPipeEarly: undefined,
      skipPipe: undefined
    }
  ]
}
*/

If that is the intention, it is certainly not a bad idea to only return issues.

Suggested change
return c.json(result, 400)
return c.json({ issues: result.issues }, 400)

Copy link
Contributor

Choose a reason for hiding this comment

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

In general result is not bad and can be better because it contains more info, but if this info is never used it is easier for the user to just get the issues.

return c.json(result.issues, 400)

I haven't used Hono with Valibot, so I can't say. It was just a question that came to mind while reviewing the code. I saw that the Zod adapter also returns result. We should keep it consistent.

Copy link
Member

Choose a reason for hiding this comment

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

In general result is not bad and can be better because it contains more info, but if this info is never used it is easier for the user to just get the issues.

Yeah, I can understand both reasons. When I created the Zod Validator, I used result because I thought more information was better. Shall we go with result as it is? Then, we will not have a breaking change.

Thanks for the comments!

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, let's leave it for now. 👍🏼

@chimame chimame marked this pull request as ready for review June 6, 2024 15:12
@chimame
Copy link
Contributor Author

chimame commented Jun 6, 2024

@yusukebe valibot 0.31.0 has been officially released!

@yusukebe
Copy link
Member

yusukebe commented Jun 8, 2024

Congrats!

@chimame Can you do the two things?

  • yarn install to update the lock file.
  • yarn changeset at the top to create a changeset file. If this has a breaking change, it's better to be major. If not, it should be minor.

@chimame
Copy link
Contributor Author

chimame commented Jun 9, 2024

@yusukebe

Sorry, I forgot.
I updated yarn.lock and added CHANGELOG.

@yusukebe
Copy link
Member

yusukebe commented Jun 9, 2024

@chimame Thanks!

Merging now!

@yusukebe yusukebe merged commit 7375c09 into honojs:main Jun 9, 2024
1 check passed
@github-actions github-actions bot mentioned this pull request Jun 9, 2024
@chimame chimame deleted the modify-support-valibot-0-31-0 branch June 9, 2024 04:42
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