reasonml / reason-react

Reason bindings for ReactJS
https://reasonml.github.io/reason-react/
MIT License
3.25k stars 347 forks source link

Make explicit array comparison error in useEffectN N>1 #830

Open benbellick opened 5 months ago

benbellick commented 5 months ago

I'm working on a project using reason-react and I encountered a mistake which I found hard to solve, and I wanted to contribute to the docs in case anyone else experiences the same issue.

The gist is that my reading of the docs led me to believe that the use of arrays in useEffect are fine in general, rather than just specifically for useEffect1.

So, I tried to be clever by gathering like types into arrays, like so:

x : int
y : int
z : string
useEffect2(effect, [|x,y|],z);

But this doesn't work, as the transpiled javascript code will always result in a comparison [x,y]==[x,y] which is false always because the arrays are being compared by reference.

I'm not sure if this is the right place to plop these docs in, but please let me know and I am happy to move it around!

anmonteiro commented 5 months ago

I'd probably rather just point to the React docs where this is explained:

From https://react.dev/reference/react/useEffect#reference :

If some of your dependencies are objects or functions defined inside the component, there is a risk that they will cause the Effect to re-run more often than needed. To fix this, remove unnecessary object and function dependencies. You can also extract state updates and non-reactive logic outside of your Effect.

From https://react.dev/learn/synchronizing-with-effects#re-render-with-different-dependencies :

The dependency array can contain multiple dependencies. React will only skip re-running the Effect if all of the dependencies you specify have exactly the same values as they had during the previous render. React compares the dependency values using the Object.is comparison. See the useEffect reference for details.

benbellick commented 5 months ago

How's the change now? I'm happy to remove the extra stuff if you feel strongly. My view is that the type- ystem may incorrectly guide the user to think the useEffectN applied on an array is working because it compiles. This was a particular place I got stuck on which took some time to fix, and so I would rather include some info on the specific issue directly in the docs so as to make easiest for users.

actionshrimp commented 5 months ago

Having been confused by this in the past in the past, I think the issue stems from the progression from the singleton array -> 'a array -> 'a array + some other type. What about something like this, including the progression in the example compiled output?

useEffect1(effect, [| 1 |])
     /* ^^^ -- Compiles to javascript as `useEffect(effect, [1])` */
useEffect1(effect, [| 1, 2, 3 |])
     /* ^^^ -- Compiles to javascript as `useEffect(effect, [1, 2, 3])` */
useEffect2(effect, ([| 1, 2, 3 |], "foo"))
     /* ^^^ --- !! Compiles to javascript as `useEffect(effect, [ [1, 2, 3], "foo" ])` */
davesnx commented 5 months ago

I'm confused now useEffect2(effect, [| 1, 2, 3 |], "foo") isn't valid to compile, which is the point: https://melange.re/unstable/playground/?language=Reason&code=UmVhY3QudXNlRWZmZWN0MihlZmZlY3QsIFt8MSwgMiwgM3xdLCAiZm9vIik7Cg%3D%3D&live=on

actionshrimp commented 5 months ago

My bad, I was typing this up in a bit of a rush and missed adding the tuple wrapper for useEffect2 which I've now edited in above.

I think that mistake partly explains my initial confusion with this stuff though when I started with ReasonReact - I don't really think of tuples and arrays in the same way intuitively, even if they both compile to an array, so I hadn't appreciated that the API was using the tuple in place of the array to try and mimick how things look in javascript - I just saw the 2, and saw that you passed it 2 things.

So following that logic, useEffect1 always seemed like the odd one out to me:

I guess ideally it might look something like this, if this were possible:

useEffect0(effect)
useEffect1(effect, (1)) <-- unfortunately this isn't valid either
useEffect2(effect, (1, 2))
...
useEffectA(effect, [|1, 2, ...|])