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

Add options for datetime into zod plugin #1881

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

harkor
Copy link

@harkor harkor commented Mar 28, 2025

Actually, we only have something like this

export const zOrder = z.object({
  complete: z.boolean().optional(),
  id: z.coerce.bigint().optional(),
  petId: z.coerce.bigint().optional(),
  quantity: z.number().int().optional(),
  shipDate: z
    .string()
    .datetime()
    .optional(),
  status: z.enum(['placed', 'approved', 'delivered']).optional(),
});

Tha goal is providing datetime options given by zod documentation
https://zod.dev/?id=datetimes

With this kind of plugin declaration inside openapi-ts.config.ts

{
      dateTimeOptions: {
        local: true,
        offset: true,
        precision: 3,
      },
      name: 'zod',
    },

we can have something like this

export const zOrder = z.object({
  complete: z.boolean().optional(),
  id: z.coerce.bigint().optional(),
  petId: z.coerce.bigint().optional(),
  quantity: z.number().int().optional(),
  shipDate: z
    .string()
    .datetime({ local: true, offset: true, precision: 3 })
    .optional(),
  status: z.enum(['placed', 'approved', 'delivered']).optional(),
});

I'm not confortables about hey-api plugins so, feel free to make corrections.

Copy link

stackblitz bot commented Mar 28, 2025

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

Copy link

vercel bot commented Mar 28, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hey-api-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 29, 2025 7:28am

Copy link
Member

@mrlubos mrlubos left a comment

Choose a reason for hiding this comment

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

Hey! You'll want to pass plugin around instead of creating your own config variable. It should contain all you need.

Copy link
Member

@mrlubos mrlubos left a comment

Choose a reason for hiding this comment

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

I'm going to run the tests to check if CI passes, but we'll also need snapshot tests covering this functionality

@@ -463,6 +471,11 @@ const stringTypeToZodSchema = ({
expression: stringExpression,
name: compiler.identifier({ text: 'datetime' }),
}),
parameters: [
plugin.dateTimeOptions
? JSON.stringify(plugin.dateTimeOptions)
Copy link
Member

Choose a reason for hiding this comment

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

I'm actually surprised this works 😂 it wouldn't work if you passed the object instead of string? I'd feel more comfortable with doing that, although being even more explicit and constructing the object would be ideal. Right now I'd need to go back to config to check what it contains. I worry it will break if the config shape changes. I don't want to introduce this as a pattern for other plugins, what do you think?

Copy link
Author

Choose a reason for hiding this comment

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

I tried other things but the only thing is working is to pass a stringfiied object to have the correct output.

There is another way ?

Copy link
Member

Choose a reason for hiding this comment

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

I'd like to see that parameter object being constructed in the plugin, even if it's assigning the same config values to an object. Have a look at how we do parameters elsewhere we call this function. It will eliminate the need to cross reference another file

@mrlubos
Copy link
Member

mrlubos commented Mar 29, 2025

Also are there any other options like this we could introduce later? As in for booleans or other types?

Copy link

codecov bot commented Mar 29, 2025

Codecov Report

Attention: Patch coverage is 0% with 30 lines in your changes missing coverage. Please review.

Project coverage is 57.17%. Comparing base (93dad69) to head (1b89885).

Files with missing lines Patch % Lines
packages/openapi-ts/src/plugins/zod/plugin.ts 0.00% 30 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1881      +/-   ##
==========================================
- Coverage   57.23%   57.17%   -0.07%     
==========================================
  Files         191      191              
  Lines       25954    25984      +30     
  Branches     1964     1964              
==========================================
  Hits        14856    14856              
- Misses      11089    11119      +30     
  Partials        9        9              
Flag Coverage Δ
unittests 57.17% <0.00%> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link

pkg-pr-new bot commented Mar 29, 2025

Open in Stackblitz

@hey-api/client-axios

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/client-axios@1881

@hey-api/client-fetch

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/client-fetch@1881

@hey-api/client-next

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/client-next@1881

@hey-api/client-nuxt

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/client-nuxt@1881

@hey-api/nuxt

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/nuxt@1881

@hey-api/openapi-ts

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/openapi-ts@1881

@hey-api/vite-plugin

npm i https://pkg.pr.new/hey-api/openapi-ts/@hey-api/vite-plugin@1881

commit: 1b89885

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