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

Fixes #3837 #3844

Merged
merged 1 commit into from
Jun 13, 2024
Merged

Fixes #3837 #3844

merged 1 commit into from
Jun 13, 2024

Conversation

ncave
Copy link
Collaborator

@ncave ncave commented Jun 13, 2024

@ncave
Copy link
Collaborator Author

ncave commented Jun 13, 2024

@MangelMaxime For some reason that I don't quite understand, there seems to be a special case for single unnamed union case fields (when used with TypeScriptTaggedUnion).

Disabling it fixes the linked issue, tests are still passing, but please chime in if you know more about it.

@MangelMaxime
Copy link
Member

I don't know much about TypeScriptTaggedUnion.

I never really used the TypeScript target unless for writing the documentation about it. Which is kind of ironic 🤣

I suppose you left the previous code commented on purpose?

@MangelMaxime MangelMaxime merged commit 7a9db87 into fable-compiler:main Jun 13, 2024
19 checks passed
@ncave
Copy link
Collaborator Author

ncave commented Jun 13, 2024

@MangelMaxime

I suppose you left the previous code commented on purpose?

Yes, as I don't understand what was it supposed to do (some sort of optimization perhaps for certain cases of unions with only one unnamed field?).
And it's not just for TypeScript. Using TypeScriptTaggedUnion attribute will add a tag property for JS too.

@ncave ncave deleted the fix3837 branch June 13, 2024 19:12
@MangelMaxime
Copy link
Member

Perhaps, it was trying to do something similar to StringEnum which allows to map DUs case to a string.

But if someone, wants to do that they can use StringEnum and now TypeScriptTaggedUnion is consistent for all type of cases. We will see, if someone report a problem with it that we are not aware of.

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.

2 participants