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

RFC: Overload-based binding syntax #87

Closed
cmeeren opened this issue May 8, 2019 · 12 comments · Fixed by #89
Closed

RFC: Overload-based binding syntax #87

cmeeren opened this issue May 8, 2019 · 12 comments · Fixed by #89

Comments

@cmeeren
Copy link
Member

cmeeren commented May 8, 2019

Currently there's quite a bit of friction in implementing alternative/simpler versions of existing bindings, because it requires new functions with unique names. This can quickly lead to a mess of functions and confuse users (and get unnecessarily verbose).

For example, based in the changes described in #86, there's subModel, subModelOpt, and subModelOptSticky. These functions have also become more complex (in order to become more powerful/general), and it might be nice to have simpler variants of them. But I don't want to have 6 or 12 different subModel bindings just for convenience. So if we stick with functions, I'm not that likely to add many such conveniences.

An alternative is an overload-based syntax, in the form of static methods on a Binding class. To keep the syntax as similar as possible to the existing one, I can imagine all the overloads return string -> Binding<_,_>. That way one could do something like this (not an exhaustive list, but also includes several combinations not currently supported):

let bindings () = [
  "OneWay" |> Binding.oneWay(getValue)
  "OneWayOpt" |> Binding.oneWay(getValueOpt)
  "OneWayLazy" |> Binding.oneWay(getValue, equals, map)
  "OneWayLazyOpt" |> Binding.oneWay(getValueOpt, equals, map)
  "TwoWay" |> Binding.twoWay(getValue, setValue)
  "TwoWayOpt" |> Binding.twoWay(getValueOpt, setValueOpt)
  "TwoWayValidate" |> Binding.twoWay(getValue, setValue, validate)
  "Cmd" |> Binding.cmd(getMsg)
  "CmdIf" |> Binding.cmd(getMsg, getCanExecute)
  "CmdIfValid" |> Binding.cmd(getMsgResult)
  "CmdIfSome" |> Binding.cmd(getMsgOpt)
  "ParamCmd" |> Binding.paramCmd(getMsg[, uiTrigger = true|false])
  "ParamCmdIf" |> Binding.paramCmd(getMsg, getCanExecute[, uiTrigger = true|false])
  "ParamCmdIfValid" |> Binding.paramCmd(getMsgResult[, uiTrigger = true|false])
  "ParamCmdIfSome" |> Binding.paramCmd(getMsgOpt[, uiTrigger = true|false])
  "SubModel" |> Binding.subModel(getSub, toBindingModel, toMsg, getBindings)
  "SubModelOpt" |> Binding.subModel(getSubOpt, toBindingModel, toMsg, getBindings[, sticky=true|false])
  "SubModelOpt" |> Binding.subModel(getSubOpt, toBindingModel, toMsg, getBindings[, sticky=true|false])
  "SubBindings" |> Binding.subModel(getSub, getBindings)
]

The drawbacks I see are:

  • It's a breaking change
  • Error messages relating to overload mismatches are less helpful
  • It requires an extra pair of parentheses 😅

The benefits I see are:

  • Conceptually simpler; pick the method based on the binding you want, then find the overload that gives you the desired variant
  • Some (uncommon?) parameters can be optional, like uiTrigger, and won't pollute the function list like now
  • The bar is much lower for adding convenience overloads with some parameters omitted (set to common defaults)

Personally I like this, but I'd like to hear what others think before delving into this. Even just a positive reaction emoji is helpful to let me know that you agree. (If you disagree, please explain in a comment! :) )

@cmeeren
Copy link
Member Author

cmeeren commented May 26, 2019

Changes pushed to wip-3.x branch. Will be released in a few days.

@cmeeren cmeeren mentioned this issue May 28, 2019
Merged
@cmeeren
Copy link
Member Author

cmeeren commented May 28, 2019

Released in 3.0.0. See the changelog for details.

@TysonMN
Copy link
Member

TysonMN commented Sep 6, 2019

I think I found another (minor) drawback.

When the 'model and 'msg types are in different submodules, then type inference for the model argument in a call to Bindings.twoWay doesn't work. Workarounds I found were to

  1. provide a type annotation,
  2. open the other submodule, or
  3. call a unique alias of the desired overload of Binding.twoWay.

I created a temporary PR in my fork of Elmish.WPF to clearly demonstrate the issue.

Among those three solutions, I prefer adding the type annotation. The main reason is that it is the most local solution (i.e. the scope of the additional code needed is as small as possible and no other code is adversely affected).

If #F ever gets shorthand function accessors to record properties, then it might become possible to directly refer to the sufficiently qualified property accessor function, which is essentially what I am doing elsewhere in my example (such as for the Msg case constructor functions). That syntax might make the line in question look like

MainModule.Model_.StepSize >> float,

@cmeeren
Copy link
Member Author

cmeeren commented Sep 6, 2019

Thanks for the heads-up! F#'s overload resolution certainly isn't perfect.

Personally I prefer opening modules in those situations. The module in question, at least the way I structure my code (and as shown in your PR), mostly contains stuff relevant for the bindings anyway (the Msg and Model types). I don't mind getting "access" to the update and update helper functions - it's very, very unlikely to cause a bug in my app.

If you don't want that, you could simply place the types (Model and Msg) in a separate module that is opened in both Bindings and, say, Update where you have your update function and any helpers for it.

@TysonMN
Copy link
Member

TysonMN commented Sep 6, 2019

If I remember correctly, here is my experience when opening submodules.

Consider the SubModel sample. If we opened the submodules Clock and CounterWithClock, then I would expect issues when referring to Model since both submodules contain a type by this name. I think F# assigns Model to the type in exactly one of those submodules (I think it is the module that was opened last). Then when trying to refer to the "other" Model, the text color will turn green telling the developer that a type by that name was found but a more complicated and confusing error message will occur for a larger block of code that uses this type (because it is not the intended type). At this point, one way to fix the compile error is to add a type annotation, which we trying to avoid by instead opening the submodules. In the end, only the (overloaded) types in one submodule don't need type annotations.

I think that is worse than just adding type annotation in the first place.

Are you able to avoid that situation when you open the submodules?

@cmeeren
Copy link
Member Author

cmeeren commented Sep 6, 2019

I avoid that situation by only opening a single module with one Model and Msg type when creating bindings:

module A

  type Model
  type Msg
  let update

module B

  type Model
  type Msg
  let update

module ABindings

  open A
  let bindings

module BBindings

  open B
  let bindings

I have never needed to open modules for multiple models when creating bindings, because the bindings I have created have always been specific to one model.

@cmeeren
Copy link
Member Author

cmeeren commented Sep 6, 2019

That said; sometimes type annotations are simply required with F#. The type inference has some quirks, particularly when combined with overloads. I haven't come across the problems you mention myself due to how I have structured my code. There may of course be valid use-cases that require another structure that brings about these type problems, but I don't know at the moment what use-cases those would be.

@TysonMN
Copy link
Member

TysonMN commented Sep 6, 2019

Oh, fantastic! I had seen opens only within submodules, but I have forgotten about that possibility. I am sure that will work very well. Thanks :)

@cmeeren
Copy link
Member Author

cmeeren commented Sep 6, 2019

Yeah, you can pretty much open anything anywhere :)

@TysonMN
Copy link
Member

TysonMN commented Sep 12, 2019

Currently there's quite a bit of friction in implementing alternative/simpler versions of existing bindings, because it requires new functions with unique names.
...
An alternative is an overload-based syntax, in the form of static methods on a Binding.

Another alternative is all unique function names (for example, in a one-to-one correspondence with the internal type BindingData<'model, 'msg>) that each accept some case in their own discriminated union. Mark Seemann discusses this in a blog post.

Here is the comparison I see with the current overload-based syntax.

Drawbacks

  1. (Another) breaking change
  2. It requires naming the chosen case of the discriminated union
  3. Each case requires a unique name, and even without optional/default arguments, the list of cases could be rather long

Benefits

  1. No unhelpful error messages relating to overload mismatches

Roughly the same

  1. Conceptually simple; pick the function based on the binding you want, then find the discriminated union case that gives you the desired variant

Another idea that could be combined with the previous one is passing optional arguments with the same fluent-like syntax that Emlish uses with its Program type. Then optional arguments could be initially be given default values and switched to a non-default with a fluent syntax. I think that makes the list of DU cases simper but makes discovery the full set of possibilities more difficult.

I am willing to "argue" for this in future comments, but the main reason that I am writing this comment is to get this idea out of my head.

@cmeeren
Copy link
Member Author

cmeeren commented Sep 13, 2019

Another alternative is all unique function names (for example, in a one-to-one correspondence with the internal type BindingData<'model, 'msg>) that each accept some case in their own discriminated union. Mark Seemann discusses this in a blog post.

Interestingly, that is one of the few posts of Mark's that I don't agree with. I don't find the syntax compelling, nor particularly discoverable without qualifying the union type name. Also it's more verbose than overloaded static methods.

(Since it's an early F# blog post of his, I am curious if he still holds the same view. I can't recall seeing that syntax much in later blog posts.)

Another idea that could be combined with the previous one is passing optional arguments with the same fluent-like syntax that Emlish uses with its Program type. Then optional arguments could be initially be given default values and switched to a non-default with a fluent syntax. I think that makes the list of DU cases simper but makes discovery the full set of possibilities more difficult.

Certainly an option, though many bindings would require several parameters immediately (on the first call) anyway, and there are several ways you could specify them (option vs. voption, 'model -> 'msg vs. 'msg, etc.). So I don't think it solves the problem.


In general, I find that the current overloaded syntax, while not perfect, is the most pleasant and concise I variant I have considered/tried. I do appreciate that you're writing this down! It's always nice to have more pros and cons. But just to go on record and be clear (to anyone who might read this), an alternative syntax would have to be pretty elegant, concise, and generally dev-friendly in order for me to consider another breaking change in the binding API.

@TysonMN
Copy link
Member

TysonMN commented Jan 9, 2020

Drawbacks

...
2. It requires naming the chosen case of the discriminated union
3. Each case requires a unique name, and even without optional/default arguments, the list of cases could be rather long

This doesn't seem as bad to me now.

The compiler branches in order to determine the function/method to call. When overloading, the branching happens implicitly based on the types of the arguments. When using DUs as described by Mark, the bracing happens explicitly in a match of the DU instance.

Overloaded methods don't have (meaningful) differentiating identifiers (since the branching is implicit), and we could closely preserve this by having the DU case names be as short as possible. For example, they could be C1, C2, C3, ... and so on.

With such short case names, I don't think the two disadvantage above are that bad. If there are always less than ten cases, then this only requires adding four additional charters. It would look something like

Binding.oneWay(f)

changing to

Binding.oneWay(C1(f))

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

Successfully merging a pull request may close this issue.

2 participants