Skip to content

[improvement] Generate more lenient enums #85

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 4, 2019
Merged

[improvement] Generate more lenient enums #85

merged 2 commits into from
Jun 4, 2019

Conversation

ferozco
Copy link
Contributor

@ferozco ferozco commented Mar 18, 2019

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 constant map

@ferozco ferozco requested a review from a team as a code owner March 18, 2019 16:49
@iamdanfox
Copy link
Contributor

iamdanfox commented Mar 28, 2019 •
edited
Loading

Is there any possible way this PR could cause consumers of conjure-typescript generated packages to fail compilation?

@ferozco
Copy link
Contributor Author

ferozco commented Mar 28, 2019

I don't believe so. As the linked issues suggests, this type of enum is more expressive than the previous one.

@ferozco
Copy link
Contributor Author

ferozco commented Jun 4, 2019

👍

@ferozco ferozco merged commit 3164166 into develop Jun 4, 2019
@ferozco ferozco mentioned this pull request Jun 4, 2019
Merged
@ferozco
Copy link
Contributor Author

ferozco commented Jun 21, 2019

It turns out that this change actually can result in compile breaks in some edge cases. I've outlined the problem, and a proposed workaround in the following playground: https://www.typescriptlang.org/play/#src=%2F%2F%20Old%20codegen%0D%0Aexport%20const%20enum%20OLD_ENUM%20%7B%0D%0A%20%20%20%20FOO%20%3D%20%22FOO%22%2C%0D%0A%20%20%20%20BAR%20%3D%20%22BAR%22%2C%0D%0A%20%20%20%20BAZ%20%3D%20%22BAZ%22%2C%0D%0A%7D%0D%0A%0D%0A%2F%2F%20New%20codegen%0D%0Aexport%20type%20NEW_ENUM%20%3D%20%22FOO%22%20%7C%20%22BAR%22%20%7C%20%22BAZ%22%3B%0D%0Aexport%20const%20NEW_ENUM%20%3D%20%7B%0D%0A%20%20%20%20FOO%3A%20%22FOO%22%2C%0D%0A%20%20%20%20BAR%3A%20%22BAR%22%2C%0D%0A%20%20%20%20BAZ%3A%20%22BAZ%22%0D%0A%7D%0D%0A%0D%0A%2F%2F%20Old%20behaviour%0D%0Afunction%20subset_old(param%3A%20OLD_ENUM.FOO%20%7C%20OLD_ENUM.BAZ)%20%7B%7D%0D%0Afunction%20defaultParam_old(value%20%3D%20OLD_ENUM.FOO)%20%7B%7D%0D%0AdefaultParam_old(OLD_ENUM.BAR)%3B%0D%0A%0D%0A%2F%2F%20New%20behaviour%20-%20fails%20to%20compile%0D%0Afunction%20subset_new(param%3A%20NEW_ENUM.FOO%20%7C%20NEW_ENUM.BAZ)%20%7B%20%7D%0D%0A%0D%0A%2F%2F%20Fails%20to%20compile%20in%203.3.4000%0D%0Afunction%20defaultParam_new(value%20%3D%20NEW_ENUM.FOO)%20%7B%7D%0D%0AdefaultParam_new(NEW_ENUM.BAR)%0D%0A%0D%0A%2F%2F%20New%20behaviour%20-%20work%20around%0D%0Atype%20Extends%3CT%2C%20U%20extends%20T%3E%20%3D%20U%3B%0D%0A%0D%0Afunction%20subset_new_workaround(param%3A%20Extends%3CNEW_ENUM%2C%20%22FOO%22%20%7C%20%22BAZ%22%3E)%20%7B%7D%0D%0A%2F%2F%20Works%20in%203.3.4000%0D%0Afunction%20defaultParam_workaround(value%3A%20NEW_ENUM%20%3D%20NEW_ENUM.FOO)%20%7B%20%7D%0D%0A%0D%0A%2F%2F%20Fails%20to%20compile%20invalid%20subset%0D%0Afunction%20myFunc3(param%3A%20Extends%3CNEW_ENUM%2C%20%22foo%22%3E)%20%7B%7D%0D%0A%0D%0A

ferozco added a commit that referenced this pull request Jun 21, 2019
@ferozco
Revert "[improvement] Generate more lenient enums (#85)"
8609c08 
This reverts commit 3164166.
@ferozco ferozco mentioned this pull request Jun 21, 2019
Merged
ferozco added a commit that referenced this pull request Jun 21, 2019
@ferozco
Revert "[improvement] Generate more lenient enums" (#93)
70549d4 
* Revert "[improvement] Generate more lenient  enums (#85)"

This reverts commit 3164166.

* merge conflicts
@p-szm p-szm mentioned this pull request Sep 10, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iamdanfox iamdanfox iamdanfox approved these changes

Assignees
No one assigned
Labels
merge when ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ferozco @iamdanfox