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

fix: Go back to generating enums out of unions containing enums #2149

Merged
merged 14 commits into from
Jan 7, 2025

Conversation

Twixes
Copy link
Contributor

@Twixes Twixes commented Jan 6, 2025

Problem

feat: improve literal union type handling #1927 improved some cases of handling union types, but it broke one we rely heavily on at PostHog:

Before #1927 types such as type X = Enum.A | Enum.B were output "enum": ["a", "b"], since #1927 they are instead series of anyOf – whereas type X = "a" | "b" continued to be enum. This doesn't seem to have been covered by any test case.

The anyOf output breaks our downstream processing of the schema, and just seems less practical than enum.

Changes

This PR updates LiteralUnionTypeFormatter to support enums too – their underlying values are literal after all, and ts-json-schema-generator did make use of this before the regression.

I committed so that it's easily immediately what the difference in union output is before (on next) vs. after (this branch).
See here: a734160#diff-ba53f2224477f939f2a9762fe35a747aede17fd1e8caab8e4054b135857f2de9

Version

Published prerelease version: v2.4.0-next.6

Changelog

🎉 This release contains work from new contributors! 🎉

Thanks for all your work!

❤️ Michael Matloka (@Twixes)

❤️ null@dcharbonnier

❤️ Werner Robitza (@slhck)

❤️ Bence Balogh (@baloghbence0915)

🚀 Enhancement

🐛 Bug Fix

⚠️ Pushed to next

🔩 Dependency Updates

Authors: 7

@Twixes
Copy link
Contributor Author

Twixes commented Jan 6, 2025

Happy to address feedback @domoritz :)

test/valid-data/enums-union/main.ts Show resolved Hide resolved
test/valid-data/enums-string-union/schema.json Outdated Show resolved Hide resolved
src/TypeFormatter/LiteralUnionTypeFormatter.ts Outdated Show resolved Hide resolved
src/TypeFormatter/LiteralUnionTypeFormatter.ts Outdated Show resolved Hide resolved
@Twixes
Copy link
Contributor Author

Twixes commented Jan 6, 2025

Good points, should be addressed now @arthurfiorette!

@Twixes Twixes requested a review from arthurfiorette January 7, 2025 18:08
src/TypeFormatter/LiteralUnionTypeFormatter.ts Outdated Show resolved Hide resolved
src/TypeFormatter/LiteralUnionTypeFormatter.ts Outdated Show resolved Hide resolved
test/valid-data/enums-union/schema.json Show resolved Hide resolved
@arthurfiorette arthurfiorette self-assigned this Jan 7, 2025
Copy link
Member

@domoritz domoritz 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. Just one comment. Thanks for the pull request!

src/TypeFormatter/LiteralUnionTypeFormatter.ts Outdated Show resolved Hide resolved
@arthurfiorette
Copy link
Collaborator

I fixed some linting issues and added a test case that also ensures some of my use cases. When enums are exported we cannot allow any merging of their actual types.

@@ -43,8 +48,8 @@ export class LiteralUnionTypeFormatter implements SubTypeFormatter {
};
}

const values = uniqueArray(types.map(getLiteralValue));
const typeNames = uniqueArray(types.map(getLiteralType));
const values = uniqueArray(types.flatMap(getLiteralValues));
Copy link
Member

Choose a reason for hiding this comment

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

this can probably become map again if we fix the thing below

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did it for us since i also need this PR :)

351e144

@arthurfiorette arthurfiorette merged commit ddd2d2c into vega:next Jan 7, 2025
4 checks passed
Copy link

github-actions bot commented Jan 7, 2025

🚀 PR was released in v2.4.0-next.6 🚀

@Twixes Twixes deleted the enums-string-union branch January 10, 2025 23:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants