-
-
Notifications
You must be signed in to change notification settings - Fork 576
Feat: Add support for pattern properties #2288
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
base: main
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for openapi-ts canceled.
|
🦋 Changeset detectedLatest commit: d6d0f0e The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
@gzm0 yes please feel free to review! The round-robin assignment was only meant to divide the PR reviews among us evenly, but always feel free to jump in and stamp a PR if you have context/time |
Any more movement on this? |
b299279
to
50ad586
Compare
@gzm0 thanks for the feedback! |
…repo and enhance type definitions in v2 and v3 schemas Until openapi-ts/openapi-typescript#2288 is merged we can use a fork to have the patternProperties openapi spec included into the generated typing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Sorry for the delay on this one.
The behavior looks good. There is just a control flow concern. Then I think we can ship this.
} | ||
|
||
if (addlTypes.length === 0) { | ||
return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not happen, but if it does, the behavior is horrible: it will completely ignore coreObjectType
and just return undefined
.
My recommendation is to remove the initial if (schemaObject.additionalProperties || options.ctx.additionalProperties || schemaObject.patternProperties) {
and basically always calculate addlTypes
(maybe call it stringIndexTypes
or something).
Then you can, at the end of the function:
if (addlTypes.length == 0) {
return coreObjectType.length ? ts.factory.createTypeLiteralNode(coreObjectType) : undefined;
}
return tsIntersection( // etc....)
Just in case I got it completely wrong what is happening here and this code is actually reachable (and necessary), please add a test case for it (or point out to ignorant me, which existing test case covers this :P).
Changes
Add support for patternProperties in addition to additionalProperties (#754).
Building on the work of @gzm0 in 1901 by adding tests
Ran the command
pnpm run update:examples
which created 129 new files and updated 69, none of which seemed related to this change. Let me know if you wan't me to do it anyways.How to Review
Refer to comments in original PR 1901
Validate tests are satisfactory
Checklist
docs/
updated (if necessary)pnpm run update:examples
run (only applicable for openapi-typescript)