[v3.0] `Can dispose of optional IDisposables`-test fails

[Freymaurer]

Test

testPromise "Can dispose of optional IDisposables" <| fun () -> promise {
        let render = RTL.render(OptionalDispose.render {| isSome = true |})

        Expect.toHaveTextContent (render.getByTestId("was-disposed")) "false" // "Should not be disposed yet"

        do! RTL.userEvent.click(render.getByTestId("dispose-button"))

        do!
            RTL.waitFor <| fun () ->
                Expect.toHaveTextContent (render.getByTestId("was-disposed")) "true" //"Should have been disposed"
    }

Summary

Checks if a react component unmounted with a React.useEffectOnce(fun () -> myIDisposable) triggers the _.Dispose() effect.

The test fails, it seems like the _.Dispose() function is not called.

Investigation

[<Hook>]
    let useEffectDisposableOptOnce (effect: unit -> #IDisposable option) =
        let destroyFunc = Interop.reactApi.useRefInternal None
        let calledOnce = Interop.reactApi.useRefInternal false
        let renderAfterCalled = Interop.reactApi.useRefInternal false

        if calledOnce.current then
            renderAfterCalled.current <- true

        useEffectDisposableOptWithDeps (fun () -> 
            if calledOnce.current 
            then None
            else
                calledOnce.current <- true
                destroyFunc.current <- effect()

                if renderAfterCalled.current
                then destroyFunc.current
                else None
        ) [||]

This is the main part of the function and to be honest: I have never used [<Hook>] before and have no idea why there are multiple internal refs here.

I had a quick look at the docs and came up with the following:

let useEffect (setup: System.Func<unit, (unit -> unit)>) = import "useEffect" "react"
let useEffectWithDisOpt (setup: unit -> System.IDisposable option) = 
    useEffect(System.Func<_,_>(fun () ->
        let dispose = setup()
        match dispose with
        | Some d -> fun () ->  d.Dispose()
        | None -> fun () -> ()
    ))

For which the test passes.

As i do not understand the original implementation i wanted to double check if this is fine @Zaid-Ajaj @MangelMaxime

[MangelMaxime]

I think this is a specialized hook but I never needed it myself, in general I kept using simple Hooks + Elmish hooks.

Behavior of the hooks will need to be checked with the move to JSX + upgrade to React 19 as they can be sensitive and sometimes React breaks stuff.

I am mentioning it in the sense that, we should wait for JSX + React 19 upgrade before taking a decision I believe.

But if this still fails afterward, personally I am fine with removing it and making the code easier to maintain. If someone needs it, they can open an issue and describing in which context such hooks is needed.

[Freymaurer]

More context on this function: #600 #595