-
Notifications
You must be signed in to change notification settings - Fork 347
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
base: main
Are you sure you want to change the base?
Conversation
docs/components.md
Outdated
@@ -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. |
There was a problem hiding this comment.
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.
I'd probably rather just point to the React docs where this is explained: From https://react.dev/reference/react/useEffect#reference :
From https://react.dev/learn/synchronizing-with-effects#re-render-with-different-dependencies :
|
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 |
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" ])` */ |
I'm confused now |
My bad, I was typing this up in a bit of a rush and missed adding the tuple wrapper for 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,
I guess ideally it might look something like this, if this were possible:
|
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 foruseEffect1
.So, I tried to be clever by gathering like types into arrays, like so:
But this doesn't work, as the transpiled javascript code will always result in a comparison
[x,y]==[x,y]
which isfalse
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!