How to fix linter error in useEffect

arrays I not primites I know… so how can I fix the linter error:
missing dependency params (exhaustive depths)

function useFetchPromiseAll(params) {
  const [data, setData] = useState([]);
  const didMount = useRef(false);
  useEffect(() => {
    if (didMount.current) {
      const data = getFetchData(params);
      Promise.all([...data]).then((res, data = {}) => {
        for (let i = 0; i < res.length; i++) {
          data[params[i]] = res[i];
        }
        setData(data);
      });
    } else {
      didMount.current = true;
    }
  }, []);
  return data;
}
const Base = "https://jsonplaceholder.typicode.com/";

const getFetchData = (params) => {
  const data = params.map((param) => {
    return fetch(Base + param).then((res) => res.json());
  });
  return data;
};

As I understand params is just an array of strings. In that case you can use JSON.stringify.

Also I wanted to point out our naming conventions - currently you have three different data variables (state variable, list of urls, temp variable for loop). Don’t do this, it makes your code very confusing to read.

1 Like

First of all, you can just use an eslint ignore if you are sure you are right.

Or you could just list the dependencies

What does that have to do with it? This would be a lot easier if you told us exactly what is happening.

I seem to remember something along these lines, like it didn’t like a dependency list like:
[didMount.current]. But [didMount] should work.

I’m a little confused by your code. You declare three different variables called “data”.

const [data, setData] = useState([]);

and

const data = getFetchData(params);

and

(res, data = {}) => {

You then go on to mutate one of these (presumably the last one) with:

data[params[i]] = res[i];

Why? Why not give them different names and why mutate a parameter?

That also seems like an odd use of a ref to me, I don’t know.

I will explain:

you can just use an eslint ignore

I wanted to avoid that. I read you should not do that.

two objects can never be equal, for example, try to console []===[]. the way useEffect works is it checks the dependencies by referential equality in this case they are not equal to what they were the last time, it reruns, resets the state, reruns resets the state…

why would I put didmount in the dependency? what would that do for me? …

I did that for useEffect hook only on update… regarding changes in React 18:

To help surface these issues, React 18 introduces a new development-only check to Strict Mode. This new check will automatically unmount and remount every component, whenever a component mounts for the first time, restoring the previous state on the second mount.

https://reactjs.org/docs/strict-mode.html#ensuring-reusable-state

Im able to remove strict mode or keep strict mode and use only on update… it didnt work to only render once in strict mode and not in strict mode

it will cause an infinite loop

what am i mutating? im declaring an empty object called data, setting the params as keys and results as values, setting the state in the custom hook with it and return it to the component thats using the hook… sorry the code is confusing to you, unfortunately I do not have anyone else to work with, or collaborate with to realize it was confusing until now

@jenovs is correct, I can stringify outside the effect and pass it in the dep array and parse it in the effect and it will stop the linter complaining… though it is a lot of work…

I dont know how to make it run ONLY once in strict mode && not in strict mode && remove that linter complaint

Well, there are times when I explicitly don’t want a particular variable to cause a fire of useEffect. Sometimes you can put logic inside the useEffect to return early to cover that, though.

Make a choice though - either we’re going to use the comment and not worry about this or not and we’re going to put everything in the dependency list and fix it.

two objects can never be equal

Yes, but references can.

the way useEffect works is it checks the dependencies by referential equality …

Yes, so make sure the references are equal if the data has not changed.

why would I put didmount in the dependency? what would that do for me? …

I don’t know, you have been vague about what the exact error is and which variable is the problem so I’ve had to guess. I just remember something about getting a warning once because I put something like “obj.prop” in the dep list - I had to either put “obj” or pull the prop out into its own variable.

it will cause an infinite loop

Which variable? Show the code that causes and infinite loop and say which variable in the dep list, when removed, stops the infinite loop. I don’t understand why this information wasn’t provided from the beginning. That is debugging 101. If you provide that information, the solution may be obvious.

what am i mutating?

(res, data = {})

that variable “data” - it is being mutated here:

data[params[i]] = res[i];

You have passed in a reference. That is then mutated - its contents are changing while the reference stays the same. That is a slight anti-pattern, depending on coding philosophy. The fact that it is a parameter makes it a big no-no.

sorry the code is confusing to you, unfortunately I do not have anyone else to work with, or collaborate with to realize it was confusing until now

Always code as if someone else will be reading it. Someone else will - you in 6 months and you can’t remember why you did anything.

You have three variables with the same name in nested scopes - that is a recipe for big mistakes.

One possible mistake is being confused and putting “data” in the dep list. That would cause infinite loops because the useEffect would be causing itself to fire, because that would refer to the state variable in the outer scope.

I guess I’m also a little confused as to what you are using useRef for and why. That just looks wrong to me.

This is how I would have implemented this.

const fetchDataAsync = async (params, setData) => {
  console.log("calling fetchDataAsync...");
  const fetchPromises = getFetchData(params);
  const fetchedData = await Promise.all(fetchPromises);
  setData(fetchedData);
};

const useFetchPromiseAll = (params) => {
  console.log("calling useFetchPromiseAll...");
  const [data, setData] = useState([]);

  useEffect(() => {
    console.log("calling useEffect...");
    fetchDataAsync(params, setData);
  }, [setData, params]);

  return data;
};

You can see a working pen here.

This works fine. I don’t need that ref. The dep list is complete. (Or maybe fetchDataAsync is supposed to be in there, too.)

Now one way to cause an infinite render with this is if you call it with an unstable reference for a params list, like if you call it with:

const fetchedData = useFetchPromiseAll(["todos", "users"]);

That array literal will have a new reference on each render, causing the useEffect to refire. You can solve that by moving the array into a variable outside the component or by stabalizing the array, like wrapping it in useState or useMemo.

I was declaring a new variable with a default assignment of an empty object, yes the contents are changing while the reference stays the same and that is what I was trying to do.

declaring a response object and filling it with parameters inside the function body and returning it is a pretty normal occurance is it not?

function multiply(a, b = 1) {
  return a * b;
}

multiply(5); 

the above is acceptable, but doing the same with an object is wrong, other than that you can declare it as a constant why its that much different, so what if its mutated? it is only exisiting inside of that small scope. please provide an example of why its an anti-pattern

True, your code doesnt have the linter error, but it is running the effect twice, it’s producing the same result for me that I was getting before with the code I wrote. (Im running react 18)

This thread has my work, if you want to check it out, I appreciate your helpful advice:

declaring a response object and filling it with parameters inside the function body and returning it is a pretty normal occurance is it not?

Sure, I don’t see your point.

the above is acceptable, but doing the same with an object is wrong, other than that you can declare it as a constant why its that much different, so what if its mutated?

I don’t really understand your point. Sometimes you want to return a new reference and sometimes you don’t. But React was kind of built around the idea of immutable reference types so you have to be careful with it. Almost always with React (and redux) you don’t want to mutate reference types - if you change them, you create a new one. Of course, you can also memoize the data in certain cases. That is a much more elegant option that stringifying things, imho.

True, your code doesnt have the linter error, but it is running the effect twice,

Not when I run it:

calling useFetchPromiseAll...
fetchedData Array(0)
calling useEffect...
calling fetchDataAsync...
calling useFetchPromiseAll...
fetchedData Array(2)

This does exactly what I would expect, the bare minimum. The custom hook gets called twice (as expected, it will be called on every render, it has to). The first time it fires the useEffect, the second time it doesn’t because the references to setData and params haven’t changed. Notice that “calling useEffect…” only shows up once. This is exactly what we want, I assume.

This is also running on React 18, not that it should matter much, if at all.

Like I said, for me it runs twice. your code does the same thing that was happening for me with my original code.

you can see it in the network tab as well:

I didnt like that it was running twice is why I used the ref. I would have removed that if I was going to run in production.

I dont understand, what your saying:

const obj={'a':6}
const somefunction=((obj={})=>{
    obj.a=-1000
    console.log('inner scope',obj)
})
somefunction()

console.log('outer scope',obj)

//VM1002:4 inner scope {a: -1000}
//VM1002:8 outer scope {a: 6}

the above is what I was doing in my code. Im not referencing obj in the outer scope or mutating it. that is es6 default assignment , Ive seen others use it as well to declare empty objects, no matter what i do in the inner scope im not messing with the object in the outer scope so how am I mutating the outer object or any other object for that matter other than the one i declared in the scope? Im changing the object that I declared as a default parameter and then using that to set the state… which I still dont see why its bad as it only exists in a few lines of code and then its garbage collected. is it not?

I believe it will runs twice on component mount, is my understanding of it.

Like I said, for me it runs twice. your code does the same thing that was happening for me with my original code.

I was able to reproduce in a CRA app. Looking into it more it looks like that is a side effect of StrictMode since version 18.

I didnt like that it was running twice is why I used the ref. I would have removed that if I was going to run in production.

Is this creating a problem? If not, then ignore it. If it is causing problems with your app, I’d rather remove the StrictMode container (probably in the root index.js) than start monkeying around with the code. The “I’ll just use this code for dev but get rid of it for prod” is a classic setup line for a bug, or several. On the rare chance I do do that, I put it behind an environment flag and then run a unit test that tests both environments.

And please don’t put pictures of code. It makes it much more difficult to help.

the above is what I was doing in my code. Im not referencing obj in the outer scope or mutating it…

Yes, you are. Even if you weren’t, you are committing the sin of variable name shadowing - it is a code smell - given enough time, it will lead to a bug. I know what you are saying, “It’s just my project, I know what I mean.” First of all, as we say in music, “The way you practice is the way you perform.” Secondly, if I were doing a code review for a hiring candidate, and I saw that, it would definitely be a checkmark against them. The point isn’t to write code that is clear and makes sense to you, it’s the rest of your team that matters.

OK, let’s ignore the shadowing. You are mutating an input parameter, another major code smell, that will lead to buggy, difficult to maintain bug (I speak as someone who has had to spend hours tracking down a few bugs because someone did exactly this - once it was even me that did it).

Ive seen others use it as well to declare empty objects

The point isn’t that you are declaring an empty object. The point is that you are defaulting. OK, there are definitely cases where you want to default to an empty object - I literally did it twice yesterday at work. That is not the issue.

The issue is that you are not testing this properly:

somefunction()

You didn’t pass in the reference. So, your function sees and undefined first parameter and uses the default. It creates an empty, anonymous object, mutates it, logs it, then throws it away. In that case, what is the purpose of that function? The more meaningful way to test this code is:

somefunction(obj)

What is the purpose of that function? To mutate the object? In that case, the default parameter makes no sense. Even if you wanted to write a mutating function, this wouldn’t work.

One of the principles of functional programming is that functions should not have side effects. Mutating parameters is a side effect. This will cause bugs. It will also cause things like React and Redux (and possibly other things) to not work properly.

Last month, I had to write some code in the service layer, the part of the app that does the fetching from the backend. I found that the backend was sending all the properties in snake_case. I hate snake_case for variable names in JS so I wrote a function that could transform the object to change have camelCase property names:

const originalObj = {
  some_prop: 0,
  some_other_prop: 1,
  some_other_prop2: 2,
  last_prop_i_promise: 3,
}

const convertObjPropsFromSnakeToCamel = oldObj => {
  const newObj = {}
  
  for (oldProp in oldObj) {
    const words = oldProp.split('_')
    const newProp = words[0].toLowerCase() + words.slice(1).map(w => w.charAt(0).toUpperCase() + w.slice(1).toLowerCase()).join('')
    newObj[newProp] = oldObj[oldProp]
  }
  
  return newObj
}

const convertedObj = convertObjPropsFromSnakeToCamel(originalObj)

console.log('originalObj...')
console.log(JSON.stringify(originalObj, null, 2))

// {
//   "some_prop": 0,
//   "some_other_prop": 1,
//   "some_other_prop2": 2,
//   "last_prop_i_promise": 3
// }

console.log('convertedObj...')
console.log(JSON.stringify(convertedObj, null, 2))

// {
//   "someProp": 0,
//   "someOtherProp": 1,
//   "someOtherProp2": 2,
//   "lastPropIPromise": 3
// }

Note, this isn’t the actual function - that was written in TypeScript and used a bunch of lodash, but this is the basic idea.

The important point here is that my function does not mutate the input parameter. i create a new one and manipulate that, and then return the new object. The original object is completely unaffected.

Why did I do this? Did it matter? I was just using this in the service layer and the original object wouldn’t be used anywhere else. What was the harm of mutating it?

None, really. Except that it is a really bad habit to get into, a violation of some basic principles. (Plus, I think our linter or sonar would start setting off alarms.) But what if … what if someone, somewhere else in the code, saw that function in the utils folder and decided to use it somewhere else. What if they assumed (as the should be able to) that it would not mutate the input parameters. Or, what if they saw that it did and didn’t care but then 6 months later someone else came along …

Just write code following basic principles. Don’t argue about them, just accept them. Read Clean Code. Read blogs. Unless you want to work alone forever, learn to write clean code. That is one of the most important things I judge code by. A lot of people can write code that works. I like working with people that write code that is easy to understand and maintain.

One of the nicest compliments I received recently was from a junior developer that did a code review on one of my PRs. He hung back after our standup and said, “I have to say, that PR was beautiful. It was a confusing task and I was worried but your code was so easy to understand.” I had to hide a blushing smile that was trying to burst out of my face. That is what I always try to do. I did it without a single code comment. It was all just logically organized code and good variable names. Those are two of the hardest things to teach but are two of the most important things.

Im changing the object that I declared as a default parameter and then using that to set the state…

OK, I’m confused.

So, you’re saying that for this:

Promise.all([...data]).then((res, data = {}) => {

You’re saying that the callback for Promise.all().then will never have a second parameter so it will always create a new object?

Maybe that is true, maybe then will never pass a second parameter - I can’t remember. But even if that is true, this is not how you declare variables. That is even a bigger sin than what I thought you were doing. That is so much worse than what I thought you were doing. That is just a mess. The purpose of a parameter list is to pass parameters. The purpose of a default parameter is a fallback in case the parameter is undefined. The purpose of these is not to declare new variables (not in that sense).

FFS, this is exactly what I am talking about. Now I think I understand what you were trying to do. The way you wrote your code was deceptive. If you’d written const data = {}; as the first line of that callback, instead of that rube goldberg in the parameters list, it would have made perfect sense what you were doing. Instead, you created a riddle. You combined two tools to do something neither was intended to do instead of using the one tool that was specifically designed to do exactly this. Good code tells a story, it tells you what it is doing. That code lied to me. It lies to everyone that reads it.

If you saw other people doing this, you either misunderstood what they were doing, or they were very bad coders.