Elmish.wpf: References to Application.Current

[TysonMN]

This is the issue I promised in https://github.com/elmish/Elmish.WPF/pull/201#issue-402463416 that I would create.

Application.Current in
https://github.com/elmish/Elmish.WPF/blob/3f0d3552203dcf3152f1088014ee608f2926f8a9/src/Elmish.WPF/ViewModel.fs#L199
was null when processing an initial state as described in #200. Indeed, this property is static with a private setter. The fix in PR #201 implies to me that the constructor of Application sets that static property (probably like Current = this). So that PR is a bug fix, but I also believe it does not satisfy the principle of least astonishment. I mean, look at how
https://github.com/elmish/Elmish.WPF/blob/3f0d3552203dcf3152f1088014ee608f2926f8a9/src/Elmish.WPF/Program.fs#L36-L40
creates an Application instance and then throws it away. Very unexpected if you ask me.

Instead, I would prefer if we accessed the current Application instance in a way that I could statically verify in the Elmish.WPF code. There is more than one way to achieve this. Here is the way that I think is best.

We could implement the F#-equivalent of a static constructor to initialize a static let-bound Application instance like
F# type CurrentApplication() let value = let current = Application.Current if isNull current then Application () else current

Then we replace all references to Application.Current with CurrentApplication.value. As always, naming is impossibly hard, so we could certainly go with better names.

[cmeeren]

Very unexpected if you ask me.

I disagree; I don't find it unexpected in a WPF world. You have to instantiate Application once when starting your app (unless it's done implicitly in XAML), and then you access it using Application.Current because it's a singleton in WPF.

Personally I prefer the bug fix in #201. Accessing Application.Current in Elmish.WPF code must be assumed to be safe, because Application must be assumed to be instantiated. Anything else is a bug. The CurrentApplication approach you sketch out above will cause any part of Elmish.WPF code currently accessing Application.Current to instantiate Application if not already done. That should never be necessary; it only happens if we have a bug (like in #200). I'd rather have the runtime throw exceptions when that happens, so we immediately see it and can fix it.

It's similar to a non-Elmish WPF app, where you wouldn't implement a CurrentApplication like this; you'd assume that everywhere Application.Current is used, it's safe because Application would already have been instantiated.

(As a side note, in case it's relevant, keep in mind that the user must be allowed to instantiate Application themselves before calling Elmish.WPF, because it might be needed for application-wide resource dictionaries.)

[BentTranberg]

Not just for resources. I instantiate Application in order to hook up outer exception handlers, so that I can log exceptions, and generally handle them the way I prefer. I also need to initialize DevExpress themes using Application, and that won't work unless I do it before Elmish.WPF fires up. I've also had other needs for Application that I don't remember at the moment.

I fully agree with your reasoning, @cmeeren. Singletons may be regarded a bad thing these days, but when WPF does it like that, don't complicate things further. I know exactly what the current source says, but the suggested change would make me wonder what purpose it serves.

[TysonMN]

@cmeeren, your position is certainly reasonable, which is partly why I went with the fix I did in PR #201.

...you access it using Application.Current because it's a singleton in WPF.

Ah, of course. This behavior never crystalized in my mind using the word singleton. I verified this also by executing Application () twice. The second time causes the exception

System.InvalidOperationException: 'Cannot create more than one System.Windows.Application instance in the same AppDomain.'

So, I agree with you now. The current behavior seems clear enough now.

P.S.

Quoting @cmeeren...

(As a side note, in case it's relevant, keep in mind that the user must be allowed to instantiate Application themselves before calling Elmish.WPF, because it might be needed for application-wide resource dictionaries.)

...and @BentTranberg...

Not just for resources. I instantiate Application in order to hook up outer exception handlers, so that I can log exceptions, and generally handle them the way I prefer. I also need to initialize DevExpress themes using Application, and that won't work unless I do it before Elmish.WPF fires up. I've also had other needs for Application that I don't remember at the moment.

To be clear, the approach I was suggesting still allowed for that functionality.

[BentTranberg]

To be clear, the approach I was suggesting still allowed for that functionality.

Absolutely.

I was thinking more along the lines that someone might get the idea at some point in the future to e.g. add a way to handle resources indirectly, while blocking the opportunity to create Application before Elmish.WPF starts. Which is why I wanted to make it clear that Application is needed for several reasons that I dare say can't be easily predicted.

[TysonMN]

As Scott Wlaschin explains in the link I gave above,

...you can create static let-bound values and static do-blocks that are executed when the class is first used.

The static class CurrentApplication I suggested would be used for the first time after the user has passed the flow of control to Elmish.WPF.

[JDiLenarda]

I don’t know if this will help or if it’s even relevant, but I’d like to bring this just in case. I write some AutoCAD palette plugin with Elmish.WPF and in this environment, the call to Application doesn’t work (I don’t remind if it returns null or throw an exception). This is the reason why back then I made a PR implementing startElmishLoop enabling the use of FrameworkElement instead of just Window, and replaced every instance of Application.Current.Dispatcher with Dispatcher from the current FrameworkElement in ViewModel module.

Currently, I cannot use windows submodel in such a plugin. Since it has not been big deal for now, I didn’t investigate the reason or opened issues, but I strongly suspect it’s because of Application.Current.Dispatcher calls. To my mind, such uses should be replaced by the Dispatcher of the main FrameworkElement.

I don’t ask for a correction for now, I just give this as food for thoughts, hope that helps.

[TysonMN]

Thanks for sharing @JDiLenarda.

While investigating this bug, the thought about which Dispatcher instance to use crossed my mind, but I don't know the difference between using one vs another.

By my static analysis, we are currently using Dispatcher and/or its Invoke method three times.

1) The Window, as a FrameworkElement, given by the user when passing the flow of control to Elmish.WPF:
https://github.com/elmish/Elmish.WPF/blob/b62f4ed28f806847d6d8521216886e82fb64f6fd/src/Elmish.WPF/Program.fs#L28

2) Application.Current.Dispatcher when opening a new window, which caused the NullReferenceException in issue #200:
https://github.com/elmish/Elmish.WPF/blob/b62f4ed28f806847d6d8521216886e82fb64f6fd/src/Elmish.WPF/ViewModel.fs#L199

3) Application.Current.Dispatcher is some deprecated code (which I think was replaced by the code in case 2 above):
https://github.com/elmish/Elmish.WPF/blob/b62f4ed28f806847d6d8521216886e82fb64f6fd/src/Elmish.WPF/Cmd.fs#L15

[cmeeren]

some deprecated code (which I think was replaced by the code in case 2 above)

Correct.

[JDiLenarda]

Thanks for sharing @JDiLenarda.

While investigating this bug, the thought about which Dispatcher instance to use crossed my mind, but I don't know the difference between using one vs another.

My guess is they’re the same, at least as long the program is launched in a STAThread.

https://stackoverflow.com/questions/20025143/application-current-dispatcher-vs-dependencyobject-dispatcher-for-ui-thread-acce

http://dotnetpattern.com/wpf-dispatcher

[TysonMN]

For those following this issue, I created PR #203 based on https://github.com/elmish/Elmish.WPF/issues/202#issuecomment-613527232 by @JDiLenarda