React: Warning for 'exhaustive-deps' keeps asking for the full 'props' object instead of allowing single 'props' properties as dependencies

[cbdeveloper]

Do you want to request a feature or report a bug?

BUG (possible) in eslint-plugin-react-hooks

What is the current behavior?

When I'm in CodeSanbox using a React Sandbox I can use single properties of the props object as dependencies for the useEffect hook:

Example 1:

useEffect(()=>{
    console.log('Running useEffect...');
    console.log(typeof(props.myProp));
  },[ ]);

The example 1 gives me the following warning in CodeSandbox environment:

React Hook useEffect has a missing dependency: 'props.myProp'. Either include it or remove the dependency array. (react-hooks/exhaustive-deps) eslint

And if I add [props.myProp] to the array, the warning goes away.

But the same example 1 in my local environment in VSCode, I get the following warning:

React Hook useEffect has a missing dependency: 'props'. Either include it or remove the dependency array. However, 'props' will change when any prop changes, so the preferred fix is to destructure the 'props' object outside of the useEffect call and refer to those specific props inside useEffect.eslint(react-hooks/exhaustive-deps)

What is the expected behavior?

I would expect that I would get the same behavior that I get on CodeSandbox in my local environment in VSCode.

But, if I add [props.myProp] to the array, the warning DOES NOT go away. Although the code works as intended.

What could be happening? Does CodeSandbox uses a different version of the plugin? Is there any configuration I can make to change this behavior?

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

Versions I'm using:

DEV:

"eslint": "^5.10.0",
"eslint-plugin-react": "^7.11.1",
"eslint-plugin-react-hooks": "^1.6.1",

REGULAR

"react": "^16.8.6",
"react-dom": "^16.8.6",

VSCODE (probably not causing this issue)

Version: 1.32.3 (user setup)
Commit: a3db5be9b5c6ba46bb7555ec5d60178ecc2eaae4
Date: 2019-03-14T23:43:35.476Z
Electron: 3.1.6
Chrome: 66.0.3359.181
Node.js: 10.2.0
V8: 6.6.346.32
OS: Windows_NT x64 10.0.17763

.eslintrc.json

{
  "root"  :true,
  "env": {
    "browser": true,
    "commonjs": true,
    "es6": true,
    "node": true
  },
  "extends": [
    "eslint:recommended",
    "plugin:react/recommended",
    "plugin:import/errors"
  ],
  "parser":"babel-eslint",
  "parserOptions": {
    "ecmaVersion": 2018,
    "sourceType": "module",
    "ecmaFeatures": {
      "jsx":true
    }
  },
  "plugins":[
    "react",
    "react-hooks"
  ],
  "rules": {
    "react/prop-types": 0,
    "semi": "error",
    "no-console": 0,
    "react-hooks/rules-of-hooks": "error",
    "react-hooks/exhaustive-deps": "warn"
  },
  "settings": {
    "import/resolver" : {
      "alias" : {
        "map" : [
          ["@components","./src/components"],
          ["@constants","./src/constants"],
          ["@helpers","./src/helpers"]
        ],
        "extensions": [".js"]
      }
    },
    "react" : {
      "version": "detect"
    }
  }
}

[cbdeveloper]

I understood what was going on. It's not a bug (I think).

This code:

useEffect(()=>{
  function someFunction() {
    props.whatever();                  // CALLING A FUNCTION FROM PROPS
  }
},[ ]);

Results in this warning:

React Hook useEffect has a missing dependency: 'props'. Either include it or remove the dependency array. However, 'props' will change when any prop changes, so the preferred fix is to destructure the 'props' object outside of the useEffect call and refer to those specific props inside useEffect. (react-hooks/exhaustive-deps)eslint

---

And this code:

useEffect(()=>{
  function someFunction() {
    props.whatever;                          // ACCESSING A PROPERTY FROM PROPS
  }
},[]);

Results in this warning:

React Hook useEffect has a missing dependency: 'props.whatever'. Either include it or remove the dependency array. (react-hooks/exhaustive-deps)eslint

---

I'm not sure why, but the plugin see it differently when you're calling a method from props or if your acessing a property from props.

[Admsol]

The warning is pretty explicit , you should destructurate your props; since a re-render is made after a prop is changed

Please try

const myProp = props.myProp
useEffect(()=>{
    console.log('Running useEffect...');
    console.log(typeof(myProp));
  },[myProp );

[cbdeveloper]

@Admsol Yes, if you destructure it, it works. But I'm not a big fan of props destructuring. I like to always see that I'm accessing the props object anywhere.

I ended up assigning the method to a local variable (inside the useEffect) before calling it. Like:

useEffect(()=>{
  const whatever = props.whatever;
  whatever();
},[props.whatever]);

This way I get the proper warning (if I omit props.whatever from the array).

Otherwise, if I call props.whatever() directly, the "dependency omission" warning will be for the full props object instead of the props.whatever method.

Thanks!

[gaearon]

The reason plugin sees it differently is because by doing props.whatever() you are actually passing props itself as a this value to whatever. So technically it would see stale props.

By reading the function before the call you’re avoiding the problem:

const { whatever } = props;

useEffect(() => {
  // at some point
  whatever();
}, [whatever]);

This is the preferred solution.

Note: If whatever itself changes on every render, find where it is being passed down from, and wrap it with useCallback at that point. See also our FAQ.

[GeoMarkou]

Is there no better way to avoid this warning? @cbdeveloper 's solution works, but it feels like I'm going out of my way to change code just to please the linter, and no actual bugs are being prevented. If anything it makes the code worse, because it's no longer obvious that whatever belongs to the parent component. I can't imagine a situation where someone would use this inside props.whatever to access the other props?

[GeoMarkou]

Ok, after some learning on the horrible way that this works in JavaScript, I found a solution that fits my needs. To avoid destructuring the props object you have to explicitly call the function supplying your own this argument, otherwise it implicitly passes props. It even plays nicely with Typescript.

// Before
React.useCallback((event) => {
    props.onChange(event);
}, [props]);

// After
React.useCallback((event) => {
    props.onChange.call(undefined, event);
}, [props.onChange]);

[cbdeveloper]

This is still bugging me.

I've been using the following pattern to build some forms:

• I have a component to hold the state for the form
• The state is an object to hold details for a blogPost, for example.
• Each each child must access it and change
• So I'm passing down the setFormState method from the parent as props to all its child components.
• Each child component is responsible for its own function to update the state. So I lot of them do something like this:

function SomeTextInput(props) {

  const updateInput = useCallback((newInputvalue) => {
    props.setFormState((prevState) => {          // CALL 'setState' STRAIGHT FROM 'props'
      return({
        ...prevState,
        inputName: newInputvalue
      });
    });
  },[props.setFormState]);   // AND I GET THE ERROR ASKING FOR THE FULL 'props' INSTEAD OF THE METHOD THAT I'M CALLING

  return (
    <SomeInput/>
  );
}

And since I'm calling a method from props. I keep getting the error asking for the full props object instead of the method.

I don't think there's anything wrong with my pattern. I like to be explicit when I access something from props. I think it helps to know immediately that that property or method is coming from above in the tree. But I'm also opened to learn something new and useful if you guys can help me out.

What I end up doing is:

  const updateInput = useCallback((newInputvalue) => {
   const AUX_setFormState = props.setFormState;              // CREATE SOME 'aux' VARIABLE
    AUX_setFormState((prevState) => {
      return({
        ...prevState,
        inputName: newInputvalue
      });
    });
  },[props.setFormState]);

And the error goes away. But just like @GeoMarkou said, it feels like I'm going out of my way just to please the linter.

@gaearon I see that props becomes the this in the call. But how would I get stale props if I'm adding the specific props.method that I'm using to the dependency array of my useCallback?

Is there a way to "fix" this (not sure I can call this a bug)? I would vote to reopen this issue, if this fix is possible.

[GeoMarkou]

I also vote to re-open the issue if it's possible to fix. My co-workers are all ignoring this warning because there's no convenient way to make it happy, especially if you're calling several props methods.

[cbdeveloper]

@GeoMarkou I re-opened the issue. From time to time, this problem bugs me again.

[dacioromero]

This isn't only affecting props.

I have several hooks which return objects which have memoized callbacks (such as input controllers) and I would prefer not to destructure since renaming the destructured values gets very tedious.

Definitely would appreciate a solution. Maybe could be an option to ignore this specific type of warning?

[PezCoder]

I think the explanation the @gaearon https://github.com/facebook/react/issues/16265#issuecomment-517518539 mentioned totally makes sense & the plugin is completely right on throwing the error. But for cases where we know the function isn't working with this & that it won't change, this just executes the effect more than what is required.

Keeping the warnings open isn't an option in such cases, as follow up changes would require attention to what warnings to actually fix & what to let it be.

Since the purpose of plugin is to help developer avoid mistakes when using useEffect, a graceful way to skip these would really help. 🙏🏼

[lewisl9029]

The reason plugin sees it differently is because by doing props.whatever() you are actually passing props itself as a this value to whatever. So technically it would see stale props. By reading the function before the call you’re avoiding the problem.

I'm struggling to think of a case for which calling a function that depends on this (whatever in your example) and then passing this (props in your example) into the dependency list _wouldn't already be ridiculously error prone_.

By using a function that depends on this, you're likely also depending on in-place mutation of some internal state within this (i.e. a stateful class instance), which means even if this was passed into the dependency list, internal state changes within this would not trigger a rerun of the hook, which would likely lead to a whole class of bugs.

Is that really the use case we want to optimize for here? It honestly seems like an exercise in futility to me, so I would prefer if we just assumed that these functions don't depend on this and make these more-likely-to-be-correct usages of hooks more ergonomic.

[gaearon]

Is there a problem with destructuring those early? That’s the solution.

[lewisl9029]

It's definitely possible to destructure earlier, but in a lot of cases that requires introducing intermediate aliases to avoid name clashing, when the things we're trying to access is already conveniently namespaced.

In my case I'm trying to use the newly released hook API for Rifm: https://github.com/realadvisor/rifm#hook

The code I want to write looks something like this:

  const rifm = useRifm({
    value,
    onChange: setValue,
  });

  const onChange = React.useCallback(
    () => rifm.onChange(),
    [rifm.onChange],
  );

The code I end up having to write due to the rule looks like this:

  const { onChange: onChangeRifm, value: valueRifm } = useRifm({
    value,
    onChange: setValue,
  });

  const onChange = React.useCallback(
    () => onChangeRifm()
    [onChangeRifm],
  );

And this is a fairly simple case where I'm using only 2 args. Hopefully you can see how this could get quickly out of hand with more.

One of the major selling points for hooks is better developer ergonomics. And I'd really like to see us optimize for ergonomics here over a usage pattern that appears to be fundamentally error-prone with hooks to begin with (using hooks with stateful objects that mutate state internally and never change reference).

[laurent22]

I'd like to see a fix for this too. The advice to destructure is odd as it's like getting rid of a very useful and meaningful namespace. It's like doing using namespace std; in C++, which is bad practice.

I get the feeling that this advice to destructure props is going to lead to poor quality code. Perhaps we'll soon even see an eslint rule to disallow props destructuring.

[stale[bot]]

This issue has been automatically marked as stale. If this issue is still affecting you, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize it yet. If you have any new additional information, please include it with your comment!

[laurent22]

This is still a problem as far as I know. This rule is not usable in many contexts (personally I had to disable it) as it's often not practical to destructure props at the top level of a component.

[zzzachzzz]

Another use case affected by this is when using useReducer for state in a component. It's common to use a single state object with useReducer, making it necessary to destructure since the following is illegal:

const [state, dispatch] = useReducer(reducer, { key: 'value' });

useEffect(() => {
  console.log(state.key);
}, [state.key]);  // Illegal

The problem is exacerbated when dealing with complex state values (a common reason for using useReducer over useState), resulting in nested destructuring being necessary:

const [state, dispatch] = useReducer(reducer, { filters: { date: { start: '7/22/19', end: '7/23/19' } } });

const { filters: { date: { start: filterStartDate, end: filterEndDate } } } = state;

useEffect(() => {
  console.log(filterStartDate, filterEndDate);
}, [filterStartDate, filterEndDate]);

[zzzachzzz]

I hope I'm not misunderstanding the issue. I noticed this snippet in the React docs today that passes props.friend.id into a dependency array without destructuring: https://reactjs.org/docs/hooks-effect.html#tip-optimizing-performance-by-skipping-effects
Relevant snippet from docs:

useEffect(() => {
  function handleStatusChange(status) {
    setIsOnline(status.isOnline);
  }

  ChatAPI.subscribeToFriendStatus(props.friend.id, handleStatusChange);
  return () => {
    ChatAPI.unsubscribeFromFriendStatus(props.friend.id, handleStatusChange);
  };
}, [props.friend.id]); // Only re-subscribe if props.friend.id changes

[dacioromero]

@zzzachzzz The specific issue I've found is with functions because of this.

See: https://github.com/facebook/react/issues/16265#issuecomment-587977285

[gaearon]

@zzzachzzz

Another use case affected by this is when using useReducer for state in a component. It's common to use a single state object with useReducer, making it necessary to destructure since the following is illegal:

const [state, dispatch] = useReducer(reducer, { key: 'value' });

useEffect(() => {
 console.log(state.key);
}, [state.key]);  // Illegal

This definitely sounds like a misunderstanding. This code is perfectly legal, and it passes the linter. Maybe you have a bad version of the plugin with a bug, so try updating to the latest.

[gaearon]

It's like doing using namespace std; in C++, which is bad practice.

I don't think this is an accurate comparison. Destructuring props doesn't bring random things in scope. It only brings what you explicitly specify there. Since props already have to be uniquely named, destructuring props or not is a completely stylistic difference, and we're going to recommend destructuring as the default pattern (you're welcome to disagree, but the linter is optimized for the recommended convention).

[gaearon]

@lewisblackwood

It's definitely possible to destructure earlier, but in a lot of cases that requires introducing intermediate aliases to avoid name clashing, when the things we're trying to access is already conveniently namespaced.

I empathize with this use case and I can see how it could be frustrating. I hope you also see where we're coming from here. In JavaScript, rifm.onChange() is the same as rifm.onChange.call(rifm), and rifm becomes an implicit argument. Sure, you personally may not write code this way, but it's easy to rely on this (imagine rifm object were a class instance), and then the value of this will be stale. The purpose of the linter is to prevent such mistakes, so we don't want to let them through.

Regarding your example, you can avoid the problem by returning an array, just like built-in Hooks:

const [name, handleNameChange] = useRifm(...)
const [surname, handleSurnameChange] = useRifm(...)

Your code only has two return values, so it seems like a good fit and would solve the issue. It would also minify better because instead of x.onChange and y.onChange you would just have x and y.

[gaearon]

Oops, didn't mean to close.

I'm curious to hear if people who commented so far still find this to be a significant issue, or if they have mostly moved onto other patterns (e.g. destructuring props).

I think an argument could be made that we should simply err on the side of this case being confusing. But I want to be clear that this is an example where somebody will definitely spend days tearing their hair out because of this decision.

[lewisl9029]

I empathize with this use case and I can see how it could be frustrating. I hope you also see where we're coming from here. In JavaScript, rifm.onChange() is the same as rifm.onChange.call(rifm), and rifm becomes an implicit argument. Sure, you personally may not write code this way, but it's easy to rely on this (imagine rifm object were a class instance), and then the value of this will be stale. The purpose of the linter is to prevent such mistakes, so we don't want to let them through.

I can definitely see how this could _in theory_ result in bugs. But I'm still honestly struggling to think of a practical use case that depends on receiving a fresh instance of a class (this) on some renders, but wouldn't also work when depending on the properties of that instance (which should also change on those renders with the new class instance, unless it's a primitive value, in which case the class instance doesn't even matter at all).

Let's say in the Rifm example, useRifm returned a new class instance every time it's called:

  const rifm = useRifm({
    value,
    onChange: setValue,
  });

  const onChange = React.useCallback(
    () => rifm.onChange(),
    [rifm.onChange],
  );

Wouldn't the above code still work exactly as intended since rifm.onChange would also have a different reference every time we get a new instance of rifm? And calling that instance of rifm.onChange would call it with the new rifm instance as this.

On the other hand, if someone's using a class, they likely are using it for the inherent statefulness, so would likely also be returning the same class instance every render, but potentially with properties/methods mutated internally. This would mean that the reference to rifm.onChange might change but the rifm instance itself would remain the same, which also doesn't result in any stale this values since this never changes. Ignoring the inherent incompatibility of this pattern with React's reactive model that depends on using built-in component state primitives to trigger rerenders, I'm actually not seeing a bug that's prevented by the rule in this case either.

Maybe I'm still missing something here, but what is the _real_ bug that this rule is guarding against? Are we imagining a use case where a class's this can change reference but this.someMethod won't? Is that actually a realistic enough possibility in practice to be worth guarding against?

[GeoMarkou]

@gaearon
I've started calling props.someFunction.call(undefined, someArguments) to avoid this issue. It's slightly annoying since I never rely on this in any functions, but it's a fairly easy habit to get into. I'm still noticing odd situations with the ESLint rule though.

The following snippet has the warning React Hook React.useEffect has a missing dependency: 'someObj'.. It asks for the whole object when I'm only using one field.

const someObj = { field: 1 };
React.useEffect(() => {
    someObj.field = 2;
}, []);

The following snippet has the warning React Hook React.useEffect has missing dependencies: 'possiblyNullObject.subObject.enabled' and 'possiblyNullObject.subObject.field'.. This one errors when possiblyNullObject is null in the dependency array but it's work-aroundable by using optional chaining everywhere instead of just the first IF check.

const possiblyNullObject = {
    subObject: {
        enabled: false,
        field: 1
    }
};

React.useEffect(() => {
    if (possiblyNullObject?.subObject.enabled) {
        console.log(possiblyNullObject.subObject.field);
    }
}, []);

In my package.json I'm using "eslint-plugin-react-hooks": "^4.0.8"

[gaurav5430]

the destructuring pattern should be used for other objects as well ?

For example, I am using history.push in my effect, and the eslint rule complains that I have a missing dependency on history . I can add history to the dependency array, but that leads to a problem that the rule does not complain anymore for any nested properties from history which might change while history itself remains the same object reference.

useEffect( () => {
    // do something with history.location.hash and history.push
}, [history])

now the rule does not complain that we should have an explicit dependency on history.location.hash even though it can change even though history has the same object reference.

Here history is from react router.

If de-structuring should be used in this case, is there a way for the linter to complain? same as it does for the props object ?

[gaearon]

It asks for the whole object when I'm only using one field.

This is because you’re assigning to that object. It doesn’t make sense to depend on a field you assign to since you never read it — it’s the surrounding object that matters for assignment.

but that leads to a problem that the rule does not complain anymore for any nested properties from history which might change while history itself remains the same object reference.

The rule assumes you’re working with immutable objects that originate in render. (Or mutable objects that originate outside of it.) useEffect won’t rerun it some random mutable fields on the history object change because React would not be notified about that mutation. So they don’t make sense as dependencies anyway.

[gaurav5430]

The rule assumes you’re working with immutable objects that originate in render. (Or mutable objects that originate outside of it.) useEffect won’t rerun it some random mutable fields on the history object change because React would not be notified about that mutation. So they don’t make sense as dependencies anyway.

Thanks for clarifying this.
For any future readers please see the discussion in https://github.com/facebook/react/issues/19636 as it might be relevant.

[seiyab]

I'm also struggling this behavior.
I understand https://github.com/facebook/react/issues/16265#issuecomment-517518539 https://github.com/facebook/react/issues/14920#issuecomment-467494468 and I agree that this is NOT a bug at all but an intended behavior.

However, it's also true that some people feel this behavior is inconvenient and makes the rule useless.
So I suggest configurations like below and I wonder if this is what people struggling this behavior need.

{
  "rules": {
    // ...
    "react-hooks/exhaustive-deps": ["warn", {
      "ignore-implicit-this-dependency": true
    }]
  }
}

or

{
  "rules": {
    // ...
    "react-hooks/exhaustive-deps": ["warn", {
      "ignore-props-this": true
    }]
  }
}

[dwelle]

Is there a problem with destructuring those early? That’s the solution.

The problem is Typescript's discriminated unions:

(simplified example)

type Obj = { isReady: false, callMeMaybe: null } | { isReady: true, callMeMaybe: () => void };

const [value, setValue] = useState<Obj>({ isReady: false, callMeMaybe: null });

const {isReady, callMeMaybe} = value;
useEffect(() => {
    if (isReady) {
        callMeMaybe(); // TS error, might be null
    }
}, [isReady, callMeMaybe]);

With destructuring we have to null-check everything.

[stefcameron]

I hit this right away, having just added the rule to my lint rules.

In a component where I have many props, some which are targeted to internal components I'm forwarding to, others targeted to a wrapper, and others which are the "main" props, destructuring is not something I'm happy with as a workaround to the rule. If I destructured everything, I'd have a complete mess in the code, not knowing what comes from what and goes to what. The namespace is definitely useful, and I'm not a fan of destructuring + renaming to re-add the namespace I had before. That's a whole lot of syntax to write to make the linter happy.

__However__, the more I read comments here, and when I finally saw what the recommended workaround was, and more importantly, _why_ (because of the this in context), I started to understand the reasoning. It's definitely a contentious issue -- I agree -- but I would say that _technically_, the rule is right (even though I don't like it!!).

When I pass a function to an event prop like onWhatever, I'm doing so with the assumption that it's going to get called without context. So I'll write the implementation of that function without using this and regardless of how it's called, I won't care. That's one side of the argument: Who cares, right?

And if I did care, then I'd be sure to pass handleWhatever.bind(something) to make sure I get the context I want regardless of how that function is called.

But internally, on the implementation that calls the event handler, calling props.onWhatever() _technically_ breaks the contract (the assumption of calling without context).

And because there's that contract in place, I'm convincing myself that the rule is actually correct the way it is.

To avoid destructuring or assigning in the outer scope, I'll prefer this syntax:

useEffect(function () {
  const onWhatever = props.onWhatever;
  if (typeof onWhatever === 'function') {
    onWhatever();
  }
}, [props.onWhatever]);