-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Adds the option of generating real TS enums instead of string unions #2854
Conversation
Removes @rtk-query/oazapfts-patched in favor of the updated oazapfts
Open in CodeSandbox Web Editor | VS Code | VS Code Insiders |
+1 |
@phryneas any chance you can take a look at this? 🥺 🙏 |
@markerikson any chance you can take a look at this? |
@phryneas, @markerikson When is it possible to check and merge this PR. Would be really helpful to a lot of dev's using the generator. |
@labmorales thanks for this great PR, unfortunately it seems that it would not compile with the latest master. Would it be possible that you make it work again? |
✅ Deploy Preview for redux-starter-kit-docs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
@labmorales thanks for merging. It seems that it is necessary to upgrade typescript version as well, since build & tests would fail otherwise. |
Just to pitch in here, it would be really valuable in a current project I'm working on to be able to iterate over enums coming from the back end. One source of truth… @labmorales You're work on this is appreciated. |
FYI: I've published @fjakop's branch in our internal NPM registry and been using it. Working really well! |
Hey, @fjakop thanks for the work! Really appreciate it 😄 . It should work now. We've been using this type generator for about 6 months now for several projects (https://www.npmjs.com/package/rtk-query-codegen-openapi-labmorales). Not sure if that's enough to remove the "Please note that |
Dang, it. Git Operations are down. Will push when it's back again. |
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 fe74bc6:
|
@phryneas @markerikson any chance one of you can take a look at this? |
@phryneas @markerikson can we help to get this PR merged? |
+1 Definitely need this! |
oazapfts needs to be updated to Version 4.7.3 |
Is there a reason for that @Valli- ? The master branch still points to the old patched version of oazapfts and the current version as it is ^4.2.0 will pick up 4.7.3 when the lock file is generated/updated. Also, why bother? It's been almost a year since I opened this PR. It's not likely that this will get merged. If anyone is interested in using this feature you can use this fork of mine https://www.npmjs.com/package/rtk-query-codegen-openapi-labmorales . |
@labmorales I do hope to get back to the codegen and get all of the PRs for it in eventually - it's unfortunately one of those situations where most other parts of RTK (and my real life) take precedence over it and I'm the only active maintainer that is familiar with the codegen part - and meanwhile the task is getting bigger and bigger :/ I sincerely want to apologize. I hope I'll get around to it soon. It's good to know that there is your fork out there that people can use though. |
Because of this issue: oazapfts/oazapfts#367, but you are right it should pick up the correct version. |
Any updates on this? |
I just wanted to make a PR on my own to just bump the oazapfts version, but then stumbled upon this PR that does the same and a bit more, and this one that also does the same. The changes are pretty straightforward, yet they have been open for many months. My personal itch was that after an update on the backend (FastAPI/Pydantic), the Spec contained In general, I fear that this package might slowly die if more API spec generators use unsupported features and this package never gets any updates. I understand that the resources of the maintainers are limited. I'd be happy to help, but it's hard to contribute if PRs aren't reviewed or merged. Is there some other way I can help? I really like this project, but right now I can't recommend it due to the maintenance situation. |
Will be the feature please merged ? |
Hello. We also really need this ability to generate enums, how can we help? |
Just to give people a sense of what's up on our side:
I acknowledge that the codegen side has been very neglected over the past year, and I'm sorry about that. Unfortunately we really are all doing this in our spare time, and it's a question of prioritization and available expertise. Realistically, once we get RTK 2.0 out the door, we can try to turn our attention to the codegen issues and PRs and try to address things there. Now that we've de-scoped how much work will go into 2.0, I'm hopeful that might be relatively soon (next couple months?). I also see a couple of offers to help, and I appreciate that. The biggest issue there is that I'm probably the one most likely to be hitting merge on PRs, and I don't have time or mental capacity to A) read through these PRs and discussions, B) understand the intent, C) meaningfully review and validate changes and decide that they're ready to merge. In theory, if someone could put in the effort to basically do most of that work for me and write it up as an extensive comment here, that would increase the changes I could eyeball it and say "okay, sounds good", and merge this. Like, summarize the original comment, any key points from the rest of the discussion, give me some screenshots or code snippets of before and after output, and whatever else would be relevant to offload the work from my own brain :) |
Let's see if it helps :-) Here's a summary: Motivation of the PRGenerally, openapi-codegen depends on oazapfts for most of the heavy lifting. So, a year ago, someone wanted to implement a feature that required support in oazapfts (PR #2376, issue #2135). To avoid waiting for oazapfts to merge the PR, oazapfts was forked into https://github.com/rtk-incubator/oazapfts . I think at the time, oazapfts was not properly maintained, which makes this decision kinda sensible -- the PR in question was open for more than eight months, from Feb 2022 to Nov 2022. The corresponding PR has long been merged, and oazapfts' maintainer Xiphe has messaged msutkowski about this afterwards. By now, the original oazapfts project seems healthy and alive again. However, we're still on the old fork here. So, in this PR, @labmorales wants to have yet another feature from upstream oazapfts to be usable in openapi-codegen: generating proper enums instead of string unions. E.g. generating something like this: export enum MyEnum {
myFirstValue = "my-first-value",
mySecondValue = "my-second-value",
} instead of this: export type MyEnum = "my-first-value" | "my-second-value" I think the test case over in the oazapfts PR also explains it well. Now, you might say, that's not a very critical feature, why all the hiss? Well, because this PR doesn't just provide us with this one feature, but with all features that have since been merged into oazapfts, like newer typescript support, support for the OpenAPI That's why there's not one, but three open PRs that want to switch to the upstream oazapfts again, all for slightly different reasons: #2854 (this one), #3430, #3434 -- and I almost opened a fourth one. The changesFirst of all, oazapfts and typescript are bumped. This causes a couple of issues that are subsequently fixed:
The discussionThe discussion doesn't contain much of interest, fjakop and labmorales helped each other out to make this branch work, many people say "yes please". If this gets merged, #3434 can be closed. #3430 needs to be rebased, but the changes should become much smaller/easier to review. So, I hope this was not too much of a wall of text. I know that I can't magically make your day 30 hours long, but maybe this makes the work take a bit less of the 24h you have :-) |
Wonderful! @GeorchW , genuinely, thank you for taking the time to write that up! That was a fantastic recap. That gave me larger context, explained why this matters, summarized the changes, and gave reasons why it should be merged. Skimmed the changes very briefly and they line up with what you're saying. I am sorta curious about the (lack of?) test setup, but this all seems plausible. I'll go ahead and merge this, close #3434 , and see if I can actually publish updated versions of the codegen packages. (I haven't tried to publish those before - need to confirm I do have permissions, and then give it a shot.) If you or anyone else can do similar writeups for other major / important PRs, that would also be a huge assist and help me figure out what to do with them. (And yes, @GeorchW , that maybe includes the "delayed tag invalidation" PR that is sitting in the Also to set some additional expectations:
|
Hmm. Our tests are now failing on Looking at them, they seem to be snapshot tests failing due to slightly different output: I see some I thinK these are all improvements? Guess we'll go with it :) |
Also turns out I don't actually have NPM publish rights to |
Regarding the // src/generate.ts ; line 262
const isPureSnakeCase = /^[a-zA-Z][\\w]*$/.test(param.name);
|
Ah, gotcha! Could you file a PR to fix that? Looks like it'll be a few hours until msutkowski can give me publish rights, and I'm busy this evening and first half of tomorrow. I will try to get the new version out tomorrow! |
Thanks so much, guys, you’re incredible! Will be waiting for an update 🙏 |
Released in v1.1.0 |
Added an option of generating real TS enums on oazapfts and was hoping to add the feature here as well 😄. I think a lot of people may benefit from this optional behavior.
It seems like the oazapfts version has all the features included on the oazapfts patched version therefore I replaced it with the new one that has the generate enums feature.
Right now it's just a config option, but I can create a CLI option for it as well. To use it the config file should pass the useEnumType option like this:
This fixes this issue.