React: Feedback on useEffect depndencies change error

[togakangaroo]

Some feedback on useEffect

What is the current behavior?

I am using useEffect to load details on a set of users that are kept in props.

I want to minimize loading and there are situations where the array of users (and the actual user objects) are recreated but really they refer to the same identities.

So I do

const userIds = users.map(u => id)
useEffect(() => {
  if(!userIds.length)
    return
  ...load more stuff by querying endpoints about these ids and set state..
}, userIds)

aand I run into a warning

The final argument passed to useEffect changed size between renders. The order and size of this array must remain constant

which to me is...kinda the point, is it not? I understand if we don't want to do a deep comparison in the dependencies, but if the dependencies themselves change...well that seems straightforward, run the effect.

I know that I can do [userIds.join(' ']) in this case, but that seems like just extra work for no reason and really anti-intuitive. And ends up doing the same exact thing but with extra steps!

To be clear, what I'm proposing is removing this whole block here and replacing it with

if (nextDeps.length !== prevDeps.length)
  return false

As far as I can tell, this warning serves no real purpose and makes the use case outlined above awkward to deal with

[universse]

Hm, what about

useEffect(
  // ....
, [ userIds.length ])

[togakangaroo]

@universse that would trigger if the ids change length only? So if I add a thing and remove a thing, it will not fetch details.

But really, its not like I can't figure out a way to hack around it - I have one above, its that I don't understand why this limitation would be there in the first place, from looking at the code, absolutely nothing seems to depend on it, if you replaced it with what I propose above it would just work.

[revskill10]

Why not just passing non zero-length userIds as params to useEffect instead ?

[universse]

oops i overly focused on this

if (nextDeps.length !== prevDeps.length)
  return false

[togakangaroo]

Yup - if you look at the code, it would seem that you can replace

    if (nextDeps.length !== prevDeps.length) {
      warning(
        false,
        'The final argument passed to %s changed size between renders. The ' +
          'order and size of this array must remain constant.\n\n' +
          'Previous: %s\n' +
          'Incoming: %s',
        currentHookNameInDev,
        `[${prevDeps.join(', ')}]`,
        `[${nextDeps.join(', ')}]`,
      );
    }

with

if (nextDeps.length !== prevDeps.length)
  return false

and then my use case would be supported and there would be no other negative effects.

So I'm asking the team what is the purpose of this warning and also why not change it?

[universse]

Anyway for your code, I feel there's a more efficient way.

const userIds = users.map(u => id)

useEffect(() => {
  if(!userIds.length)
    return
  ...load more stuff by querying endpoints about these ids and set state..
  // since you are setting state here, component will be rerendered, causing the above array.map
  // to re-run and thus userIds is re-computed every render unnecessarilty.
}, userIds)

What could be better

// so now userIds is only re-computed when users change and not whenever state is set
const userIds = useMemo(() => users.map(u => id), [users])

// here effect only re-run when userIds is recomputed, which in turn depends on users
useEffect(() => {
  if(!userIds.length)
    return
  ...load more stuff by querying endpoints about these ids and set state..
}, [userIds])

Or if you don't need to use variable userIds anywhere else

// this case, same as above, effect only re-runs when users change
useEffect(() => {
  const userIds = users.map(u => id)
  // ....
}, [users])

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

[stale[bot]]

Closing this issue after a prolonged period of inactivity. If this issue is still present in the latest release, please create a new issue with up-to-date information. Thank you!

[ngolin]

Anyway for your code, I feel there's a more efficient way.

const userIds = users.map(u => id)

useEffect(() => {
  if(!userIds.length)
    return
  ...load more stuff by querying endpoints about these ids and set state..
  // since you are setting state here, component will be rerendered, causing the above array.map
  // to re-run and thus userIds is re-computed every render unnecessarilty.
}, userIds)

What could be better

// so now userIds is only re-computed when users change and not whenever state is set
const userIds = useMemo(() => users.map(u => id), [users])

// here effect only re-run when userIds is recomputed, which in turn depends on users
useEffect(() => {
  if(!userIds.length)
    return
  ...load more stuff by querying endpoints about these ids and set state..
}, [userIds])

Or if you don't need to use variable userIds anywhere else

// this case, same as above, effect only re-runs when users change
useEffect(() => {
  const userIds = users.map(u => id)
  // ....
}, [users])

Some other props of user may be changed, but useEffect should depend only on users.map(u => u.id). How to accomplish in this case?

[ngolin]

Yup - if you look at the code, it would seem that you can replace

    if (nextDeps.length !== prevDeps.length) {
      warning(
        false,
        'The final argument passed to %s changed size between renders. The ' +
          'order and size of this array must remain constant.\n\n' +
          'Previous: %s\n' +
          'Incoming: %s',
        currentHookNameInDev,
        `[${prevDeps.join(', ')}]`,
        `[${nextDeps.join(', ')}]`,
      );
    }

with

if (nextDeps.length !== prevDeps.length)
  return false

and then my use case would be supported _and_ there would be no other negative effects.

So I'm asking the team what is the purpose of this warning and also why not change it?

We should do this. Please re-open.

[universse]

Yup - if you look at the code, it would seem that you can replace

    if (nextDeps.length !== prevDeps.length) {
      warning(
        false,
        'The final argument passed to %s changed size between renders. The ' +
          'order and size of this array must remain constant.\n\n' +
          'Previous: %s\n' +
          'Incoming: %s',
        currentHookNameInDev,
        `[${prevDeps.join(', ')}]`,
        `[${nextDeps.join(', ')}]`,
      );
    }

with

if (nextDeps.length !== prevDeps.length)
  return false

and then my use case would be supported _and_ there would be no other negative effects.
So I'm asking the team what is the purpose of this warning and also why not change it?

We should do this. Please re-open.

This doesn't solve your problem though, since users length is the same. You can consider putting users in the dep array.