Avalonia.FuncUI icon indicating copy to clipboard operation
Avalonia.FuncUI copied to clipboard

Ensuring unique hook identity for custom hooks (v2)

Open JaggerJo opened this issue 2 years ago • 10 comments

More elegant than what I did in https://github.com/fsprojects/Avalonia.FuncUI/pull/207 .

Basically this allows library authors to create composed hooks without explicitly giving each hook a unique identity.

JaggerJo avatar Jul 17 '22 21:07 JaggerJo

Note that this is a (small) breaking change.

JaggerJo avatar Jul 17 '22 21:07 JaggerJo

@JordanMarr can you take a look?

JaggerJo avatar Jul 18 '22 06:07 JaggerJo

Very nice! I should have some time to look at this in more detail on Wed morning.

JordanMarr avatar Jul 20 '22 06:07 JordanMarr

After looking at your PR, I have some questions.

First, when do I, as an end user, need to be cognizant of line numbers? Is it anytime I am creating a hook, or does this only apply to "composed hooks"?

So then let's say I make a useCounter hook that encapsulates a useState hook; is that a composed hook? Note: this example is still based off of the master branch. Also, I added the ?identity arg only because I copied and pasted from another hook. Should I have used it somewhere in my hook? Does your PR make it unnecessary?

module Examples.CounterWithCustomHook

open Avalonia.Layout
open Avalonia.Controls
open Avalonia.FuncUI
open Avalonia.FuncUI.DSL
open Avalonia.FuncUI.Types

[<AutoOpen>]
module CustomHook = 
    open System.Runtime.CompilerServices

    type IComponentContext with
        member ctx.useCounter(initialCount: int, [<CallerLineNumber>] ?identity: int) =
            let counter = ctx.useState(initialCount)
            let increment () = counter.Set(counter.Current + 1)
            let decrement () = counter.Set(counter.Current - 1)
            counter.Current, increment, decrement

let cmp () = Component (fun ctx ->
    let count, increment, decrement = ctx.useCounter 0
    
    Grid.create [
        Grid.rowDefinitions "20"
        Grid.columnDefinitions "*, 80, *"
        Grid.children [

            StackPanel.create [
                Grid.column 1
                Grid.row 1

                StackPanel.children [
                    // Increment
                    Button.create [
                        Button.content "▲"
                        Button.horizontalAlignment HorizontalAlignment.Stretch
                        Button.horizontalContentAlignment HorizontalAlignment.Center
                        Button.onClick (fun _ -> increment ())
                    ]

                    // Current count
                    TextBlock.create [
                        TextBlock.text (string count)
                        TextBlock.horizontalAlignment HorizontalAlignment.Center
                    ]

                    // Decrement
                    Button.create [
                        Button.content "▼"
                        Button.horizontalAlignment HorizontalAlignment.Stretch
                        Button.horizontalContentAlignment HorizontalAlignment.Center
                        Button.onClick (fun _ -> decrement ())
                    ]

                ]
            ]

        ]
    ]
    :> IView
)

JordanMarr avatar Jul 21 '22 18:07 JordanMarr

@JordanMarr excellent questions.

Every state hook is identified in the component context by its caller line number vs. the order in which they are called in react. This makes is possible to render hooks conditionally, because we don't use the order (which would change) to store/restore the hooks state next time we render.

The problem with this approach is that if a hook is not directly called in the component's render function the caller line number of the function in which the hook is called will be used.

In the example below the caller line number for the let counter = ctx.useState(initialCount) never changes. This results in all ctx.useCounter hooks pointing at the same state when used in a component.

module Main =
    open Avalonia.Controls
    open Avalonia.FuncUI
    open Avalonia.FuncUI.DSL
    open Avalonia.Layout

    [<AutoOpen>]
    module CustomHook =
        open System.Runtime.CompilerServices

        type IComponentContext with
            member ctx.useCounter(initialCount: int) =
                let counter = ctx.useState(initialCount)
                let increment () = counter.Set(counter.Current + 1)
                let decrement () = counter.Set(counter.Current - 1)
                counter.Current, increment, decrement

    let view =
        Component
            (fun ctx ->
                // increment / decrement on this state hook modify state of count below.
                let _, increment, decrement = ctx.useCounter(0)
                let count, _, _ = ctx.useCounter(0)

                DockPanel.create [
                    DockPanel.children [
                        Button.create [
                            Button.onClick (fun _ -> decrement ())
                            Button.content "-"
                        ]
                        Button.create [
                            Button.onClick (fun _ -> increment ())
                            Button.content "+"
                        ]
                        TextBlock.create [
                            TextBlock.text (string count)
                        ]
                    ]
                ]
            )

This can be solved by passing the caller line number like this:

[<AutoOpen>]
module CustomHook = 
    open System.Runtime.CompilerServices

    type IComponentContext with
        member ctx.useCounter(initialCount: int, [<CallerLineNumber>] ?identity: int) =
            let counter = ctx.useState(initialCount, identity.Value)
            let increment () = counter.Set(counter.Current + 1)
            let decrement () = counter.Set(counter.Current - 1)
            counter.Current, increment, decrement

This brings us to the problem this PR is trying to solve: multiple state hooks in a custom hook.

[<AutoOpen>]
module CustomHook = 
    open System.Runtime.CompilerServices

    type IComponentContext with
        member ctx.useCounter(initialCount: int, [<CallerLineNumber>] ?identity: int) =
            let counterA = ctx.useState(initialCount, identity.Value) // <- can't be the same
            let counterA = ctx.useState(initialCount, identity.Value) // <- can't be the same
            counterA, counterB

Right now you need to manually add a identity suffix, with this PR you can do this:

[<AutoOpen>]
module CustomHook = 
    open System.Runtime.CompilerServices

    type IComponentContext with
        member ctx.useCounter(initialCount: int, [<CallerLineNumber>] ?identity: int) =
            ctx.useGrouped (identity.Value, fun ctx -> 
                let counterA = ctx.useState(initialCount) <- implicitly get's set to `identity.{42} assuming this is line 42`
                let counterA = ctx.useState(initialCount) <- implicitly get's set to `identity.{43}`
                counterA, counterB
            )

JaggerJo avatar Jul 22 '22 09:07 JaggerJo

@JordanMarr not sure if the additional complexity of using the caller line number is worth it vs. using the calling order (like react). We might want to consider switching to the react way.

JaggerJo avatar Jul 22 '22 09:07 JaggerJo

@JordanMarr not sure if the additional complexity of using the caller line number is worth it vs. using the calling order (like react). We might want to consider switching to the react way.

I honestly do think conditional hooks are a useful thing, I'm doing React most of my dev day and there's a bunch of situations when you want a hook with side effects to be executed under a certain condition and having to go through all the trouble of setting up a new component just to be able to conditionally call a hook is a hassle. I haven't seen the code of this PR yet, but unless it's an incredibly complex logic to allow this or it affects performance greatly I'd say it's a good thing to have.

sleepyfran avatar Jul 22 '22 10:07 sleepyfran

Sorry for the delay, it's been a busy couple of weeks.

That's definitely an improvement!

My only concern is that the concept of passing a line number at all is hook internal implementation details leaking. If line numbers are confusing to me, they will be confusing to others too.

Is it fair to say that the tradeoff is: no conditional hooks and line numbers or conditional hooks with no line numbers? (EDIT: I had it backwards)

If so, conditional hooks would require a lot less knowledge -- only one simple rule for end users to remember: "no conditional hooks!"

JordanMarr avatar Jul 27 '22 14:07 JordanMarr

Approved (with comment that it will be confusing to users)

I agree with @JordanMarr and think we should move to simpler calling order based identity. Also think that conditional hooks are sometimes pretty useful @sleepyfran but we could still allow explicit hook identity to enable this to more experienced users if we really wanted.

I'm on vacation starting from today, but think the "a hooks identity is determined by the caller order" is pretty simple to implement - and what we should do IMO.

JaggerJo avatar Jul 28 '22 14:07 JaggerJo

That sounds like a best of both worlds approach. The common scenario “just works” if you follow React rules and avoid conditional hooks, but a line number can be passed for more niche, advanced cases.

JordanMarr avatar Jul 28 '22 18:07 JordanMarr

Closed in favour of #214

JaggerJo avatar Aug 21 '22 17:08 JaggerJo