Skip to content
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

Implement enums as type union but still have a dedicated type #2740

Closed
3 of 7 tasks
jvassev opened this issue Mar 24, 2021 · 3 comments
Closed
3 of 7 tasks

Implement enums as type union but still have a dedicated type #2740

jvassev opened this issue Mar 24, 2021 · 3 comments
Labels
closed-for-staleness effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. module/compiler Issues affecting the JSII compiler module/runtime Issues affecting the `jsii-runtime` p2

Comments

@jvassev
Copy link

jvassev commented Mar 24, 2021

🚀 Feature Request

Affected Languages

  • TypeScript or Javascript
  • Python
  • Java
  • .NET (C#, F#, ...)
  • Go

General Information

  • JSII Version: 1.25.0
  • Platform:
  • I may be able to implement this feature request
  • This feature might incur a breaking change

Description

Simplify Typescript enum generation based on a type union. This issue was prompted by this discussion: https://github.com/cdk8s-team/cdk8s/issues/215:

Benfits:

  • no new api types (enums are just constraints in the string type in json schema)
  • more idiomatic usage in typescripts: string literals instead of enums
  • still great IDE completion
  • avoid problem where the string constant value starts with a number
  • a type for the union is still emitted in case you want to have a var of this type
  • keep the code in cdk8s closer to a bare manifest

Proposed Solution

Example:
Current emitted code:

/**
 * @schema VerticalPodAutoscalerSpecResourcePolicyContainerPoliciesControlledResources
 */
export enum VerticalPodAutoscalerSpecResourcePolicyContainerPoliciesControlledResources {
  /** cpu */
  CPU = "cpu",
  /** memory */
  MEMORY = "memory",
}

Proposed change:

/**
 * @schema VerticalPodAutoscalerSpecResourcePolicyContainerPoliciesControlledResources
 */
export type VerticalPodAutoscalerSpecResourcePolicyContainerPoliciesControlledResources =  "cpu" |  "memory";

This would be a breaking change but migration would be trivial. At the cost of one extra type - the union of the current enum and the union of the bare strings - the change can be made fully backward compatible:

@jvassev jvassev added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Mar 24, 2021
@RomainMuller
Copy link
Contributor

That's an interesting idea - and it appears to go in the same direction as our aws/aws-cdk-rfcs#193. I'd be supportive of this... I think it's mostly compatible with our current implementation (it would change de-serialization of enum values in the runtimes, though - but it should be relatively easy to do.

We however need to ensure we are able to maintain a consistent representation of the "enum constants" in languages where they're not just "string constants" (Java, C# - at minimum). This means we need to turn the string value into a constant name (including SHOUT_CASE transformation) - and so the literals probably will need to be restricted to valid identifier components, or things we know we can transform without risk of collision (maybe something like ^/[a-z][a-z0-9_-]*$/I, and ensuring two literals of the same union don't share the same SHOUT_CASE representation?)

In any case, we're probably not going to be able to get into this really soon, but if you want to give it a shot and try to implement that yourself - I'll be happy to provide guidance & reviews along the way.

@RomainMuller RomainMuller added effort/large Large work item – several weeks of effort p2 module/compiler Issues affecting the JSII compiler module/runtime Issues affecting the `jsii-runtime` and removed needs-triage This issue or PR still needs to be triaged. labels Mar 25, 2021
@RomainMuller RomainMuller removed their assignment Jun 24, 2021
@github-actions
Copy link
Contributor

This issue has not received any attention in 2 years. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Jun 25, 2023
@rassie
Copy link

rassie commented Nov 7, 2023

Can this be reinstated? Enums instead of type unions are still a pain and still make the lives of e.g. cdk8s's users (and developers) way more difficult.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-for-staleness effort/large Large work item – several weeks of effort feature-request A feature should be added or improved. module/compiler Issues affecting the JSII compiler module/runtime Issues affecting the `jsii-runtime` p2
Projects
None yet
Development

No branches or pull requests

3 participants