Skip to content

[improvement] Generate more lenient enums #170

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

Merged
merged 7 commits into from
Sep 15, 2021
Merged

[improvement] Generate more lenient enums #170

merged 7 commits into from
Sep 15, 2021

Conversation

p-szm
Copy link
Contributor

@p-szm p-szm commented Sep 10, 2021

Fixes #80

Based on #85 (which ended up being reverted) but I used namespaces instead of objects in order to preserve back compatibility.

Before this PR

We generated TypeScript enums which were difficult to use across library boundaries since equivalent enums could no be assigned to each other. See #80 for more details

After this PR

We generate a union type and a namespace.

Notes

I tested the edge case in #85 (comment) which caused the original attempt to be reverted and this time everything compiles correctly:

https://www.typescriptlang.org/play?#code/PTAEHkBsBNQYwPbQKYHNkDsCwAoZAPABwQCcAXeBDAZwswFcBbCAGQBEB9AUQDkBVALKgA3rlDjQAMXDhQAXlAAiaeEUAaMRIBCAQQBK8pbr3rN43QC1Diy6ZwBfXLhCgeyAO6UU6bHiKkKDABDRmRqQiC4ZFcuAHVufiFRHAlQAmJyUDIAT0JolWsVRQBuMzT-TJy80GNrYxKy9ICs3OjLOp0LBpxGiopEGgoChWUZbtSmzIHaGv0Ok1KUiUn+qhn2kdtFxz8MiironjiEwUMAa2RshAAzFrybmPjeQUXnMChYACNkAAsggDcAJYIegkXDXegYOBkYEYUDUeifajIMgcBAwAAUERIIQAXKxOM8BAA6AoAHwJJxJlgAlCIdhCoTCqKAUNcgvRIGQAApBHGMNGY-5BSD0aIKcDsKmkmR04Q7Nkcrm8-mC6AYyWExLE4w0144FxuTzfP5AkEkUAAWkojEIgMgYXBkOhsPhiORqIwHixfLxj2l5P9RJ1nTloB2bykQXt1CyCBtdodoEBGGFkEBsARSJRTqZrsY2UkzoAzD7+fiuPgyJhoNQADxHJ6JNRKa4IBCKAB8cojOCAA

@changelog-app
Copy link

changelog-app bot commented Sep 10, 2021

Generate changelog in changelog/@unreleased

Type

  • Feature
  • Improvement
  • Fix
  • Break
  • Deprecation
  • Manual task
  • Migration

Description

Generate more lenient enums.

Check the box to generate changelog(s)

  • Generate changelog entry

@policy-bot policy-bot bot requested a review from jkozlowski September 10, 2021 13:20
@ferozco
Copy link
Contributor

ferozco commented Sep 10, 2021

Lets try out the code change in a large internal repo and then we should be good to go

@jkozlowski jkozlowski requested review from fawind and removed request for jkozlowski September 13, 2021 10:31
@p-szm
Copy link
Contributor Author

p-szm commented Sep 13, 2021

I tested this on one of our conjure generated packages that has a bunch of enums, and it all seems to work.

Copy link
Contributor

@ferozco ferozco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@p-szm p-szm merged commit e81cffe into develop Sep 15, 2021
@tjwilson90
Copy link

This change breaks the generated code for empty enums. Before this change

types:
  definitions:
    default-package: "com.palantir.cosmos.generated"
    objects:
      AircraftGeoPointField:
        values: []

generated

export enum AircraftGeoPointField {
}

and after this change it generates

export namespace AircraftGeoPointField {
}

export type AircraftGeoPointField = keyof typeof AircraftGeoPointField;

which fails to compile with the error

> Task :cosmos-api:compileTypeScript FAILED
cosmos-generated/aircraftGeoPointField.ts(4,50): error TS2708: Cannot use namespace 'AircraftGeoPointField' as a value.
cosmos-generated/aircraftGeoPointField.ts(4,50): error TS4081: Exported type alias 'AircraftGeoPointField' has or is using private name 'AircraftGeoPointField'.

@p-szm
Copy link
Contributor Author

p-szm commented Sep 16, 2021

thanks for spotting @tjwilson90 . I think we can generate this instead for empty enums:

export namespace AircraftGeoPointField {
}

export type AircraftGeoPointField = never;

I can put up a fix first thing tomorrow morning if you think that works, but I'm also happy to revert in the meantime.

@p-szm
Copy link
Contributor Author

p-szm commented Sep 16, 2021

Also, I'm curious - do you actually use empty enums in your typescript code, or was this an API in progress?

@ferozco
Copy link
Contributor

ferozco commented Sep 16, 2021

@patrickszmucer is never the right type here, seems like never can be assigned to anything?

@tjwilson90
Copy link

My conjure api has a lot of redundancy / self similarity and is itself generated (you can imagine Aircraft being one of many different types of objects). I generate a similar search api for all of them. Some of those generated types (like aircraft) don't have any geo point fields, so the corresponding enum is empty.

@tjwilson90
Copy link

I don't mind waiting until tomorrow. It blocks an autobump which isn't high priority.

@p-szm
Copy link
Contributor Author

p-szm commented Sep 17, 2021

@ferozco you're right - seems like never is assignable to anything so it would be a bit dangerous to generate it (although I'm not too worried because empty enums should be super rare).

However, I think I found a better solution - we can use void instead which can't be instantiated and also can't be assigned to anything else. Typescript folks did something similar to the window.name variable which used to be never but it was changed to void for better type safety - microsoft/TypeScript-DOM-lib-generator#883

@p-szm
Copy link
Contributor Author

p-szm commented Sep 17, 2021

Fix here #176

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enums should be generated as string constant maps
4 participants