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

Give format precendence over type property #831

Closed
wants to merge 5 commits into from

Conversation

helt
Copy link
Contributor

@helt helt commented Feb 24, 2024

Equal to #777 but rebased on current kubb main branch.

Observation

The JSON schema spec suggests that the format keyword has a higher specificity than the type keyword, since format it provides information about the contents of the schema at hand.

Prior this PR, the format property would only be used by kubb iff the type property is not known. However, this prevents the format property to be used in most cases (except for date, and date-time).

For example
a plain file upload

Plain_file:
  format: binary
  title: File
  type: string

leads to a

export type PlainFile = string;

although a Blob oder FormData would be more helpful in many cases.

Goals

This PR enables format to "overrule" type and therefore generalizes the ad hoc approach that was begun date and date-time format parsing.

Open questions:

However, some changes are still open for discussion, since they might involve inclusion of further dependencies (e.g. uuid validation).

  • How to work with format: uuid?
  • Should the format only be used for schema.type === "string", or shall it stay above those?
  • is this a minor or a breaking change? Without activation flag, I would argue for breaking.
  • Should this PR also trickle into downstream packages like zod or swr?

Copy link

changeset-bot bot commented Feb 24, 2024

⚠️ No Changeset found

Latest commit: 9702e76

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link

vercel bot commented Feb 24, 2024

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

Name Status Preview Comments Updated (UTC)
kubb ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 24, 2024 10:55pm

Copy link

codesandbox-ci bot commented Feb 24, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 9702e76:

Sandbox Source
advanced-pet-store Configuration
faker-pet-store Configuration
msw-pet-store Configuration
msw-v2-pet-store Configuration
react-query-pet-store Configuration
react-query-v5-pet-store Configuration
typescript-pet-store Configuration
simple-single-pet-store Configuration
solid-query-pet-store Configuration
svelte-query-pet-store Configuration
swr-pet-store Configuration
vue-query-pet-store Configuration
vue-query-v5-pet-store Configuration
zod-pet-store Configuration
zodios-pet-store Configuration
client-pet-store Configuration

@helt helt reopened this Feb 24, 2024
@helt helt changed the title Wrote tests and documented expected outcome Give format precendence over type property Feb 24, 2024
Copy link
Contributor Author

@helt helt left a comment

Choose a reason for hiding this comment

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

// 7.3.1. Dates, Times, and Duration
case 'date-time':
case 'date':
case 'time':
Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually, im not sure if time should really be cast a Date. Maybe number instead?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We have dateType which can now be a date or a string so maybe we can also add number if that would be needed for other users.

return factory.createTypeReferenceNode(factory.createIdentifier('Date'))
}
case 'uuid':
// TODO how to work with this? use https://www.npmjs.com/package/uuid for 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.

Currently, uuid would be ignored like all the others, although it would made sense to make it an uuid, but this would require a custom task or a dependency uuid

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the end, it will be a string, Typescript does not have a type uuid so I don't think we need to change something here for the ts plugin.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing is, the cases added here are directly from the openapi spec. That's why I added them here, too.
But I'll remove unsupported cases if you like.

Imo, being able to map to a specific type is very convinient. But kubb itself should add as little runtime dependencies as possible. Hence, uuid support via @types/uuid and uuid would be a great use case for a kubb plugin? Is that already possible with plugins, then I might give them a try.

Copy link

codecov bot commented Feb 24, 2024

Codecov Report

Attention: Patch coverage is 98.24561% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 76.10%. Comparing base (748f394) to head (9702e76).

Files Patch % Lines
packages/swagger-ts/src/TypeGenerator.ts 98.24% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #831      +/-   ##
==========================================
+ Coverage   75.97%   76.10%   +0.13%     
==========================================
  Files         135      135              
  Lines       12690    12739      +49     
  Branches     1392     1412      +20     
==========================================
+ Hits         9641     9695      +54     
+ Misses       3038     3033       -5     
  Partials       11       11              

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

@stijnvanhulle
Copy link
Collaborator

How to work with format: uuid?

I would say to ignore for the TS plugin(string would be fine) but for the Zod plugin we can use z.uuid

Should the format only be used for schema.type === "string", or shall it stay above those?

Following the rules of JSON/OpenAPI it seems that format has a higher prior so should be checked first, same for other types

is this a minor or a breaking change? Without activation flag, I would argue for breaking.

Jeah seems like that would change some types so I was thinking about using a new flag/option strict so we can change all of this based on strict mode.

Should this PR also trickle into downstream packages like zod or swr?

We will indeed also need those changes for Zod and Faker but I was also thinking about combining the logic between the 3 plugins so we only need to write the logic once but have not yet found the time to do so.

@vchirikov
Copy link

I'm also interesting in correct generation to FormData / Blob for:

      properties:
        file:
          type: string
          format: binary

@stijnvanhulle What I can use as workaround for now?

@stijnvanhulle
Copy link
Collaborator

Moved to #935

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