React: [react-hooks/exhaustive-deps] Does not complain for nested dependencies if the parent object is added in dependencies array

[gaurav5430]

React version: 17.0.0-rc.0

Steps To Reproduce

1. add a useEffect which does something with history.location.hash and history.push
2. The eslint hook complains that you should have a dependency on history. I understand that is because we are passing history as this to the push function
3. If you add history to dependencies, it will stop complaining, although history.location.hash can change without history object itself being changed.

Link to code example:
https://codesandbox.io/s/react-router-usehistory-hooks-forked-2p5mk?file=/src/App.js

The current behavior

• lint rule does not complain or suggest anything related to destructuring the nested properties, or adding them explicitly after the top level object is added

The expected behavior

• if possible, it can show similar suggestions as it does when the whole props object is specified as a dependency

[gaearon]

The whole notion of dependencies rests on the assumption that the values you're specifying are immutable and are created during rendering. Essentially, you're saying the linter doesn't handle code like this:

let obj = {}
setTimeout(() => {
  obj.foo = 'bar'
})

function MyComponent() {
  useEffect(() => {
    console.log(obj.foo);
  }, [obj]);
  return null;
}

But there's no way it could work anyway. The React component wouldn't know that this object could be mutated.

In my example, the lint rule would actually not suggest adding obj at all because it's clearly declared outside of render scope. But if it were passed from a parent component, the rule would have to be conservative and assume it might be something from the render scope. In React, things declared in render scope are supposed to be immutable (unless they're refs). This is why we can assume that obj.foo can't change without obj changing, in the general sense.

Hope that makes sense.

[gaurav5430]

But there's no way it could work anyway. The React component wouldn't know that this object could be mutated.

Just to clarify, by this you mean that the react component would not re-render if the object is mutated, because it can only re-render based on props or state, and so the effect would not re-run anyway ?

This makes sense and helps with better understanding of how the lint rule works, but does it then mean that we shouldn't be adding those dependencies in deps array?

Is it possible for you to explain it in context of the history example?

when history.location.hash changes, the component re-renders because of the Router. It then calls this effect and the effect checks if any of the dependencies has changed.
In my case, if i specify history.location.hash as a dependency, the hook correctly recognizes that it has changed and re-runs the effect.

I get history from react router in the render scope, so the lint rule is correct in assuming that history object is immutable and it wouldn't expect any nested property in history to be updated without history object itself being re-created.

But I should still add this as an extraneous dependency? If yes, then it means that sometimes the lint rule does miss out on some dependencies which have to be manually added? Do we have other examples of this?

_sidenote:_ If I remove history.push usage , then the lint rule complains that history.location.hash should be specified in dependencies, (And not history) which leads me to believe that it is indeed a dependency, just that the lint rule is not able to automatically add it in case the parent object has already been added. Which is similar to the flow when we specify the whole 'props' object as dependency, and the rule suggests de-structuring. Can that not be applicable here?
Or is that a general guideline to be followed that if you are using an object in dependencies, for which the nested properties might change without the object itself changing, you might need to add extraneous dependencies yourself to the deps array.

[gaearon]

Just to clarify, by this you mean that the react component would not re-render if the object is mutated, because it can only re-render based on props or state, and so the effect would not re-run anyway ?

Yes.

when history.location.hash changes, the component re-renders because of the Router.

This sounds somewhat fragile. If it re-renders because some router prop changed, then you should use that prop/context to determine "where you are" rather than reading it from a mutable object.

If I remove history.push usage , then the lint rule complains that history.location.hash should be specified in dependencies,

The lint rule can't guess we're dealing with a mutable object in this case, so it assumes this is a regular piece of data.

Or is that a general guideline to be followed that if you are using an object in dependencies, for which the nested properties might change without the object itself changing, you might need to add extraneous dependencies yourself to the deps array.

If you're using a mutable object in dependencies, specifying its properties as dependencies is going to lead to confusing behaviors because you can never be sure whether it was updated "just in time" before a React render or slightly afterwards (in which case you'll "miss" the chance to respond to it).

In general, reading mutable data during render (which is what you're doing when you read history.something to put it in dependency list) is a fragile pattern.

[gaurav5430]

This sounds somewhat fragile. If it re-renders because some router prop changed, then you should use that prop/context to determine "where you are" rather than reading it from a mutable object

Oh ok, so in this case, it would make sense to use location.hash in the dependency array as location prop is used to determine (and suggested way) where the user is right now. So, the dependency would now be on something that React component knows has changed and the effect re-runs.

If you're using a mutable object in dependencies, specifying its properties as dependencies is going to lead to confusing behaviors because you can never be sure whether it was updated "just in time" before a React render or slightly afterwards (in which case you'll "miss" the chance to respond to it).

Right, we can only respond to a change in the mutable object property if it is paired with either a prop or state update which leads to re-rendering the component. (A similar example to why class component does not re-render if you just change an instance variable, unless it is paired with changing a state variable/prop as well)

I have a follow up question. Feel free to ignore if it is unrelated, or let me know if a new issue would make sense for that.
Let's say I correctly defined the dependencies and added location.hash as the prop which should be watched. Now, inside my effect i am changing location.hash by pushing to history. This would lead to a re-render of the component and hence re-evaluating the hooks deps, and since the location.hash has now changed due to history.push, the hook would re-run.

In this case, the new location.hash would be equivalent to what we wanted to push in history, we can probably have a check that we should not push if the hash is already present.

But, in my actual use case, I remove the hash when the hook cleans up, which leads to changing location.hash again and hook re-runs which again tries to add the same hash. This leads to a cyclic dependency (kinda?) and infinite update.

https://stackoverflow.com/questions/63462671/react-hooks-useeffect-exhaustive-deps-cyclic-dependency-on-location-hash/63466873

I understand that if there are multiple dependencies, then you might have to write your effect in a way that you check which of the dependencies has actually changed and run code specific to that change. But in this case, I am just not sure what that condition should be, to avoid infinite render.

(Also, is this a general pattern with hooks that if there are multiple dependencies, you might need to check for what dependencies have changed and run some code accordingly?)

[gaearon]

In this case, the new location.hash would be equivalent to what we wanted to push in history, we can probably have a check that we should not push if the hash is already present.

This sounds reasonable. You can usually do something like this with refs.

In general, the guideline is that effects should be resilient to occasionally re-running. If it is super important that the code inside doesn't reexecute unless some specific case happens, it is best to code it explicitly.

[gaurav5430]

If it is super important that the code inside doesn't reexecute unless some specific case happens, it is best to code it explicitly.

By explicitly you mean not coding it as an effect? (in which case, what are some options?)
or as an effect but bypassing the exhaustive-deps rule as an exception?

[gaearon]

You don't need to bypass the rules.

const prev = useRef(value)

useEffect(() => {
  if (prev.current !== value) {
    prev.current = value;
    // execute some code here
  }
}, [value, prev])

[gaurav5430]

Right, I thought you were talking about some other use cases where we might need to be more explicit and bypass the deps rule. Say, we only want to run something on mount (which is almost always a misunderstanding and might lead to subtle bugs), then we can just enforce an empty array in the deps and bypass the rule.
I get your point about the current functionality.

In my current use case where the effect cleanup would just remove the hash from the URL, i can't check before re-adding the hash that 'only add if hash is not present', because that would always be true. So i am just not sure if I need to have a better condition or should i bypass the deps rule and remove location.hash as a dependency (which does not seem to be correct, as ideally the component might need to run the effect if location.hash changes, not the case right now in my usage, but may be in future)
I just can't figure out a better condition to check for this.

[gaearon]

I think any dependency condition that includes a mutable value is wrong. Instead it is better to include the parent object instead (the closest immutable thing) but write the body of the effect in a way that uses refs to decide what to do.

[gaurav5430]

Ohk, so in this case the mutable value is location.hash ?
and you are suggesting that the dependency should be on location instead? In which case, how would the effect re-run if location.hash changes? (assuming location is the closest immutable parent)

[gaearon]

You can remove the dependency array altogether and then it will re-run after every render. Then use refs to decide when to do things. This still has the pitfall that relying on something mutable with no notification mechanism (except rendering itself) seems like a pitfall. It would make more sense to me if you subscribed to the history and pumped the location into state.

[gaurav5430]

I don't really understand how location.hash is different from any other prop (say props.hash), when props.hash changes, we re-run the effect, is props.hash considered mutable in this case? And would it be wrong for us to have a dependency on props.hash?

[gaearon]

I don't really understand how location.hash is different from any other prop (say props.hash),

I don't know whether location you're referring to is a mutable object or not so it's hard for me to say.

[gaurav5430]

It would make more sense to me if you subscribed to the history and pumped the location into state.

you mean subscribing to history changes and based on that changing a state variable? How would that help?

[gaurav5430]

I don't really understand how location.hash is different from any other prop (say props.hash),

I don't know whether location you're referring to is a mutable object or not so it's hard for me to say.

location is immutable , it comes from react router.

https://reactrouter.com/web/api/history#:~:text=The%20history%20object%20is%20mutable,are%20correct%20in%20lifecycle%20hooks.

[gaearon]

Sorry, I got confused because browser has its own location object and I wasn't sure if you're referring to that or not.

[gaearon]

I think the answer to your question is that if you want something to only happen once, doing it with a ref is a better way than relying on dependencies.

const didTheThingRef = useRef(false);

useEffect(() => {
  if (didTheThing.ref.current) {
    return
  }
  didTheThing.ref.current = true
  doTheThing(location.hash)
}, [location.hash])

[gaurav5430]

Right, makes sense. But in my case, i don't really want to do it once.

I want the effect to run and cleanup when any of the dependencies change, but the problem is that the effect itself is responsible for changing one of the dependencies which would lead to an infinite update problem, unless we have a condition which checks whether this effect is re-running because the dependency was changed from inside this effect, which (the condition) is something that I find difficult to figure out in this case.

Thanks for your time and patience. I guess i will try to solve my use case with this information and come back with a codesandbox example.