-
-
Notifications
You must be signed in to change notification settings - Fork 531
Fix defaultNonNullable behavior #1445
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
Conversation
|
Deploying with
|
Latest commit: |
01eb231
|
Status: | ✅ Deploy successful! |
Preview URL: | https://5d4fc629.openapi-ts.pages.dev |
Branch Preview URL: | https://fix-default-non-nullable.openapi-ts.pages.dev |
7c4305c
to
506eb49
Compare
}, | ||
], | ||
[ | ||
"defaultNonNullable > parameters aren’t required even with defaults", |
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 new test asserts that a parameter
with a default is ignored, but a response object with a default is not
506eb49
to
01eb231
Compare
("default" in v && options.ctx.defaultNonNullable) | ||
("default" in v && | ||
options.ctx.defaultNonNullable && | ||
!options.path?.includes("parameters")) // parameters can’t be required, even with defaults |
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 seems like the dumbest solution to the problem, but I actually think it’s the correct one—parameters go through normal SchemaObject transformation, however, in this one scenario we apply different rules because of where they live (in any parameters
object). I can’t think of a single scenario where this wouldn’t be true.
I also feel like the alternative—trying to override ctx.defaultNonNullable
conditionally based on the parent—is a worse approach that could lead to other unintentional bugs.
Changes
defaultNonNullable: true
can’t make parameters required even if a default value is given (comment).How to Review
Checklist
docs/
updated (if necessary)pnpm run update:examples
run (only applicable for openapi-typescript)