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

Added type annotations on the mkProgram functions #313

Merged

Conversation

pauldorehill
Copy link

When starting to using the Program.mkProgram, Program.mkSimple the intellisense/compiler currently show generic type names like 'a which makes it hard to work out what your functions need to do:

image

So I've added the type annotations to make it a bit clearer:

image

@pauldorehill
Copy link
Author

pauldorehill commented Feb 5, 2019

As a side note while doing this I was also curious on the Program type:

type Program<'model, 'msg, 'view> = 
    { init : unit -> 'model * Cmd<'msg>
      update : 'msg -> 'model -> 'model * Cmd<'msg>
      subscribe : 'model -> Cmd<'msg>
      view : 'view
      debug : bool
      onError : (string * exn) -> unit }

Does the view property need to be generic? I'm just getting started with Xamarin.Forms so there may well be an obvious reason I don't know... but I couldn't see a reason why it can't be defined as:

type Program<'model, 'msg> = 
    { init : unit -> 'model * Cmd<'msg>
      update : 'msg -> 'model -> 'model * Cmd<'msg>
      subscribe : 'model -> Cmd<'msg>
      view : 'model -> ('msg -> unit) -> ViewElement
      debug : bool
      onError : (string * exn) -> unit }

This would then make the required function signatures clearer still? If you agree I can update the commit?

It would also require adding say a StaticProgram type in StaticViewProgram.fs.

@TimLariviere
Copy link
Member

TimLariviere commented Feb 6, 2019

Does the view property need to be generic?

Maybe it's related to the StaticView part, but this seems more like an early code that haven't needed updating since then.
Makes sense to enforce a specific type annotation, clearer that way.

@pauldorehill
Copy link
Author

Ok, I'll update the pull request with the types updated.

@dsyme
Copy link
Collaborator

dsyme commented Feb 7, 2019

Yes the generic 'view is related to the half-elmish support

I wonder if we should duplicate out the half-elmish support into an entirely separate DLL. Or deprecate/remove it - though there are still people who advocate it as a stepping stone.

@pauldorehill
Copy link
Author

pauldorehill commented Feb 8, 2019

If you were to remove it I guess the question is whats the target audience for Fabulous?. Maybe initially move it to a separate project/dll? If you want to move I can add as part of this pull request?

@TimLariviere
Copy link
Member

Today, most of the work goes to the full-elmish approach. We're not actively supporting half-elmish.
I think we still need to keep it around, but maybe we should go as far as putting it in a different Github project?

That way, we separate it from Fabulous and say we won't support it further.
But if people think it has its place, they will be able to maintain it.

Maybe initially move it to a separate project/dll? If you want to move I can add as part of this pull request?

I would prefer to merge this PR as-is. It's useful enough.
And do another PR later to add type annotation to 'view.

@TimLariviere
Copy link
Member

Merging.
We will separate the half-elmish in a another PR later, before changing type annotation for 'view

@TimLariviere TimLariviere merged commit ce21d88 into fabulous-dev:master Feb 12, 2019
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 this pull request may close these issues.

3 participants