Skip to content
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

v4 release scope #253

Open
7 of 9 tasks
TysonMN opened this issue Aug 6, 2020 · 47 comments · Fixed by #337
Open
7 of 9 tasks

v4 release scope #253

TysonMN opened this issue Aug 6, 2020 · 47 comments · Fixed by #337
Milestone

Comments

@TysonMN
Copy link
Member

TysonMN commented Aug 6, 2020

Branch v4 contains these new features.

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).

@cmeeren

This comment has been minimized.

@TysonMN

This comment has been minimized.

@TysonMN

This comment has been minimized.

@cmeeren

This comment has been minimized.

@TysonMN
Copy link
Member Author

TysonMN commented Aug 11, 2020

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 #143 (comment). 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?

@cmeeren
Copy link
Member

cmeeren commented Aug 11, 2020

@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. :)

@TysonMN

This comment has been minimized.

@TysonMN TysonMN pinned this issue Aug 15, 2020
@TysonMN

This comment has been minimized.

@cmeeren

This comment has been minimized.

@TysonMN

This comment has been minimized.

@cmeeren
Copy link
Member

cmeeren commented Aug 17, 2020

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

@TysonMN
Copy link
Member Author

TysonMN commented Aug 17, 2020

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.

@cmeeren
Copy link
Member

cmeeren commented Aug 17, 2020

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

@et1975
Copy link
Member

et1975 commented Aug 18, 2020

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.

@TysonMN
Copy link
Member Author

TysonMN commented Aug 18, 2020

@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 #266 (comment).

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.

@cmeeren
Copy link
Member

cmeeren commented Aug 18, 2020

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.

@TysonMN
Copy link
Member Author

TysonMN commented Aug 18, 2020

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...

static member subModelSeq
(getSubModels: 'model -> #seq<'subModel>,
toBindingModel: 'model * 'subModel -> 'bindingModel,
getId: 'bindingModel -> 'id,
toMsg: 'id * 'bindingMsg -> 'msg,
bindings: unit -> Binding<'bindingModel, 'bindingMsg> list)
: string -> Binding<'model, 'msg> =

...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.)
...

@cmeeren
Copy link
Member

cmeeren commented Aug 18, 2020

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?

@TysonMN

This comment has been minimized.

@TysonMN

This comment has been minimized.

@TysonMN

This comment has been minimized.

@cmeeren

This comment has been minimized.

@TysonMN

This comment has been minimized.

@TysonMN

This comment has been minimized.

@TysonMN

This comment has been minimized.

@cmeeren

This comment has been minimized.

@TysonMN

This comment has been minimized.

@TysonMN

This comment has been minimized.

@TysonMN

This comment has been minimized.

@cmeeren

This comment has been minimized.

@cmeeren
Copy link
Member

cmeeren commented Aug 23, 2020

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 have now read your comments. I do not see the full implications of making this change. Unfortunately I can't prioritize digging into this at the moment, nor do I know when I can. But at least it's backwards compatible. If you feel fairly sure that this will not drive users away from the pit of success (also here), feel free to go ahead and work on this. Also, is 'bindingModel the most general/flexible thing to pass, or is there a story for passing the 'model, too?

@BentTranberg
Copy link

Just a thought, for whatever it's worth: It's possible to mix curried style and tupled style.

@TysonMN
Copy link
Member Author

TysonMN commented Sep 29, 2020

FYI: I learned about profunctors from this tweet, so I modified the tutorial to mentioned this concept.

@TysonMN
Copy link
Member Author

TysonMN commented Sep 29, 2020

FYI: I modified the release notes to match the new behavior introduced by PR #272, which replaced mapMsgWithModel with just mapMsg.

Since we are no longer using mapMsgWithModel, I think it would be reasonable to remove this feature. The easiest way to do this is to make this function internal in the Binding and Bindings modules.

I plan to read and learn more about profunctors. As I do, I expect that this might give me more ideas for how to design the binding API. I don't want to make the same mistake I did with the wrapDispatch feature and expose a "bad" API.

@TysonMN
Copy link
Member Author

TysonMN commented Feb 10, 2021

I would like to release version 4 with the currently implemented features.

Features like mapModel/mapMsg and the ability to log to any sink are significant improvements. On the other side, achieving the "entire" composable binding API is a lot of work, and I don't see the end yet either. I think I will be able to find more motivation to work on it if I can see things more quickly merged into master. I also expect much of the composable binding API can be added without further breaking changes.

The next step in this direction is to merge master into v4. However, those sample renames we did recently are making that difficult. I am not sure of the correct way to resolve those conflicts.

@cmeeren
Copy link
Member

cmeeren commented Feb 10, 2021

I would like to release version 4 with the currently implemented features.

👍

The next step in this direction is to merge master into v4. However, those sample renames we did recently are making that difficult. I am not sure of the correct way to resolve those conflicts.

Neither am I. The easiest way to merge may be to take the versions from master, and manually apply any changes to the samples from v4.

@TysonMN
Copy link
Member Author

TysonMN commented Feb 10, 2021

I did a "manual" rebase of master onto v4 by cherry picking commits. The only downside is that commits that had conflicts are now shown as authored by me. I don't think we care about that.

I pushed v4. All the tests pass. I tested all the samples, and everything works. I have been using some previous version of v4 at work (by committing the NuGet package into your repo and referencing the containing folder as a local package source). I have not had any problem. Quite the opposite of course. I love the ability to map bindings and log to Seq.

Now I suggest we merge v4 into master, change the version to something like 4.0.0-beta-1, and do a release.

At this point, I no longer want to maintain two branches: i.e. no more changes to 3.x. By only having one branch again, I think my motivation to make progress will return.

We had some discussions in the past about how exactly the mapping API would look. (Some of that discussion might be in #295, but it is long, and I didn't reread it.) By creating a prerelease, we can still change the API before releasing 4.0.0. One thing we had considered is making mapMsgWithModel internal since none of the samples use it. However, I think we should keep it public, especially because I am using in my program at work.

Normally the intention of such a prerelease version is to do a feature freeze while finding and fixing bugs. Although some bug might exist, that is not my concern. This project is the most robust codebase (for its size) I have ever seen. In our case, the purpose of this prerelease is more for you (@cmeeren) and others to try the new (mapping) API, which is much more subjective.

Because I am not concerned about bugs, I don't mind continuing to add features into master while incrementing the prerelease version number. I also don't mind waiting until after 4.0.0 exists to make more code changes.

What do you think @cmeeren?

@cmeeren
Copy link
Member

cmeeren commented Feb 10, 2021

The only downside is that commits that had conflicts are now shown as authored by me. I don't think we care about that.

I don't.

Now I suggest we merge v4 into master, change the version to something like 4.0.0-beta-1, and do a release.

Sounds fine.

By only having one branch again, I think my motivation to make progress will return.

Nothing better! 🥳 I had no idea this was a pain point for you.

One thing we had considered is making mapMsgWithModel internal since none of the samples use it. However, I think we should keep it public, especially because I am using in my program at work.

This is fine. I have forgotten the discussion(s) on whether or not mapMsgWithModel should be public or internal, and I don't care enough to prioritize re-reading it. It would, at least at some point, be nice to have a sample that demonstrates a use-case where it is the best (or only) option.

In our case, the purpose of this prerelease is more for you (@cmeeren) and others to try the new (mapping) API, which is much more subjective.

👍

Because I am not concerned about bugs, I don't mind continuing to add features into master while incrementing the prerelease version number.

This sounds good to me!

Are you able and willing to make a changelog for this release? And briefly describe any relevant migrations?

@TysonMN
Copy link
Member Author

TysonMN commented Feb 10, 2021

I just pushed this potential release to commit to v4. I think the release notes are already up to date. I added one line about equality type constraints.

If you think we should elaborate more about the breaking changes and migrations, then I suggest we use create a "new release" using GitHub. Here is an example.

@cmeeren
Copy link
Member

cmeeren commented Feb 10, 2021

I left a comment: 0b1603a#r46988514

Other than that, I think this is sufficient.

@TysonMN TysonMN mentioned this issue Feb 11, 2021
Merged
@TysonMN

This comment was marked as outdated.

@TysonMN TysonMN reopened this Aug 26, 2021
@TysonMN
Copy link
Member Author

TysonMN commented Aug 29, 2021

I updated the initial comment in this issue. I think it is now rather accurate.

@marner2

This comment was marked as outdated.

@marner2
Copy link
Collaborator

marner2 commented Jun 27, 2022

@TysonMN I made an issue that should be linked on the top instead of #452 for static view models: #494

@marner2
Copy link
Collaborator

marner2 commented May 2, 2023

I merged in #560 which should mark a feature-complete version of the Static View Models feature. It needs mention in the docs yet, however.

@marner2 marner2 added this to the 4.0 milestone Nov 7, 2023
@marner2
Copy link
Collaborator

marner2 commented Nov 7, 2023

I'm proposing making a release in #585 with the current state of the beta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants