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

fix(zod-openapi): Fix basePath method to transfer defaultHook of the parent app. #408

Merged
merged 3 commits into from
Mar 4, 2024

Conversation

DavidHavl
Copy link
Contributor

There is currently an issue with basePath method where it does not transfer defaultHook to the newly created instance.
This pull request fixes it.

In the following code (taken from the current docs) the defaultHook is never called because it does not exist on the new instance created when basePath method is called.

const app = new OpenAPIHono({
  defaultHook: (result, c) => {
    if (!result.success) {
      return c.json(
        {
          ok: false,
          errors: formatZodErrors(result),
          source: 'custom_error_handler',
        },
        422
      )
    }
  },
}).basePath('/api/v1')

Copy link

changeset-bot bot commented Mar 3, 2024

🦋 Changeset detected

Latest commit: 828e136

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

This PR includes changesets to release 1 package
Name Type
@hono/zod-openapi Patch

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 yusukebe changed the title Fix basePath method to transfer defaultHook of the parent app. fix(zod-openai): Fix basePath method to transfer defaultHook of the parent app. Mar 4, 2024
@@ -1270,7 +1279,9 @@ describe('Named params in nested routes', () => {
it('Should return a correct path', async () => {
const res = await root.request('/doc')
expect(res.status).toBe(200)
const data = await res.json()
const data: {
Copy link
Member

Choose a reason for hiding this comment

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

I think this change is not necessary. What is it for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, this is not necessary per se. That code was showing typescript error so I just fixed it so I could run the whole test suite.

Copy link
Member

Choose a reason for hiding this comment

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

We can ignore the typescript error, so please remove it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Removed.

@yusukebe
Copy link
Member

yusukebe commented Mar 4, 2024

Hi @DavidHavl

Thank you for the PR. I've left the comment. And could you run yarn changeset? This will be a patch change.

@DavidHavl DavidHavl changed the title fix(zod-openai): Fix basePath method to transfer defaultHook of the parent app. fix(zod-openapi): Fix basePath method to transfer defaultHook of the parent app. Mar 4, 2024
@yusukebe
Copy link
Member

yusukebe commented Mar 4, 2024

@DavidHavl

Thanks! Last one. thing. Run yarn changeset on the top of the project, add it, and push!

@DavidHavl
Copy link
Contributor Author

Done

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

yusukebe commented Mar 4, 2024

@DavidHavl

Merge now. Thanks!

@yusukebe yusukebe merged commit d4ca1ce into honojs:main Mar 4, 2024
1 check passed
@github-actions github-actions bot mentioned this pull request Mar 4, 2024
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.

2 participants