elmish / react

Elmish React extensions for writing SPA and Native apps
https://elmish.github.io/react
Other
104 stars 21 forks source link

See if we can get rid of explicit `dispatch` in the views #39

Open et1975 opened 5 years ago

et1975 commented 5 years ago

People keep finding interesting ways to misuse the dispatch function we are passing into views. Eliminating it has been on my wishlist from day one, unfortunately it means a DSL on top of React and all the libs we may want to use in the views. Up till now I couldn't see a way around it, but maybe with type providers now working on Fable we can take a generic React lib and produce a DSL that would let us remove dispatch argument from the views?

MangelMaxime commented 5 years ago

I am a bit sceptical about type providers capabilities to support any generic React library. There is many different ways of writing React components and that many ways to understand/support.

Also, it will require having d.ts definition files update to date which is rarely the case. Some react components don't have one.

I am not sure generic type providers are working yet with Fable but I can be wrong.

alfonsogarciacaro commented 5 years ago

Yes, I'm not sure if we could use type providers for this. What kind of misuses you have in mind? What I've heard is the dispatch argument makes it difficult to make elmish views compatible with React.memo. I'm trying now with this adapter:

let memoElmish name (view: 'Model->('Msg->unit)->ReactElement) =
    let mutable cachedView = None
    fun model dispatch ->
        let view =
            match cachedView with
            | Some v -> v
            | None ->
                let v = memoBuilderWith name (=) (fun m -> view m dispatch)
                cachedView <- Some v
                v
        view model

This caches the dispatch function the first time so it's not an issue when comparing the models to see if there've been actual changes, so it's more restrictive and users have less opportunities to misuse it :)

et1975 commented 5 years ago

There's a recent question in gitter where someone thought it would be a good idea to capture the dispatch reference and pass it via a message over to some other UI element. Before that people saw dispatch in the view and thought it would be be neat to have it in the update as well. Eliminating the callback lambdas (or rather building them ourselves) would let us optimize the diffing as well.

I'm not sure yet how elm figures ChildMsg >> dispatch automagically, but it would be fun to figure out.

alfonsogarciacaro commented 5 years ago

someone thought it would be a good idea to capture the dispatch reference and pass it via a message over to some other UI element.

Was that because of the view hierarchy? Maybe React portals could help with that.

people saw dispatch in the view and thought it would be be neat to have it in the update as well

Yeah, I remember trying to do something similar before understanding Cmd well 😸 Having to write extra messages for successful responses sometimes feel verbose too, using Fable.Reaction can help here too.

I'm not sure yet how elm figures ChildMsg >> dispatch automagically, but it would be fun to figure out.

When I tried to adapt Elmish to Vue, there wasn't a render function (only a template string). So the update function was called directly after dispatch (without bubbling up the message) and then the state was updated directly with the result, but this is only possible because Vue's reactive system and I didn't get to build a centralized state management: https://github.com/alfonsogarciacaro/fable-vue/blob/85500d7a8805565c24215bcfb38f59180d3ccb37/samples/elastic-header/src/DraggableHeader.fs

alfonsogarciacaro commented 5 years ago

I just found that React has added the useReducer hook to get the dispatch function from within a function component. As it's possible to create custom hooks, maybe we could do something similar for Elmish.

0x53A commented 4 years ago

A useDispatch hook would be great, maybe this could use the Context API internally?

I have thought about this for a total of 5 Minutes, and this is what I came up with:

// ------------------------------------------------------------------------------
// 1. there is one top-level context for the "dispatch" function, probably defined by the framework, but could also be defined by the application itself.

let globalDispatchCtx = createContext (fun (_:Msg) -> ())

// ------------------------------------------------------------------------------
// 2. each module/Page defines one context for dispatch
let dispatchCtx = createContext (fun (_:Msg) -> ())
let private useDispatch() = Hooks.useContext dispatchCtx

// remove dispatch from view
let view model = ...

// if you need the dispatch inside the view (no FunctionComponent), it's easy:
let view model =
    contextConsumer dispatchCtx (fun dispatch ->
        // ...
    )

// inside a FN it's even easier:
let dispatch = useDispatch()

// ------------------------------------------------------------------------------
// 3. before you call a sub-view, you need to set the context:
// One issue is that you lose the compile time safety, if you forget to register the sub-dispatch, the default handler will be called

let withCarouselDispatch =
    let mutable outerDispatch = ignore
    // static instance so it stays stable!
    let subDispatch = fun msg -> outerDispatch(CarouselMsg msg)
    fun children ->
        contextConsumer globalDispatchCtx (fun dispatch ->
            outerDispatch <- dispatch
            contextProvider Carousel.dispatchCtx subDispatch children
         )

let view (model:Model) =
    div [] [
        withCarouselDispatch [
            Pages.Carousel.view model.CarouselModel
        ]
    ]
MangelMaxime commented 4 years ago

It's just a draft, but as it is if the user need to write all this code to have the dispatch arguments removed from the view function, I don't think it's worth it.

IHMO there is too much code to write and it can break too easily.

0x53A commented 4 years ago

I have the issue that I need to somehow pass dispatch to the child of a third-party component, so I'm already using a context for dispatch, with this I was trying to formalize it a little.


IHMO there is too much code to write [...]

Parts of it can be moved to a lib, for example:


// ------------------------------------------------------------------------------
// Lib

let withMappedDispatch map =
    let mutable outerDispatch = ignore
    // static instance so it stays stable!
    let subDispatch = fun msg -> outerDispatch(map(msg))
    fun children ->
        contextConsumer globalDispatchCtx (fun dispatch ->
            outerDispatch <- dispatch
            contextProvider Carousel.dispatchCtx subDispatch children
         )

// ------------------------------------------------------------------------------
// User

let withCarouselDispatch = withMappedDispatch CarouselMsg 

[...] and it can break too easily

Yeah, that's also my major concern, that I can just forget to wrap it in withCarouselDispatch.


Maybe I'll try to "convert" the bookstore, that should give a feeling whether this pattern makes sense in the large.


Edit:

Maybe a CE could be (mis)used help here, just for the type annotations:


// Lib

let DispatchableSubViewBuilder<'TMsg>() = ...

// Pages/Carousel

let carouselView = DispatchableSubViewBuilder<Carousel.Msg>()

let view model = carouselView {
    div [] [ str "Hello World" ]
}

// Main

let view model = mainViewCE {
    // doesn't compile because the types don't match
    yield! carouselView model

    // custom CE operation enforces the dispatch mapping
    subView withCarouselDispatch Carousel.view
}

That's probably too complicated, but it's a tiny little bit like Saturn

kspeakman commented 1 year ago

It looks like Fabulous was able to remove dispatch from being a user-facing concern.

Elm's approach for this is that event attributes accept a msg tag fn instead of a side effect (unit-returning) fn. For example onInput in F# would be OnInput of string -> 'msg. Dispatch is propagated to views behind the scenes, and the returned Msg is fed into the view's dispatch. The views (and attributes) have a type parameter for Msg: Html msg, would be in F# Html<'msg>. There is also a Html.map fn to tag child view msgs to parent msg type. It's like doing this in Elmish:

type Msg =
    | SidebarMsg of Sidebar.Msg
    | OtherMsg of . . .

let view model dispatch =
    // tag child msg as a SidebarMsg before dispatch
    Sidebar.view model (SidebarMsg >> dispatch)

Additionally, Elm makes some default choices for common event usage. For example, OnInput above provides e.target.value string -- the most common way you'd use it -- instead of the browser event. This makes it soooo nice to use: OnInput MyFieldChanged, compared to current usage: OnInput (fun e -> dispatch (MyFieldChanged e.Value)). There are only 13 event attributes defined in Elm (maybe more in specific modules). I'm assuming it's only the ones that have an obvious default. Otherwise, there is a generic on event to turn whatever event you want into a msg.

As far as a generic solution that could apply to multiple UIs, there could be a View<'msg> type that keeps the local dispatch fn. And a similar View.map fn to propagate the dispatch down to child views. Often the Msgs are kept flat and the map fn doesn't need to get used.

Edit: Instead of dispatch being propagated down to child views, maybe the msg tag could be propagated? Then dispatch is only called at the top level? Elm may actually do this, been a minute since I looked at the view code.

et1975 commented 1 year ago

Right, Html.map - as long as you have your own view DSL getting rid of dispatch arg is not complicated, but short of introducing 3rd React DSL and requiring whole new ecosystem of binding libs, what can we do?

alfonsogarciacaro commented 1 year ago

My thinking is that with the improvements in Elmish 4, users will tend to use Elmish at the component level more often (as with UseElmish) so passing dispatch to children will become less common.

About event defaults, the way Feliz solves this is by using a class for the API, so it can provide overloads with convenient methods when necessary, for example for onTextChange or p containing only text.

kspeakman commented 1 year ago

Right, Html.map - as long as you have your own view DSL getting rid of dispatch arg is not complicated, but short of introducing 3rd React DSL and requiring whole new ecosystem of binding libs, what can we do?

Lol, sarcasm detected (and enjoyed). You're right. But the reality is that dispatch is depended on all the way down into the UI tech at this point. Removing dispatch would need to happen in the opposite direction. It can only be removed from Elmish when its no longer depended on to very high degrees. Or more realistically by starting a shiny new project.

kspeakman commented 1 year ago

@alfonsogarciacaro Thanks for those links. I've seen previous approaches and it's great that UseElmish doesn't require side effecting hooks in the logic areas.

It seems like this also makes a hybrid approach possible. Where there's still a ✌️deterministic✌️ html dsl view fn (b/c reasons), underpinned by a React component calling Fable.React.UseElmish. So the wiring would only be thru view and not also thru init/update/model/msg.

et1975 commented 1 year ago

Sorry Kasey, no sarcasm intended. This was really a pie in the sky kinda question and our own DSL would be great in several ways (no need to worry about event handler diffing when caching the view, for example). But it would also be totally impractical without a way to use existing bindings.

kspeakman commented 1 year ago

All good. I was cracked up. :) And you're right as in my suggestion was impractical.

The other equally impractical way I thought of was to change the Elmish view API to msg-returning, but under the covers convert the user view to a dispatch-based react view. But that's still writing our own DSL. And potentially bad perf if we have to walk the user view to convert it.

alfonsogarciacaro commented 1 year ago

But it would also be totally impractical without a way to use existing bindings.

One thing to note is in the latest Fable.React we have moved away ReactElement and other basic types into their own library Fable.React.Types. The purpose was that Feliz (or any other library) could just take that dependency and use the ReactElement interface so even when writing a new API, the code will still be compatible with components using other DSLs.