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

Implement Elmish Commads in ReactiveCom #63

Closed
wants to merge 3 commits into from

Conversation

EdouardM
Copy link

@EdouardM EdouardM commented Feb 26, 2018

Fix #62. Enables Elmish Commands in ReactiveCom:

  • Added a depedency to Fable.Elmish
  • Implemented Cmd<'Msg>
  • Implemented an error handler in the component

- Added a dependency to Fabl.Elmish
- Rewrite of the update loop of ReactiveCom
- Added an Error handler to ReactiveCom
- Added a dependency to Fabl.Elmish
- Rewrite of the update loop of ReactiveCom
- Added an Error handler to ReactiveCom
@alfonsogarciacaro
Copy link
Member

Great work @EdouardM, thank you! Hmm... I'd prefer to avoid the dependency as Fable.React should be architecture agnostic. Maybe move reactiveCom to Elmish.React (though @MangelMaxime didn't like it very much 😉) or to its own package?

@alfonsogarciacaro
Copy link
Member

Any thoughts on this @zaaack?

@zaaack
Copy link
Contributor

zaaack commented Feb 27, 2018

At the very beginning I implemented Reactive Component only for ui components which only contains small UI state that I don't want to put in global state, so Elmish style side effects manager is not needed here.

But if you want to do side effects like fetch some data from network, currently you can only do it in view function because only view has access to dispatch. Perhaps we can passing dispatch to the update function or even further, moving dispatch to component instance method, and passing the instance to update as context, then we can access all react functionality in update, which is much light weight and don't need to dependent on Elmish.

@MangelMaxime
Copy link
Member

I agree that Fable.React should not include a deps on Fable.Elmish

I am much more in favor of an independant package but I am probably in minority here :)

@EdouardM
Copy link
Author

I agree I can send a PR to Elmish.React with this something similar to this code.

I could also add another "simple" Reactive Component with no side effect manager. Like it is the case in Fable.Elmish with mkSimple and mkProgram.

@alfonsogarciacaro
Copy link
Member

So these are our options 😄

  1. Move reactiveCom to Fable.Elmish.React to use Elmish Cmd
  2. Do 1 but also keep a simple reactiveCom without side effects in this library
  3. Pass the dispatch function to update (or move dispatch to component instance method) as @zaaack suggests

Not sure what's the best one, though I'd try to avoid confusion for the users (for example, having two similar components in different libraries). Passing the current instance as context will probably mean we go "full React" and reactiveCom becomes just a way to build a reusable React component with a nicer syntax in F# 🤔

@EdouardM
Copy link
Author

There is another option, mixing 1 and 2: Do 1 and also have a simple reactiveCom without side effects in the Elmish.React library.

@irium
Copy link
Contributor

irium commented Feb 28, 2018

I'd vote for option 2, giving them separate names, eg reactiveElmishCom

@alfonsogarciacaro
Copy link
Member

@et1975 What do you think? What would be your preferred way to create reusable React components using the Elmish architecture?

@et1975
Copy link
Member

et1975 commented Mar 1, 2018

I agree that React bindings should remain just that - language support for the library. However tempting tacking more helpers on it might be, it would impact maintainers ability to keep up with changes in React and Fable.

I also don't see adding any stateful components support into Elmish.React happening, as me and my colleagues have managed to avoid needing it over the course of several years and numerous projects... maybe we just didn't have the problem you are facing or maybe there's a stateless solution that would accomplish it.

In any case, I'm in the Separate Library camp. Even if it's just 1 F# file, one could import it as github dependency with paket.

@alfonsogarciacaro
Copy link
Member

So what do we do? Shall we move this to an independent package (we can put it in this repo if you prefer) with the Elmish dependency? And remove reactiveCom from here or make it more React-ish?

@Luiz-Monad
Copy link
Contributor

Luiz-Monad commented Jun 10, 2019

I think it would be possible to make 'Cmd a generic parameter to avoid the dependency on Elmish.
But I guess this is stale.

@alfonsogarciacaro
Copy link
Member

Yes, we haven't dealt with this PR for a very long time 😅 But you're right @byte-666, that could be a solution. I've been also thinking about it and if we want to create React components in Elmish-style, instead of bringing the full Elmish architecture here, maybe we should just PR to Elmish.React to add the possibility of instantiating the Elmish app as a component instead of a full program.

@Luiz-Monad
Copy link
Contributor

Luiz-Monad commented Jun 12, 2019

Yeah, I did something like that, creating a full elmish style rooted component.
I was trying to solve that other performance problem with MaterialUI withStyles, the problem was elsewhere (makeStyles solved it, together with the solution to that pesky InputRef problem, using Elmish.React.Helpers.valueOrDefault [already closed] ).

Example:

[<RequireQualifiedAccess>]
module Internal =

    type RootProps<'model> = {
        key: string
        init: unit -> 'model
        hook: ('model -> unit) -> unit
        equal: 'model -> 'model -> bool
        render: 'model -> ReactElement
    }

    type RootView<'model> (props) as this =
        inherit Component<RootProps<'model>, 'model> (props)
        do this.setInitState <| props.init ()
        do props.hook <| this.update

        member this.update model = this.setState ( fun _ _ -> model )

        override this.shouldComponentUpdate (_nextProps, nextState) =
            not <| this.props.equal this.state nextState

        override this.render () =
            this.props.render this.state

    let rootView2With key equal init hook render =
        ofType<RootView<_>,_,_>
            { key = key; init = init; hook = hook; equal = equal; render = render }
            []

let withReactRootUsing equal placeholderId (program: Elmish.Program<_,_,_,_>) =
    let setState = ref ignore
    let root =
        Internal.rootView2With "root"
            ( fun x y ->
                match x, y with
                | (Some x, _), (Some y, _) -> equal x y
                | _ -> false )
            ( fun () -> ( None, ignore ) )
            ( fun hook -> setState := hook )
            ( fun ( model, dispatch ) ->
                match model with
                | Some m -> Program.view program m dispatch
                | _ -> div [] [] )
    let renderRoot () =
        ReactDom.render ( root, document.getElementById placeholderId )
    let setState model dispatch =
        let render = lazy ( renderRoot () )
        !setState ( Some model, dispatch )
        render.Value
    program |> Program.withSetState setState

I plugged it to Elmish, but you could supply setState to any kind of dispatcher and it would work.
Set state is not fired from the render, that makes it easier to plug into Elmish dispatcher, and I don't think it's a problem for Stateful React components.
But I agree that this code like this should belong elsewhere, not here, and this is only binding and React has tons of ways of doing things.
Now we could even use Hooks to create this kind of feature. (I'll try it later as I don't like creating OO types, also I didn't like using ref, but it made the code shorter by using Elmish infrastructure)

@alfonsogarciacaro
Copy link
Member

If it's ok with everybody I will close this PR as the preferred way to have Elmish components now is useElmish: Zaid-Ajaj/Feliz#481

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.

Implement Elmish commands in ReactiveCom
7 participants