fabulous-dev / Fabulous

Declarative UI framework for cross-platform mobile & desktop apps, using MVU and F# functional programming
https://fabulous.dev
Apache License 2.0
1.13k stars 122 forks source link

Bypass View.map conversion if msg is already of type 'newMsg #1019

Closed TimLariviere closed 1 year ago

TimLariviere commented 1 year ago

When combining a child view with a parent view, we can use View.map to convert the child's messages to the parent Msg type. This approach even lets us define additional modifiers to that child view at the parent's level.

type ParentMsg =
    | ChildMsg of Child.Msg
    | MoreMsg

(View.map ChildMsg childView)
    .backgroundColor(Color.Red)
    .onTapped(MoreMsg)

This is completely legal in Fabulous as the types are the ones expected by the F# Type Checker.

But there is an issue. When calling View.map on the child view, Fabulous assigns a conversion function (from Child.Msg to ParentMsg) to the child control itself. This conversion will then get applied to all messages dispatched by the child, including the ones we defined after, at the parent's level.

This leads to an invalid cast exception at runtime.

InvalidCastException: ParentMsg.MoreMsg is not of type Child.Msg

This PR aims to provide a workaround to this bug by checking the dispatched msg type before trying to convert it. If the msg type already matches the final type, we do nothing.

twop commented 1 year ago

Nice one! In theory there might be a cascade View.map. Maybe we can somehow prohibit adding any other modifiers after View.map on a type level?

Not sure if/how it can be possible though, potentially we will need add several marker interfaces and remove one of them after map. Hard one :(

twop commented 1 year ago

In theory we can use the same trick as we use with lazy'

 let lazy'<'msg, 'key, 'marker when 'key: equality>
        (fn: 'key -> WidgetBuilder<'msg, 'marker>)
        (key: 'key)
        : WidgetBuilder<'msg, Memo.Memoized<'marker>>

E.g. we don't allow to add any other attributes after lazy, it is quite a lot of code to add, but I think that might be a more reliable solution

TimLariviere commented 1 year ago

@twop I agree with you it would be safer to prevent adding event listeners to a mapped view, but I found it's a pretty common pattern to adapt layout in the parent by adding extra modifiers.

Grid(...) {
    (View.map ChildMsg childView)
        .margin(5.)
        .gridRow(1)
}

Blocking all modifiers after View.map would add a lot of friction when actually adding extra event listeners feels more like a corner case.

I made a full production app without ever encountering this issue. It's only when I tried to make a self-contained extension that plugs into the event of the app (https://github.com/fsharp-mobile/Fabulous.XamarinForms.SaveState) that I found this problem.

If this PR doesn't impact performances, I would prefer just using it rather than preventing people from adding extra modifiers.

twop commented 1 year ago

Makes sense! Would be nice to bullet proof some edge cases, but I feel like it should be super rare

from perf point of view this is totally fine, we can probably consider sending messages as rare, and even then the effects of messages dwarf any internal shenanigans of unwrapping the message

TimLariviere commented 1 year ago

Would be nice to bullet proof some edge cases

I added a couple unit tests, including cascading View.map.

Caught a small problem while writing unit tests. The underlying type of a DU case is not necessarily the DU type itself, based on if at least one case has parameters. The equal condition was invalid in this case, I changed it to IsAssignableFrom and it works way better.

Source: https://stackoverflow.com/questions/56510719/f-how-to-get-the-type-of-an-instance-of-an-empty-discriminated-union-case-usin