Open lukaszkrzywizna opened 4 months ago
@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).
@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.
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! 😎
@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.
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
@Zaid-Ajaj @JordanMarr I've checked the PR #480 and I wonder if that code is correct when not using strict mode:
I'm asking because if I an component unmounts the destroy func is not invoked:
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:
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?