Elmish.WPF icon indicating copy to clipboard operation
Elmish.WPF copied to clipboard

v4 release scope

Open TysonMN opened this issue 5 years ago • 47 comments
trafficstars

Branch v4 contains these new features.

  • [x] PR #256 - Remove the wrapDispatch feature (which was proposed in #114 and implemented in #118) so that we can add the mapMsg feature instead (c.f. PR #244)
  • [x] PR #255 - Support logging to any sink (c.f. #105)
  • [x] PR #257 - Remove everything currently marked with the Obsolete attribute
  • [x] PR #258 - oneWaySeq binding implemented via oneWaySeqLazy with equals = refEq and map = id.
  • [x] PR #259 - Have F# "infer" equality type constraints for type parameters like id (c.f. #139)
  • [x] Add throttling, debouncing, and limiting (c.f. #271)
  • [ ] Catch and log exception from update (c.f. #265)
  • [ ] Composable binding API (c.f. #263)
  • [x] Static view models (c.f. #494)

Would anyone like to see any other changes? Does anyone have any concerns with this list of changes?

I will edit this comment to keep this list of items current (such as to add, check off, and strikethrough).

TysonMN avatar Aug 06 '20 12:08 TysonMN

  • [ ] Remove the obsolete Cmd.showWindow

cmeeren avatar Aug 07 '20 20:08 cmeeren

Added to my first comment as

[ ] Remove everything currently marked with the Obsolete attribute

TysonMN avatar Aug 09 '20 11:08 TysonMN

We have had the branch #143_#157_optimize_updateValue_using_refEq sitting around for 10 months. In contains a breaking change, but it wasn't very important, so we didn't do anything with it.

I suggest we include this change in v4. I rewrote and recommitted the changes from that branch in a new branch called #157_OneWaySeq_via_OneWaySeqLazy to make the changes easier to follow. I did not (yet) make changes to the tutorial or the release notes in that branch. I was planning on waiting for PR #255 to be completed first.

For reference, the issue discussing the change is #157 and PR #162 originally merged this change into the dormant branch.

Here is a self-contained explanation of the change. The oneWaySeq method changes from https://github.com/elmish/Elmish.WPF/blob/0ae868df7d590046899bd8b582bca174426481d2/src/Elmish.WPF/Binding.fs#L450-L461

to

static member oneWaySeq
    (get: 'model -> #seq<'a>,
     itemEquals: 'a -> 'a -> bool,
     getId: 'a -> 'id)
    : string -> Binding<'model, 'msg> =
  Binding.oneWaySeqLazy(get, refEq, id, itemEquals, getId)

As I said in https://github.com/elmish/Elmish.WPF/issues/157#issuecomment-543173555

Any way, here is how I see this change impacting different cases.

  1. If 'a is a mutable or if itemEquals or getId are impure, then the user might have to change their code to compensate for the proposed change.

  2. If 'a is immutable and 'itemEquals and getId are pure, then no change is required by the user. Behavior (in terms of correctness) is preserved. Furthermore,

    1. if 'a is "computed", then performance will be essentially unchanged, but
    2. if 'a is not "computed", then performance would be significantly better (directly related to how many elements are in 'a)

In response, you @cmeeren said in https://github.com/elmish/Elmish.WPF/issues/157#issuecomment-543281455

If what you are suggesting is simply replacing the current oneWaySeq with the code you posted above, I'm in. Feel free to create a PR. :)

It might be helpful to skim your other comments to remember what you thought back then.

TysonMN avatar Aug 11 '20 03:08 TysonMN

Thanks! Yes, let's do this for v4.

cmeeren avatar Aug 11 '20 05:08 cmeeren

I think the only other thing to consider for inclusion in version 4 is support for a lazy SubModelSeq binding. See issue #143 and especially the recent conversation starting in https://github.com/elmish/Elmish.WPF/issues/143#issuecomment-641532812. In reality, we can probably add this feature without a breaking change by copy-pasting the three methods that create this binding type and adding the functions needed to support laziness.

However, I would like to consider a more radical change to the SubModelSeq binding. I don't have this clear in my mind, so maybe the correct choice is to release v4 without waiting for this understanding to solidify. On the other hand, maybe we can release what we have so far as a v4 pre-release, which allows for more time.

Anyway, here is my idea about SubModelSeq. It was created to support a sequence representing a tree and when mapModel and mapMsgWithModel didn't exist. Now mapModel and mapMsgWithModel do exist and an issue exists about having an optimized binding specifically for trees (c.f. #236). Maybe we can simplify some things here.

I could say more (because I can always say more), but I will pause here for now. @cmeeren, what do you think about making a breaking change related to SubModelSeq in v4?

TysonMN avatar Aug 11 '20 20:08 TysonMN

@cmeeren, what do you think about making a breaking change related to SubModelSeq in v4?

Depends on the change, of course. The fact that it's breaking is not itself a problem, though. So please go on. :)

cmeeren avatar Aug 11 '20 20:08 cmeeren

(Different train of thought)

I want to try and add the requires equality type constraint to 'id for v4 as described in https://github.com/elmish/Elmish.WPF/issues/139#issuecomment-672815011. I think we could just "manually" add this type constraint, but that is "cheating". I want it exposed through the Binding API because it is inferred by F#.

TysonMN avatar Aug 12 '20 11:08 TysonMN

(Another different train of thought)

When update throws an exception in Elmish 3.x, only the exception and message are logged. I want the model logged as well. In elmish/master, which is the release branch for v4, I don't think the exception is caught. It appears that the exception from update causes the Elmish dispatch loop to stop.

(After investigating this further...) I suggest we wrap update in try-with, log everything (the exception, message, and model), and then return the given model to

  • v3: prevent two log events for the same exception
  • v4: keep the Elmish dispatch loop running.

TysonMN avatar Aug 16 '20 19:08 TysonMN

Are you sure that the loop stops in the v4 branch? I thought Elmish made sure of continuing the loop, while calling the error handler that we have added in v4:

https://github.com/elmish/Elmish.WPF/blob/78ae2c137de3fb17c74387f857cbe78c98da87ad/src/Elmish.WPF/WpfProgram.fs#L80

If you have tested and it does indeed stop, then yes, we should probably do as you say.

On a related note, we should definitely give the user a say in what happens when update throws. We can for example have an withUpdateExnHandler that is just exn -> unit, an/or withUpdateExnMsg with type exn -> 'msg that lets the user choose a message we dispatch when update throws.

cmeeren avatar Aug 17 '20 07:08 cmeeren

I thought Elmish made sure of continuing the loop, while calling the error handler...

That is the behavior in Elmish 3.x. In Elmish 4.x, I just tested that the program crashes. I asked about this in https://github.com/elmish/elmish/issues/194#issuecomment-675092793.

TysonMN avatar Aug 17 '20 20:08 TysonMN

Should we wait until Elmish 4 stable comes out before releasing v4? Do you know the timeframe for that?

cmeeren avatar Aug 17 '20 20:08 cmeeren

I don't know their timeframe.

I think it would be good to wait for technical reasons (so we don't need to make another major release to support their major release) and for the current version symmetry (we are both on v3 now and both moving to v4 "soon"). However, I don't want to delay publishing a new NuGet package with the new features just because they are delaying.

I would be satisfied if we published a NuGet package from our our v4 branch at various points as a pre-release. Something like Elmish.WPF 4.0-beta.1.

TysonMN avatar Aug 17 '20 21:08 TysonMN

@et1975 do you have a timeframe for final Elmish v4 release?

cmeeren avatar Aug 17 '20 21:08 cmeeren

Sorry, no time frame - I've been experimenting with Elm-like subscriptions, but got nothing definitive yet. Also I would not recommend coupling the Elmish.WPF major version component to the core elmish, all other packages in the ecosystem are bound only by semver guidelines, not which version of core they are built against.

et1975 avatar Aug 18 '20 12:08 et1975

@cmeeren, what do you think about making a breaking change related to SubModelSeq in v4?

Depends on the change, of course. The fact that it's breaking is not itself a problem, though. So please go on. :)

When I wrote that, the suggestion I was considering was to related to SubModelSeq being too complex. Its toMsg function has signature 'id * 'subMsg -> 'msg but the SubModelSeq sample just passes in snd. I would prefer that each binding is used to its full expression in at least one sample. This is not the case for SubModelSeq binding because of the toMsg arguemnt.

However, #266 is improving this situation. It uses a different value for toMsg than snd.

In fact, I now think I would like to make toMsg more expressive by adding 'bindingModel as an argument as described in Solution 2 in https://github.com/elmish/Elmish.WPF/pull/266#discussion_r472302177.

I would also like the function arguments in all the Binding methods to be fully curried (as I mentioned in https://github.com/elmish/Elmish.WPF/pull/266#discussion_r472307305). I think think of two advantages for pairing some inputs. The first is because passing snd is easier than passing fun _ a -> a. The second is because DU case constructors are uncurried functions. However, I think the extra verbosity in those cases would make things more readable. We could deprecate the overloads with uncurried function arguments.

TysonMN avatar Aug 18 '20 17:08 TysonMN

Sorry, no time frame - I've been experimenting with Elm-like subscriptions, but got nothing definitive yet.

Gotcha. We'll release v4 when we're ready, and if updating to Elmish 4.0 causes breaking changes, we'll release v5 at that time.

Also I would not recommend coupling the Elmish.WPF major version component to the core elmish, all other packages in the ecosystem are bound only by semver guidelines, not which version of core they are built against.

That was not the intention; the version match in this specific case is purely coincidental. Semver guidelines is certainly the way to go and the way we're currently doing it.

In fact, I now think I would like to make toMsg more expressive by adding 'bindingModel as an argument as described in Solution 2 in #266 (comment).

I'll get back to you when I have read your comment. Might be a day or two until I can find the time.

I would also like the function arguments in all the Binding methods to be fully curried (as I mentioned in #266 (comment)). I think think of two advantages for pairing some inputs. The first is because passing snd is easier than passing fun _ a -> a. The second is because DU case constructors are uncurried functions. However, I think the extra verbosity in those cases would make things more readable. We could deprecate the overloads with uncurried function arguments.

I don't understand. Do you have an example? As you just said, passing toMsg=snd is no problem since it's how it's currently done in the SubModelSeq sample.

cmeeren avatar Aug 18 '20 19:08 cmeeren

I would also like the function arguments in all the Binding methods to be fully curried (as I mentioned in #266 (comment)). I think think of two advantages for pairing some inputs. The first is because passing snd is easier than passing fun _ a -> a. The second is because DU case constructors are uncurried functions. However, I think the extra verbosity in those cases would make things more readable. We could deprecate the overloads with uncurried function arguments.

I don't understand. Do you have an example? As you just said, passing toMsg=snd is no problem since it's how it's currently done in the SubModelSeq sample.

Yes, sorry. I should have given an example from the start.

For example, in this function... https://github.com/elmish/Elmish.WPF/blob/9222c08ff30682719969a241ee063acab5afd923/src/Elmish.WPF/Binding.fs#L1740-L1746

...I am suggesting that

toBindingModel: 'model * 'subModel -> 'bindingModel

...be replaced with...

toBindingModel: 'model -> 'subModel -> 'bindingModel

...and...

toMsg: 'id * 'bindingMsg -> 'msg

...be replaced with

toMsg: 'id -> 'bindingMsg -> 'msg

("Replaced" here actually means (1) copy the method, (2) paste the method, (3) make the change in the pasted method, (4) mark the original method as obsolete.) ...

TysonMN avatar Aug 18 '20 19:08 TysonMN

Thanks, I see. The reason for toMsg being the way it is, is because typically the DU constructor case for the child message has that signature, as you say. I do not agree that

the extra verbosity in those cases would make things more readable.

The whole point is that you can just pass the DU case. It's less noise. If you have an example that could convince me otherwise, feel free to share it.

Regarding toBindingModel, I can't remember why it was like that in the first place. Perhaps it was just some internal design that leaked out into the signature without my noticing. Do you have an example where currying would improve it?

cmeeren avatar Aug 18 '20 19:08 cmeeren

...The second is because DU case constructors are uncurried functions....

The reason for toMsg being the way it is, is because typically the DU constructor case for the child message has that signature, as you say. I do not agree that

the extra verbosity in those cases would make things more readable.

The whole point is that you can just pass the DU case. It's less noise. If you have an example that could convince me otherwise, feel free to share it.

Ah! I was so trapped in in the debugging cycle this weekend that I didn't realize I could do that right now. Check out this commit that I just pushed to PR #266. Now I like the uncurried signature :D

TysonMN avatar Aug 18 '20 20:08 TysonMN

Regarding toBindingModel, I can't remember why it was like that in the first place. Perhaps it was just some internal design that leaked out into the signature without my noticing,

I will review the internal design.

Do you have an example where currying would improve it?

If there is no advantage to the uncurried form, then the curried form is better because fun a b -> ... is easier to read than fun (a, b) -> .... I feel like the burden of proof should be on the side of uncurried forms justifying their existence.

TysonMN avatar Aug 18 '20 20:08 TysonMN

Regarding toBindingModel, I can't remember why it was like that in the first place. Perhaps it was just some internal design that leaked out into the signature without my noticing,

I will review the internal design.

You added toBindingModel with paired input to four binding methods in the same commit in these four places.

TysonMN avatar Aug 19 '20 05:08 TysonMN

Oh, I remember now. For SubModel bindings, as shown in the SubModel, SubModelOpt, and SubModelWin samples, you may typically want the binding model to be one of:

  • a tuple containing the current (main) model and the sub-model (pass id)
  • the sub-model (pass snd)
  • the main model (pass fst)

These examples become more verbose if you curry that argument. If you have compelling counter-arguments, freel free to share.

cmeeren avatar Aug 19 '20 06:08 cmeeren

Thanks for pointing out those examples. I suppose I could have found them if I had looked harder. I guess I am fine with the status quo.

TysonMN avatar Aug 19 '20 12:08 TysonMN

I would be satisfied if we published a NuGet package from our our v4 branch at various points as a pre-release. Something like Elmish.WPF 4.0-beta.1.

It would be useful to me if we published a prerelease before next week. Is it easy to do this with the exiting devops?

TysonMN avatar Aug 19 '20 13:08 TysonMN

All branches are built, so as long as you tag a commit on the v4 branch, I think it'll work.

Prerelease version format on NuGet (and in the fsproj) is 4.0.0-alpha-1. If you think there's a chance there'll be more than 9 alphas (or betas), use two digits after alpha/beta, because NuGet sorts prereleases alphabetically and doesn't know that 4.0.0-alpha-10 is newer than 4.0.0-alpha-2.

Any strong opinions on alpha vs. beta?

cmeeren avatar Aug 19 '20 19:08 cmeeren

Any strong opinions on alpha vs. beta?

None at all.

Prerelease version format on NuGet (and in the fsproj) is 4.0.0-alpha-1. If you think there's a chance there'll be more than 9 alphas (or betas), use two digits after alpha/beta, because NuGet sorts prereleases alphabetically and doesn't know that 4.0.0-alpha-10 is newer than 4.0.0-alpha-2.

One digit is fine. We don't need 10 prereleases.

TysonMN avatar Aug 19 '20 19:08 TysonMN

Great! I suggest alpha for now. Feel free to do alpha releases from v4 when you want.

Note that since there's so many changes, it's fine if the ReleaseNotes in the fsproj just contains a link to the v4 RELEASE_NOTES.md. I haven't been able to get release notes to show up on NuGet.org or in VS anyway. (Am I missing something?)

cmeeren avatar Aug 20 '20 05:08 cmeeren

Almost worked.

Quoting the build logs when I pushed the tag.

"NuGet" deployment has been skipped as environment variable has not matched ("appveyor_repo_tag" is "false", should be "true")

TysonMN avatar Aug 20 '20 15:08 TysonMN

Actually, I don't need this on nuget.org. I just remembered that we have a folder as a NuGet package source for some other NuGet packages for conveniences just like this. I will do that instead.

TysonMN avatar Aug 20 '20 15:08 TysonMN

Oh, now I see that the tag I created only exists locally. That explains it. I will still go with the local approach unless someone else requests a prerelease.

TysonMN avatar Aug 20 '20 15:08 TysonMN