Elmish.WPF
Elmish.WPF copied to clipboard
elmish
Add vm type information to `BindingData` and `VmBinding`
Waiting on #505 and #504
This Pull Request adds another type parameter to BindingData which allows it to carry the actual view model property type statically through the builder.
From a conversation with @TysonMN:
When used in the context of "dot" notation in the XAML project, you will be able to get compile-time help for things like ViewModelProp.SubModel.Prop. If those were constrained to obj, you wouldn't get compile-time checking of those types. Also XAML will warn you about type issues (number vs string, etc)
Note that the current usage of Binding<'model,'msg> constrains the new type parameter to be obj for every construction of it (this is the entire public interface in Binding.fs). This is intentional, as all of those bindings are by necessity constrained to obj in order to work in the various SubModel and SubModelSeq bindings. The advantage mentioned above will not be realized until after a static helper is created that can turn a binding into a statically-typed getter or setter.
I merged the previous PR using FastHub with merge type "rebase". I see this merged a commit with a different hash.
The other options were "merge" and "squash". I think "merge" creates a merge commit. I will try "squash" with the next PR.
Neither FastHub nor GitHub's app provide me the ability to update the branch. In GitHub's app though, there is a card saying that there are merge conflicts. I will check shortly if I can fix this on the GitHub website on my work laptop.
I don't see a way to drop the previous commit via the GitHub website. Can you do that and push the resulting branch?
Why always the ValueOption.map box >> ValueOption.toObj >> unbox sequence? Does type safety interfere with ValueOption.toObj somehow?
@the-not-mad-psychologist
Why always the
ValueOption.map box >> ValueOption.toObj >> unboxsequence? Does type safety interfere withValueOption.toObjsomehow?
What it does is handle both cases where 'a is nullable and isn't nullable. Boxing the inner value and then unboxing it like this makes a nullable type just return null, while a non-nullable type will throw an exception in the null case (which is correct behavior).
Converting into draft until #474 is merged in. Also added tests (enabled by #474)
@TysonMN I had to revert some of the changes from PR 474 that you added because they moved the box functions into BindingData which breaks this PR. That boxing now doesn't happen until createBinding is called. However, it's possible that we could reintroduce those changes just with id instead of box.
Can you make a PR before this one that adds ofNull and toNull for ValueOption along with some tests containing examples with data on which they could be called?
I want to see how toNull and ofNull in ValueOption are used. Specifically, I want to see a use case where toNull returns Error.
Can this happen with both a dynamic view model and a static view model?
Can you create executable projects for each type of view model for which this is possible?
For a dynamic view model, you can edit one of the samples if you want. We don't necessarily need to merge in this code. Best case scenario is that this helps me figure out how to make this error case impossible. If I can't find a way to make it impossible, I might turn this code into tests.
@TysonMN it's currently always impossible in DynamicViewModel because the type of 'vm is always obj, which is always nullable.
As to where it is used, ValueOption.toNull is used in 3 places in Get<'a>.Base(). ValueOption.ofNull is used in one place in Set<'a>.Base(). DynamicViewModel only ever calls Get<obj> and Set<obj>, so there's nothing really to test other than that null values go all the way through.
I can add a test in BindingVmHelpersTests.fs to illustrate the failure mode. Basically you create one using Initialize<'a>() for a nullable type, set it to null, box it to obj (needed for Update()), unbox it to a non-nullable type, then call Get<'a>() on it.
Quoting https://github.com/elmish/Elmish.WPF/pull/470#issuecomment-1160492351
However, it's possible that we could reintroduce those changes just with
idinstead ofbox.
I think I prefer that. Can you make that change?
Quoting https://github.com/elmish/Elmish.WPF/pull/470#issuecomment-1160492351
However, it's possible that we could reintroduce those changes just with
idinstead ofbox.I think I prefer that. Can you make that change?
It's in the first commit already. I liked it a whole lot better.
Can you squash the commit with message
into the commit that adds the code being tested?
Can you also squash the commit with message
into the various commits where the code being changed was first added/changed?
@TysonMN moved the other comment to the right place. Do you have anything else for this PR?
@TysonMN I put #498 before this one now, since I was rebasing a lot. That one now needs to be merged in first
For each comment that had a pin put in it, can you add a comment on the new PR that is adding that code, link back to the pinned comment here, quote what I asked, and (if you responded here) quote your response?
For each comment that had a pin put in it, can you add a comment on the new PR that is adding that code, link back to the pinned comment here, quote what I asked, and (if you responded here) quote your response?
Done for every comment explicitly referenced as pinned for a future PR.