Skip to content

Fix tracer support for options (was crashing) #77

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 3 commits into from
Mar 19, 2025

Conversation

mlaily
Copy link
Contributor

@mlaily mlaily commented Mar 16, 2025

Also see fable-compiler/Fable#4082

Fix options not being considered as DU in the tracer.

The previous implementation could actually crash:
if the input is Some(DUCase), Fable.Core.Reflection.isUnion returns true
because it does not "see" the option, and only see the wrapped DUCase.
Reflection.FSharpType.GetUnionCases() would then attempt to get the fields
of the input, and since option types are not considered DU under Fable,
it would throw.

Now there is a special case for options so they work the same as other DU for the tracer.


Note: I also took the liberty to replace the build.fsx file with an equivalent build.fsproj project, because I couldn't manage to get it to build and run without errors locally.

With hindsight, maybe it was just a question of pinning the script-referenced nugets to a working version instead of always restoring the latest one, but I didn't try to go back after I got it working with an fsproj. It seems more robust to me that way.

I kept the build changes in their own commit so they can easily be removed if desired...

@mlaily mlaily changed the title Fix tracer support for options Fix tracer support for options (was crashing) Mar 16, 2025
@mlaily mlaily force-pushed the fix-tracer-support-for-options branch from cd7f403 to a6b3084 Compare March 16, 2025 20:14
@MangelMaxime
Copy link
Member

Using a fsproj for the build is fine to me, but could you please add a build.bat and build.sh scripts for making it easier to invoke the build ?

@et1975
Copy link
Member

et1975 commented Mar 17, 2025

Please open an issue proposing the switch fsx -> fsproj instead of taking liberties, let's get this PR to building without the switch.
Build infra is pain to maintain, but there was recent update in elmish core that could be replicated here and across other elmish org repos.

@mlaily mlaily force-pushed the fix-tracer-support-for-options branch from a6b3084 to d0ec296 Compare March 17, 2025 15:00
The previous implementation could actually crash:
if the input is Some(DUCase), Fable.Core.Reflection.isUnion returns true
because it does not "see" the option, and only see the wrapped DUCase.
Reflection.FSharpType.GetUnionCases() would then attempt to get the fields
of the input, and since option types are not considered DU under Fable,
it would throw.
@mlaily mlaily force-pushed the fix-tracer-support-for-options branch from d0ec296 to 7330652 Compare March 17, 2025 15:03
@mlaily
Copy link
Contributor Author

mlaily commented Mar 17, 2025

Please open an issue proposing the switch fsx -> fsproj instead of taking liberties, let's get this PR to building without the switch. Build infra is pain to maintain, but there was recent update in elmish core that could be replicated here and across other elmish org repos.

No problem, I removed the commit.

EDIT: looks like pinning the build.fsx dependencies versions was enough to fix the build.

@mlaily mlaily force-pushed the fix-tracer-support-for-options branch from 00212d8 to 208dde2 Compare March 19, 2025 06:22
@et1975 et1975 merged commit dc8ba8d into elmish:v4.x Mar 19, 2025
2 checks passed
@mlaily
Copy link
Contributor Author

mlaily commented Mar 19, 2025

Thank you!

@et1975
Copy link
Member

et1975 commented Mar 19, 2025

Thanks, published as 4.1.0

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.

3 participants