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

Allow non F# union-types to be used as messages #21

Merged
merged 1 commit into from
Feb 22, 2019

Conversation

matthid
Copy link
Contributor

@matthid matthid commented Feb 22, 2019

fixes #20

Actually, I think this should have created problems for anyone not using F#-unions. I think we should add a test for this case.
Are there already tests in this library?

@et1975
Copy link
Member

et1975 commented Feb 22, 2019

You are the first to forgo F# unions for messages, didn't seem a likely scenario, but here we are.

I'd usually fire up one of the samples to test the debugger, but if you can think of some integration tests that would be cool.

Otherwise, does this PR as it is completely solve the issue for you? Also, do you want to backport it to 2.x branch (the master branch is for 3.x release, which will not release until elmish 3.x is out)?

@matthid
Copy link
Contributor Author

matthid commented Feb 22, 2019

Otherwise, does this PR as it is completely solve the issue for you?

It seems like it does. I don't like how it depends on fable internals but yeah I need to verify in the complete application (wrote a test and verified this fixes it as of now)

Also, do you want to backport it to 2.x branch

Having a v2 release with this would be very much appreciated, what do I need to do? Send the same change to v2 branch? Cherry-pick?

@et1975
Copy link
Member

et1975 commented Feb 22, 2019

Send the PR for v2 branch, the commit is up to you.

@matthid
Copy link
Contributor Author

matthid commented Feb 22, 2019

Regarding elmish v3. While fixing various issues around the debugger I think it might make sense to introduce a shutdown function to the program object (along init/update/view) such that you can properly shutdown and close the socket in a unit-test... or maybe not?

@et1975
Copy link
Member

et1975 commented Feb 22, 2019

There's a discussion @ elmish/elmish#162, I will consider addressing this use case as well.

@et1975 et1975 merged commit 0acff04 into elmish:master Feb 22, 2019
match FSharpValue.GetUnionFields(x, typeof<'a>) with
| case, _ -> case.Name
let t = typeof<'a>
if (isNull t?cases)
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of relying in the internal JS representation of F# unions by Fable (which may change in the future), you can just use FSharpType.IsUnion: https://github.com/fable-compiler/Fable/blob/e2ce9884390d95d223044e666c7c251c7392b428/tests/Main/ReflectionTests.fs#L385

Latest Fable.Core also includes some reflection helpers: https://github.com/fable-compiler/Fable/blob/e2ce9884390d95d223044e666c7c251c7392b428/src/Fable.Core/Fable.Core.Util.fs#L26-L32

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.

Breaking change in v2 related to non-F# objects?
3 participants