-
-
Notifications
You must be signed in to change notification settings - Fork 525
Add a flag to skip Schema prefix when rendering root types #1958
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
|
dd55bfe
to
2a92289
Compare
2a92289
to
3fa8dbe
Compare
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.
In favor of this in general! I think we need some tests here just to ensure it works as expected, but that test can be minimal since it’s an opt-in flag.
Also agree with the “off/on” approach here, since the prefixes MAY be Schema
but also may be Response
or Request
, etc. I don’t think it would make sense to have a flag to manually set that prefix, since that prefix isn’t hardcoded to begin with.
But with that in mind, a test that **tests what happens when you have a naming conflict between #/components/schemas
and #/components/responses
would be great (not required, but it would really help validate that all bases are being covered)
Hey @drwpow thanks for review, I will add tests and update the PR with requested changes some time next week. |
@drwpow I will add another test which tests when there is a parameter with the same name. I added a test case for the new flag. |
@drwpow can you please take a look again? |
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.
Sorry for the delay. This looks FANTASTIC. Thank you!
That's awesome, thanks @drwpow! |
…s#1958) * Add a flag to skip Schema prefix when rendering root types * Add missing option in tests * Add test case * Fix linting issues * Update examples * Update docs * Update examples
@sultaniman @drwpow First, great job on adding the new --root-types-no-prefix flag! It’s a really useful addition. However, I've noticed that when the --root-types-no-prefix flag is used alongside the --enum flag, it generates a root type with the same name as the enum, leading to naming conflicts. Given that enums already serve as types in TypeScript, it seems unnecessary to generate a separate root type in these cases. Skipping the root type when both flags are used would help avoid conflicts and streamline the output. It would be great if this could be addressed, whether by skipping the root type or considering other potential solutions to prevent such conflicts. Looking forward to seeing how this can be improved! |
👋 Good point @sultaniman. Regarding the desired behavior, I think it would be a bit odd if this feature doesn't work when the One option could be to skip generating the root type only for enums when both the |
Yeah with some of these flags, they’re not always perfectly compatible with one another. We could either throw an error that these flags don’t work together. Or do what @fluctus suggested by modifying the behavior of |
alright then as @fluctus suggested it would mean if |
@sultaniman if @drwpow agrees with this solution, would you be handling the fix? |
Hey @fluctus sorry for being slow, I was sick, will try to carve out some time this week for it. |
@sultaniman No worries, hope you're feeling better! Thanks for taking the time to look into this. |
@fluctus can you please provide some minimal OpenAPI spec example with enum definitions you have? |
@sultaniman Sure, this is a sample spec that generates types with the same name for enums:
Without
With
Which results in these TS errors: |
@fluctus thank you very much for providing an example I will use it in the tests. |
Hi @sultaniman @fluctus I have also run into exactly those 3 errors, because I was running both |
@theo-staizen yes, @sultaniman is working on a fix that will work with both flags. |
Hey guys, sorry for delays I wanted to work on this last week but was unable to do so. I will do it in the coming weekend and the following week. |
hi @sultaniman, sorry to bother you, but have you had a chance to work on this at all? is there anything we can do to help? |
Good evening @drwpow @theo-staizen @fluctus sorry for being so slow had many personal things going on recently. I linked the PR which disables |
Changes
This PR adds another cli flag
--root-types-no-prefix
which complements--root-types
.If set it will render root types without
Schema
prefix.Checklist
docs/
updated (if necessary)pnpm run update:examples
run (only applicable for openapi-typescript)