Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

benbellick
Copy link

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!

@@ -109,6 +109,18 @@ useEffect1(effect, [|dep|])

However, as the length of the array is not specified, you could pass an array of arbitrary length, including the empty array, `[||]`.

> *NOTE*: When using `useEffectN` for `N` greater than 1, do not try to gather like types into arrays. Instead, leave each item separate in the tuple.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would explain this a bit differently:

Using useEffectN1/N2/N3/... make sure you don't wrap the dependencies in arrays. Since tuples are represented in arrays in JavaScript it would trick React into compare arrays by reference (which is often undesired).

Use the tuple given one dependency per item, even when those types are the same.

@anmonteiro
Copy link
Member

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
Copy link
Author

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
Copy link

actionshrimp commented Apr 9, 2024

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
Copy link
Member

davesnx commented Apr 10, 2024

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
Copy link

actionshrimp commented Apr 10, 2024

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:

  • useEffect0 takes 0 things
  • useEffect1 takes an array of things
  • useEffect2 takes 2 things
  • useEffect3 takes 3 things etc..

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, ...|])

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants