-
-
Notifications
You must be signed in to change notification settings - Fork 71
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
DRAFT: Improve sub model seq composability #244
Conversation
Got it! :D See the current state of the branch in this PR. Try the In the
Since
In summary, the two API changes I made were
What do you think @cmeeren? |
a57ecc7
to
71030b9
Compare
@ScottHutchinson, just like PR #241, I think you will be interested in these further improves to the |
I'm confused on several fronts here, but first: Currently you're appending a lot of bindings where many overlap the source bindings ( |
Yes, that was a mistake. I pushed a fix to remove those duplicate bindings shortly (about 90 minutes) before you posted that comment. As a side note, I am surprised that these duplicate bindings didn't cause a problem. I am going to investigate that further. |
I forgot what happens when there are bindings with the same name. We log about the duplicate and continue using the first binding. Elmish.WPF/src/Elmish.WPF/ViewModel.fs Line 617 in c20e323
The sample still works because... Counter.bindings ()
|> Bindings.mapModel (fun (_, c) -> c.Value)
|> Bindings.mapMsgWithModel (fun (_, c) msg -> CounterMsg (c.Id, msg)) ...and.. [ "CounterValue" |> Binding.oneWay(fun (_, c) -> c.Value.Count)
"Increment" |> Binding.cmd(fun (_, c) -> CounterMsg (c.Id, Increment))
"Decrement" |> Binding.cmd(fun (_, c) -> CounterMsg (c.Id, Decrement))
"StepSize" |> Binding.twoWay(
(fun (_, c) -> float c.Value.StepSize),
(fun v (_, c) -> CounterMsg (c.Id, SetStepSize (int v))))
"Reset" |> Binding.cmdIf(
(fun (_, c) -> CounterMsg (c.Id, Reset)),
(fun (_, c) -> Counter.canReset c.Value)) ] ...are identical, which is the entire goal of this PR. Here is some output in the logs that I didn't notice before.
Once we have better logging (c.f. #105), it won't be as easy to miss noticing these. |
71030b9
to
9a19c95
Compare
Ok, I hadn't started looking at the code, I just read your description. Continuing, then: I'm in favor of anything that improves Elmish.WPF, but I need to understand this better. It may be the sample, or it may be me missing something here (it's been long since I've delved deeply into Elmish.WPF code), but I can't immediately see why the new API makes things significantly better. I get that |
Good catch. I just removed the "bug" from the code in my initial comment as well.
Ah, yes. I will give you executable code. This makes me realize that I forgot one thing, which is to "push" the composability into the XAML code. I will improve the |
I just pushed this commit, which extracts the individual counter bindings into their own XAML file. Now in the |
That's awesome. Do you have ideas about how to document the new APIs/functionality? Not just how to use it, but when to use it. Is this only useful for recursive bindings?
That would require additional projects for each sample, right? (I know this has been debated before, but I'm unsure if something has changed since then.) Also, I just noticed a typo, not related to this PR but I'm putting it here before I forget - feel free to fix:
Should be "occurrence". |
aad5527
to
ed23e43
Compare
I know!! It makes me so happy :D
On a micro level, "CounterValue" |> Binding.oneWay(fun (_, c) -> c.Value.Count)
"Increment" |> Binding.cmd(fun (_, c) -> CounterMsg (c.Id, Increment))
"Decrement" |> Binding.cmd(fun (_, c) -> CounterMsg (c.Id, Decrement))
"StepSize" |> Binding.twoWay(
(fun (_, c) -> float c.Value.StepSize),
(fun v (_, c) -> CounterMsg (c.Id, SetStepSize (int v))))
"Reset" |> Binding.cmdIf(
(fun (_, c) -> CounterMsg (c.Id, Reset)),
(fun (_, c) -> Counter.canReset c.Value)) The function arguments getting data from the model and sending it to WPF all do "this": You might recall that I gave another (but admittedly weaker) example in #212 (comment) where I would consider using On a macro level, the only use case I see is the present one. The items in a That counter code doesn't contain any On the other hand, this one-two punch of
Not just recursive bindings. This is also helpful in a flat list of submodel bindings. This reminds me that our On a related note, #236 is about the idea of creating a binding specifically for trees (aka recursive bindings). @ScottHutchinson has identified that the existing code is very inefficient. If we created a binding for trees, then we might be able to simplify the current
No extra projects needed. One C# project and one F# project is sufficient. The C# project can be the executable project. It instantiaties I think this would be a good change. I think it would be better to show people with executable code how design-time support works than only describe it in the README. (However, those words in the README are excellent. I prefer to leave them just as they are.)
Great catch. I pushed a fix to |
Good idea. I'll take a look at this PR when you mark it as "Ready for review". Let me know in a comment before that if there's something in particular you want me to look at.
Thanks for the helpful explanations!
That would be great. If you want to have a go at that, knock yourself out.
I see, so more or less what I suggested in #93 except that the F# projects are merged?
I think that's a great idea. (Why haven't we done that already?) Feel free to go ahead with this. |
Yep.
I created issue #246 for this work. |
I created issue #247 for this work. |
Yes, there is one thing. Please consider the implications of removing the I originally requested, contributed, and later regretted this Even though I originally requested the So although there might not be a migration path in theory, maybe a migration path in practice is to add support for the higher-level concepts of throttling, debouncing, and limiting. I say "maybe" because I will need to verify (by implementing one of them) that these features do not prevent |
If we need to make that breaking change with no migration path to identical functionality, we should announce that in an issue and request feedback, and leave that up for a while. And before that we should look at the existing discussions of the feature and any comments we have received there. Is it really impossible to wrap dispatch when we have mapMsg? If we can implement throttling/debouncing, wouldn't it also be possible to implement a generic wrapDispatch? (Note, I haven't looked at the code, and cocariance/contravariance currently just makes my head hurt). |
ed23e43
to
27a1294
Compare
PR #249 is now complete, which added design-time view models to our samples. I rebased the branch in this PR onto In #244 (comment), I said (without that cross out, which I added in the quote):
Now the C# project is the entry point, and we do have design-time view models in use. After rebasing I added one new commit, which is to hook up the view model for |
Yes, it really is impossible. Here is my argument.
type internal TwoWayData<'model, 'msg, 'a> = {
Get: 'model -> 'a
Set: 'a -> 'model -> 'msg
WrapDispatch: Dispatch<'msg> -> Dispatch<'msg>
} The type of On the other hand, the function Wikipedia has an article called Covariance and contravariance (computer science). Overall, I don't like this article very much. I find it too verbose. However, the initial section of the Formal definition subsection clearly and succinctly defines covariant, contravariant, bivariant, variant, and invariant. Another downside of this Wikipedia is that it definitions use the concept of subtyping, which is a form of polymorphism, which is very common in OOP. In contrast, my experience (mostly with F#) is that prefers FP tries to avoid polymorphism (although the Java dependency ZIO claims to both be FP and polymorphic as I read here). Anyway, the takeaway (as the names suggest) is that it is impossible to be both covariant and invariant at the same time. Edited to add an example Here is an example to help make this more familiar. A generic type
However, ignore the functor laws for a moment and suppose
try to create an instance of type (*I might be slightly lying here. Suppose |
I don't think so. My idea here is to name the specific types of dispatch wrapping. Something like type DispatchWrapper =
| Limit of LimitParams
| Throttle of ThrottleParams
| Debounce of DebounceParams I would change our current
...and then change... let dispatch' = wrapDispatch dispatch ...to something like... let dispatch' =
dispatchWrappers
|> Seq.map toDispatchWrappingFunction
|> Seq.fold (>>) id
<| dispatch This (avoids the previous impossibility and) works because the concepts of throttling, debouncing, and limiting a data stream do not depend on the type of the data in the steam. Specifically, |
Thanks. I wrote the following to convince myself of this: let mapWrapDispatch
(map: 'a -> 'b)
(wrap: ('a -> unit) -> ('a -> unit))
: (('b -> unit) -> ('b -> unit)) =
fun dispatch msg ->
IMPOSSIBLE Since this will be a breaking change, I currently stand by my previous remark, though I'm open to other opinions.
Furthermore, if we actually need to implement this ourselves, that requires some research, too, to make sure it's performant (both CPU and memory). |
Do you mean the The issue that created the
You mean the throttling, debouncing, and limiting features, right? My hope is that we can delay this work. If no one is using the |
Yep.
Yep, that's fine. We may want to request comments in an issue to be sure (as sure as we can, anyway). Alternatively we can just release v4 and take it from there if we get any comments. People don't have to upgrade right away, after all. Perhaps that's the best course of action? It certainly allows us to move faster. What do you think? |
Yes, I think moving faster would be ok. We can do a bit of both though. I created issue #253, which can be a source of discussion for both the proposed removal of the |
Great! I'm happy with that approach. Thanks for creating that issue. |
This PR into |
This is a repost as a draft PR of issue #242 since the ability to include specific comments in the corresponding branch is likely useful. The original comment from that issue now follows in full. The branch linked at the start of the second paragraph is now the branch in this PR.
I want the
SubModelSeq
sample to have more composable code. In particular, I want the counter bindings to be defined with the rest of the counter code and then for those bindings to be "mapped" to higher-level bindings at theApp
level.I am trying to achieve this in this branch. Although I am trying to write really good code, I am not suggesting that necessarily merge in any of those commits as is. The primary purpose of this branch is as a proof of concept. I first want to get it working and then consider how to internally structure the code and what the external API should be.
Below, the function
Bindings.mapMsgWithModel
doesn't exist, but I have almost created it. To get this far,the two important changes areone important change is removing thewrapDispatch
feature (which I originally requested, contributed, and later regretted)and changed the type of the binding argument(Edited: I did add issue #243 about changing the type ofOnCloseRequested
from (essentially)'msg
to'model -> 'msg
. I am going to (independently) suggest this second change in a separate issue.OnCloseRequested
, but I realized that it is not important because it doesn't require a change to the public API.)Tomorrow, I plan to continue trying to create
Bindings.mapMsgWithModel
. A remaining difficulty is thetoMsg
binding arguments. I currently don't expect to encounter any other difficulties.Now to be more specific, I will compare what we have now to what I want. For what we have now, I use the code in PR #241.
Current Code
The counter code is...
...which lacks bindings while the recursive bindings are
Desired Code
Instead, I want the counter bindings to be defined with the rest of the Counter code like they are in the
SingleCounter
sample, which would look like......and then the recursive bindings would be defined something like this