Elmish.wpf: Merging of cleanup/re-implementation of Elmish.WPF

[cmeeren]

In my quest for recursive sub-model lists, I ended up rewriting most of Elmish.WPF from scratch, cleaning up some cruft and making names and types clearer (IMHO). I find the new implementation easier to grasp (says the author…), and I think you'll find there's quite a bit of attention to detail. In any case, I’d like to explain why and would like an open discussion on how we should proceed (use all/most of the changes vs. rewriting the features that are actually new into the existing implementation).

The new implementation is here. I haven’t made a PR because the diff would be pointless, but let me know if you want one anyway.

My implementation solves/supersedes #5, #38, #41, #42, #43, #44, and #45.

Disclaimer: The views expressed here are subjective. It is not my intention to step on any toes. But I don’t want to waste anyone’s time sugarcoating or beating around the bush, so I’ll call ’em like I see ‘em.

Why a re-implementation?

Simply put, I had trouble understanding the existing implementation. Not because there’s anything fundamentally wrong with it – it’s great, and mine is fundamentally similar. But all the type aliases confounded me, and it’s clear that the ViewModelBase, ViewBinding, ViewBindings, Variable, and PropertyAccessor types weren’t made with sub-model usage in mind, or lazy evaluation (which I also wanted).

So I did the only thing that made sense to me in order to understand the architecture: Using the existing implementation as a guide, I rewrote Elmish.WPF mostly from scratch, only complexifying as needed and in the direction that made most sense to me considering the final features I had in mind.

My rewrite turned out very similar, but types and functions relating to bindings and the view model is refactored quite a bit compared to the existing implementation.

Significant internal changes

• The binding union types have been rewritten from scratch and IMHO have a clearer function. BindingSpecData is generated by the Binding functions and contains everything the view model needs to set up a binding. Binding contains all data used by active bindings in the view model (including mutable types / ref cells where relevant).
• ViewModelBase (now called ViewModel) has been rewritten from scratch (but works in fundamentally the same way)
• More samples, and all samples are consistently designed and implemented.
• All projects use the new fsproj/csproj formats.
• All non-public stuff has been moved to an .Internal namespace.

Public breaking changes

Note that I don’t care about binary breaking changes here, only source breaking changes. These changes can of course be tweaked/reverted.

• twoWayValidation is called twoWayIfValid (because that’s what it is, and it clearly separates it from the new twoWayValidate)
• oneWayMap is called oneWayLazy (its implementation has changed, and the use case has expanded, but is similar)
• cmd and cmdIf have been renamed paramCmd and paramCmdIf (the old names exist but have new signatures / use-cases, see below)
• model has been renamed subModel because it’s more clear
• Program.runDebugWindow has been removed in favour of Program.runWithConfig (see below)

Public new stuff

• Improved documentation, particularly in the Binding module
• Binding.oneWayLazy replaces oneWayMap and is more efficient
• Edit: Binding.oneWaySeq for avoiding re-rendering whole lists etc. each time an item changes
• Binding.twoWayValidate is just like twoWay but with a separate validate function. The benefits of this is explained in the documentation and the Validation sample.
• Binding.cmd and Binding.cmdIf depend only on the model, which allows us to trigger CanExecuteChanged only when changed instead of for each update
• Binding.cmdIfValid allows better re-use and type safety, see the documentation for details
• Binding.paramCmd and Binding.paramCmdIf take an extra parameter to control whether to use CommandManager.RequerySuggested. In many cases (when not binding CommandParameter to another UI control) it’s not needed, and it triggers all. the. time., so this could be a good optimization.
• Binding.subModelSeq, perhaps the central new feature.
• Edit: Binding.subModelOpt, allowing a sub-model wrapped in option. This makes it easier to make illegal states unrepresentable in the model (though it's still limited by the fact that Elmish.WPF uses bindings instead of dynamically generated views). None models are surfaced as null to the XAML bindings.
• New configuration record type passed to new Program.runWithConfig function that allows extensible configuration (e.g. log to trace or console or both, in the future perhaps verbosity level and who knows what else).
• Contains the change from #42 (use existing Application instance if it exists)

Small changes

• More generous use of comments throughout
• No type aliases. IMHO, 'model -> obj is much more clear than Getter<'model>, and there is five of these (and more for new functions), so I constantly had to look them up.
• Code style is a bit cleaner IMHO – for example, no more 150 character lines, and I’ve been slightly more generous with whitespace. (The 2 space indentation can of course be changed to 4 spaces.)
• I may have made some other performance improvements I haven't mentioned

Things I didn’t copy over

• The try/with around the GetSetValidate setter. I don't see why it should be a good idea to just drop all exceptions.
• Cmd.ofAsyncCallback – I don’t know the use-case for this. Wouldn’t it be better to just implement it with try ... with inside the async, and couldn’t users easily do that themselves?
• Program.runWindowWith – I don’t see why we need a separate function just to pass an initial parameter to the init function, because you can just do that with partial application (or wrapping in a lambda) before calling Program.runWindow. IMHO that’s easier for consumers of the library, because it’s one less thing to learn. But I might have missed a valid use-case that requires this function.
• The Performance and DesignDataContext samples

Final notes

As said, I encourage you to take a careful look at the implementation.

I like the rewrite (well duh), and think it’s clearer for newcomers wanting to contribute to Elmish.WPF as well as making future development slightly easier due to the architecture being a bit more clear (the binding union types and ViewModel implementation). But I don’t want it hanging around on GitHub alongside Elmish.WPF doing exactly the same thing, potentially splitting the community.

How do we move forward from here? I see a couple of paths:

• You say “thank you, but no thanks” and I pout for a while and continue to use my implementation privately because, while it won’t benefit anyone else (aside from those stumbling over it on GitHub) it does suit my own needs better.
• You say “well, we really only want feature X and Y from this” and I may create PRs for just those, but no guarantees since half the reason I rewrote it in the first place was because I had trouble with the recursive submodel list stuff in the existing framework.
• You say ”this looks nice, let’s make some adjustments here and there and we’ll take it” and I’ll happily work with you on that.

Any input is most welcome.

[cmeeren]

By the way, I'm also considering adding Binding.oneWaySeq that's just like oneWay, but for sequences, and is backed by an ObservableCollection (which is updated just like subModelSeq), so that the entire list is not re-rendered/re-bound on each update (as AFAIK it would be with oneWay). Should be a simple addition.

[rchybicki]

Great work! Would love to see this incorporated to the repo, the breaking changes seem very reasonable and the added functionality is just great.

[JDiLenarda]

That looks great, I like your take on validations updating the model too. I will test it furtherly, expect some feedback.

[cmeeren]

Binding.oneWaySeq is now implemented. Check out the sample, the performance increase is drastic over Binding.oneWay.

[cmeeren]

I have now implemented Binding.subModelOpt, which allows a sub-model wrapped in option. This makes it easier to make illegal states unrepresentable in the model (though it's still limited by the fact that Elmish.WPF uses bindings instead of dynamically generated views). None models are surfaced as null to the XAML bindings. I'll make a sample sooner or later.

[et1975]

Sorry for the delay, none of us are available at the moment to take a good care of it. How would you like to take on the stewardship of this repo? I'll send you an invite into the elmish org where hopefully you can get right exposure and find collaborators to move it forward.

[cmeeren]

Part of me would like that, but my time too is limited, so I can't promise that I'll be able to make regular contributions. Let me think on that for a bit.

[et1975]

There's no "till death do us apart" clause, so you don't have anything to loose ;) Come join us!

[cmeeren]

Great, sure! 👍

[et1975]

Awesome! If you want to chat interactively we can be found either in fable gitter or in fsharp.org slack.

[cmeeren]

Closed by #48

[rchybicki]

A big, tasty beer for @cmeeren and a big thank you, will test in a production project very soon, so happy to switch to a fully immutable model from a Frankenstein immutable and VM collection one.

[cmeeren]

Thanks! Looking forward to that switch myself in a pet project of mine.

Note that there are some issues with the deployment; it seems only the symbol package got published (it contains everything the non-symbol package contains, so I think it'll work fine, but I'll investigate what went wrong.)