fable-react icon indicating copy to clipboard operation
fable-react copied to clipboard

Implement Elmish Commads in ReactiveCom

Open EdouardM opened this issue 7 years ago • 14 comments

Fix #62. Enables Elmish Commands in ReactiveCom:

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

EdouardM avatar Feb 26 '18 19:02 EdouardM

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 :wink:) or to its own package?

alfonsogarciacaro avatar Feb 26 '18 21:02 alfonsogarciacaro

Any thoughts on this @zaaack?

alfonsogarciacaro avatar Feb 26 '18 21:02 alfonsogarciacaro

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.

zaaack avatar Feb 27 '18 00:02 zaaack

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

MangelMaxime avatar Feb 27 '18 00:02 MangelMaxime

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.

EdouardM avatar Feb 27 '18 09:02 EdouardM

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# 🤔

alfonsogarciacaro avatar Feb 28 '18 13:02 alfonsogarciacaro

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

EdouardM avatar Feb 28 '18 15:02 EdouardM

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

irium avatar Feb 28 '18 20:02 irium

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

alfonsogarciacaro avatar Mar 01 '18 15:03 alfonsogarciacaro

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.

et1975 avatar Mar 01 '18 15:03 et1975

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?

alfonsogarciacaro avatar Mar 05 '18 21:03 alfonsogarciacaro

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

Luiz-Monad avatar Jun 10 '19 14:06 Luiz-Monad

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.

alfonsogarciacaro avatar Jun 11 '19 08:06 alfonsogarciacaro

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)

Luiz-Monad avatar Jun 12 '19 14:06 Luiz-Monad

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

alfonsogarciacaro avatar Sep 23 '22 01:09 alfonsogarciacaro