-
Notifications
You must be signed in to change notification settings - Fork 400
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
Add option to create string enums based on JSON enums #262
Conversation
bump? Otherwise I may end up using a custom version of this package in our flows here internally at Microsoft until this gets reviewed/released |
I published and installed a dev version of this locally here and see
which I'm trying to debug now, but am not an expert with npm/Node/console logging in this case (not seeing debugging messages I'd expect to see) |
Looks like that error was an interesting edge case and when removed from my schemas it passes. It's very likely that's a bad edge case/incorrect schema definition as it was old and barely used for us. I'd go back and say that this PR is ready for review/merging when possible. Sigh, nevermind. I guess I don't fully understand how this all gets generated together, as in my test case, my enums aren't being placed anywhere, only referenced by enum name in my output TS file, but they are there without my new flag. This is unfortunate for me as I'm nearing this great tool because I can't manage to make it work for what I need to do with it. |
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.
Thanks for the contribution, and sorry about the delay in taking a look at this!
What do you think about taking a slightly different approach here?
- Instead of
enableStringEnums
, would you mind calling the flaginferStringEnumKeysFromValues
? - Instead of implementing it as logic in the parser, could we implement it as a normalizer step that adds
tsEnumNames
to an enum ifinferStringEnumKeysFromValues
is set (and skips the enum iftsEnumNames
is already defined on it)? You'll need to update thenormalize
function to also passoptions
to each normalizer step.
@cdietschrun thank you so much for adding this! Do you need any help with the requested changes by @bcherny? |
I can probably pick this up, if you don't mind @bcherny |
I would love if this feature made it in! |
@cdietschrun @bcherny I'm going to pick this up over the weekend if there are no objections |
Any update on this PR? Worthless to mention the benefits of this |
Closing out stale PRs. Feel free to rebase and re-open if you like. |
Just a fork and apply changes of bcherny#262 from @cdietschrun
Addresses #200 . Tests seem to do what I expect and the snapshot file generates what I want as well.
I'm hopeful this can be merged and released soon, or at least reviewed and I can reply to feedback ASAP.
Thanks!