Skip to content
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

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

Open
cbdeveloper opened this issue Jul 31, 2019 · 56 comments

Comments

@cbdeveloper
Copy link

cbdeveloper commented Jul 31, 2019

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
Copy link
Author

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.

@ghost
Copy link

ghost commented Jul 31, 2019

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
Copy link
Author

@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
Copy link
Collaborator

gaearon commented Aug 2, 2019

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
Copy link

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
Copy link

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
Copy link
Author

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
Copy link

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
Copy link
Author

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

@cbdeveloper cbdeveloper reopened this Oct 29, 2019
@dacioromero
Copy link

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
Copy link

I think the explanation the @gaearon #16265 (comment) 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
Copy link

lewisl9029 commented Feb 18, 2020

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
Copy link
Collaborator

gaearon commented Feb 19, 2020

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

@lewisl9029
Copy link

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
Copy link

laurent22 commented Mar 1, 2020

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
Copy link

stale bot commented Jun 17, 2020

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!

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Jun 17, 2020
@laurent22
Copy link

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.

@stale stale bot removed the Resolution: Stale Automatically closed due to inactivity label Jun 17, 2020
@zzzachzzz

This comment has been minimized.

@zzzachzzz

This comment has been minimized.

@dacioromero
Copy link

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

See: #16265 (comment)

@gaearon
Copy link
Collaborator

gaearon commented Aug 4, 2020

@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
Copy link
Collaborator

gaearon commented Aug 4, 2020

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
Copy link
Collaborator

gaearon commented Aug 4, 2020

@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 gaearon closed this as completed Aug 4, 2020
@o-alexandrov
Copy link

o-alexandrov commented Feb 23, 2022

It's clear the community shares a single wish about this issue:

  • ability to disable the check for this when invoking a function nested prop
    • instead of: props.exampleFunction.call(null, ...)
    • let us do: props.exampleFunction(...)

It would be great, if someone from the React's team either:

  • acts on the ~1.5 years old add ignoreThisDependency option to exhaustive-deps #20521
  • or closes this issue, if you are not willing to add an override to the eslint plugin
    • this way the community would know the only way to deal with it is
      • to create/use another eslint plugin that extends from eslint-plugin-react-hooks
      • or to submit to the way (destructuring, or using .call) the authors of this plugin want to see how other devs code with React

@laurent22
Copy link

It doesn't seem the PR will ever be merged unfortunately. Has anyone created a custom rule that could be used as an alternative to react-hooks/exhaustive-deps?

@seiyab
Copy link

seiyab commented May 18, 2022

I, author of #20521, published @seiyab/eslint-plugin-react-hooks on trial.
I'll publish non-alpha version after some feedback.

Edited: ↑It doesn't work correctly. I'm working with it.
Edited: 4.5.1-alpha.5 works fine in my environment. Tell me if it has trouble.

@todorone
Copy link

It's clear the community shares a single wish about this issue:

  • ability to disable the check for this when invoking a function nested prop

    • instead of: props.exampleFunction.call(null, ...)
    • let us do: props.exampleFunction(...)

It would be great, if someone from the React's team either:

  • acts on the ~1.5 years old add ignoreThisDependency option to exhaustive-deps #20521

  • or closes this issue, if you are not willing to add an override to the eslint plugin

    • this way the community would know the only way to deal with it is

      • to create/use another eslint plugin that extends from eslint-plugin-react-hooks
      • or to submit to the way (destructuring, or using .call) the authors of this plugin want to see how other devs code with React

@gaearon Could You please reiterate on this thread and possibly push forward reviewing of the PR that fixes this issue?

@derekstavis
Copy link

derekstavis commented Nov 15, 2022

Glad this is still open, because I feel like the proposed code-taxing solutions for this problem are non-solutions. Using arrays is non-intuitive, and leads to error prone code when the array has more than 2 values. It works for useState because of that, but it doesn't work when the returned value from a hook has too many values.

Example:

const useDeviceParam = (device: Device, param: Parameter) => {
  // ...

  return {
    value,
    remote,
    set,
    write,
    refresh,
    loading
  }
}

Similar to using arrays, destructuring and renaming becomes quite tedious. For example, in the code below (real production application code), destructuring + renaming makes the code significantly more difficult to read, and significantly more boring to write:

image

The other proposed solutions of assigning the function to variables also involve writing a lot of boilerplate code:

useEffect(() => {
  if (!device) return;

  const setVersion =  version.refresh;
  const setPolePairs =  polePairs.refresh;
  /* repeat for every refresh function */
 
  const refreshers = [setVersion, setPolePairs /* ... remaining refresh funcs */];
  refreshers.forEach(r => r.refresh());
  ...
}, [device, version.set, polePairs.set /* ... remaining refresh funcs */]);

I think that the proposed solution of a flag to the lint rule works, but if I can offer a better option it would be to split this lint rule into two rules, one that checks exhaustive-deps, and another that checks implicit this, e.g.: exhaustive-deps-implicit-this, which could be added/removed at convenience. The added benefit of this approach is that the developer could keep all exhaustive dependency checks running, but use a magic comment to disable exhaustive-deps-implicit-this whenever it's not-applicable. Right now my code is ignoring completely dependency checks, which can lead to bugs:

useEffect(() => {
  if (!device) return;
  resistance.refresh();
  // eslint-disable-next-line react-hooks/exhaustive-deps
}, [resistance.refresh]); // device is missing but should not be

with the proposed solution this would not happen:

useEffect(() => {
  if (!device) return;
  resistance.refresh();
  // eslint-disable-next-line react-hooks/exhaustive-deps-implicit-this
}, [resistance.refresh]); // react-hooks/exhaustive-deps warns about device missing 

@laurent22
Copy link

We've been using @seiyab/eslint-plugin-react-hooks for several months now and it works just fine. Just add the package, then update your eslint config:

'plugins': [
    '@seiyab/eslint-plugin-react-hooks',
],
'rules': [
    '@seiyab/react-hooks/rules-of-hooks': 'error',
    '@seiyab/react-hooks/exhaustive-deps': ['error', { 'ignoreThisDependency': 'props' }],
]

(Note that the package own documentation got the rule names wrong)

@seiyab
Copy link

seiyab commented Nov 15, 2022

@laurent22
Thank you for your feedback.
I updated README and published as beta.
https://www.npmjs.com/package/@seiyab/eslint-plugin-react-hooks

@Jackman3005
Copy link

Jackman3005 commented Nov 18, 2022

Hey, just another question/idea to throw into the mix. I understand that the default behavior of calling a function on an object uses the object as this for that function.

In this situation when you are passing in props to a component I'm not sure of any conventions or maybe even any reasons why someone would be using the this property to access other props and it just sounds like a terrible idea anyway.

Proposal

React binds the this of any functions given as props to undefined or something else besides the current default, the props object. From what I can imagine, this likely would not effect many people and anyone effected should have simple alternatives given. (I could be totally off-base here, but would love to hear why.)

This would remove the reason that this lint rule identifies props.onChange() as being dependent on props.

Edit: As I give more thought, I can see some situations where overriding this is a bad idea. e.g. the function provided has a this that is not props. So maybe only do this when the this is props? Maybe too much to ask for this to be changed at the React level.

@strickc
Copy link

strickc commented Sep 22, 2023

After reading this discussion I agree the behavior is correct - use of props.whatever() is passing the whole props object in the call as this whether it is used in the function or not, so it's correct for the linter to flag it. I find the work around of using an assignment within the callback to be meaningfully correct to not leak props unintentionally, and doesn't disrupt any other conventions in the component like destructuring may.

Could the message be more helpful in these cases? Adding something like below to the existing message:

Props is being used as 'this' context for a function call so 'props' itself is a dependency of the hook...

@laurent22
Copy link

laurent22 commented Sep 23, 2023

@strickc, most of the comments are not discussing the correctness of the rule, we all know it's technically correct.

What's needed however is a way to ignore the rule for cases where this is irrelevant because it's never accessed, as it happens in any modern codebase. In fact this is a Facebook repo, and I'm sure it's extremely rare that anyone would access this at Facebook and if they do it would probably be flagged in review regardless of any rule of hook.

I think the only reason this rule is not updated is because the author got used to destructuring props so they have no need to ease the restriction no matter how pointless it is.

@BCarley
Copy link

BCarley commented Oct 24, 2023

We bumped our shins on this, in the case that you've got a useFetch / useAxios style hook where your object is {request, data, error, loading, cancel} this rule will send you into a infinite render loop of new hook object -> request -> new data / error -> new hook object -> request -> ...

We've updated our hook to return a stable object via useMemo()

    ...snip...
    // as we do not destructure the `useAxios` based hooks to keep the namespace, the returned object
    // is used in `useEffect` `dependency` arrays. This object needs to be stable to avoid
    // infinite render loops.
    const stableReturnObject = useMemo(() => {
      return { request, cancel };
    }, [request, cancel]);

    // we are using `Object.assign` here to mutate the object rather than creating
    // a new one, which cause downstream re-renders
    Object.assign(stableReturnObject, { data, error, loading });

    return stableReturnObject;
}

@vpzomtrrfrt
Copy link

Might be out of scope for this plugin, but TypeScript allows annotating function types with the type of this. Ideally any functions with this: void would be excluded from needing the whole object as a dep

@zeorin
Copy link

zeorin commented Nov 23, 2023

Could use https://github.com/not-an-aardvark/eslint-rule-composer for that.

FWIW I've taken to just putting props.onClick in the dependency array, and then just calling it as props.onClick.call(undefined, event). It's really not that difficult. TypeScript even warned me the one time I did this and the function in question did expect a specific this value! I.e. my experience has been that this ESLint rule is actually sensible, and as a result of me using the .call() method on the functions, it did actually help me avoid a Heisenbug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests