Skip to content

[improvement] Generate more lenient enums#85

Merged
ferozco merged 2 commits intodevelopfrom
fo/js-enums
Jun 4, 2019
Merged

[improvement] Generate more lenient enums#85
ferozco merged 2 commits intodevelopfrom
fo/js-enums

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