Clarification and Potential Improvement on PR #480 useEffectOnce behavior

[lukaszkrzywizna]

@Zaid-Ajaj @JordanMarr I've checked the PR #480 and I wonder if that code is correct when not using strict mode:

if renderAfterCalled.current
then destroyFunc.current
else None

I'm asking because if I an component unmounts the destroy func is not invoked:

// not using strict mode

let dispose = React.createDisposable(fun () -> eprintf "Disposed")
    
// dispose never invoked
React.useEffectOnce(fun () -> dispose)
    
// disposed invoked
React.useEffect((fun () -> dispose), [||])

I wonder what's the point of this function? If it's created to handle strick mode's double useEffect run, then we should reflect this in a name or comment. Otherwise we should fix it somehow - my first idea is to add check for a debug:

  static member private useEffectOnce(effect: unit -> System.IDisposable) = 
    let destroyFunc = React.useRef None
    let calledOnce = React.useRef false
    let renderAfterCalled = React.useRef false

    if calledOnce.current then
        renderAfterCalled.current <- true
    
    React.useEffect (fun () -> 
        if calledOnce.current 
        then None
        else
            calledOnce.current <- true
            destroyFunc.current <- effect() |> Some
#if DEBUG
            if renderAfterCalled.current
#else
            if not renderAfterCalled.current
#endif
            then destroyFunc.current
            else None
    , [||])

But this does not guarantee that user has strict mode enabled and we can still have situation with not invoked dispose.

What do you think about it?

[JordanMarr]

@lukaszkrzywizna I admit that the changes to useEffect in React were more than I could keep up with. It sounds like there are even more Edge cases to be concerned with if you factor in “strict mode” (which I also was not aware of).

So the only way to fix would be to come up with a handful of use cases that should all work and verify they do. (It sounds like you have identified one case that doesn’t already).

[lukaszkrzywizna]

@JordanMarr, which edge cases are you considering? I assumed that this was related to strict mode because I can't think of any other scenarios where useEffect would run twice. Nevertheless, we shouldn't account for strict mode in this hook since the React team introduced the double render feature to catch logic errors.

[JordanMarr]

I find the changes to React useEffect in 18 confusing, and as a result, have moved on to using Fable.Lit.
But it sounds like you know what you're talking about and have a good read on the issue. I trust your judgement! 😎

[lukaszkrzywizna]

@JordanMarr thanks for the info 😄 @Zaid-Ajaj, In that case, I think we should adjust the useEffectOnce code to the simple alias useEffect(func, [|]) or remove it completely. If you are okay with that I will create a simple PR.

[Zaid-Ajaj]

I think we should adjust the useEffectOnce code to the simple alias useEffect(func, [|]) ... If you are okay with that I will create a simple PR

@lukaszkrzywizna that would be awesome 🙏 thanks a lot