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

[ESLint] Fix a bug causing a too coarse dependency suggestion #19313

Merged
merged 2 commits into from
Jul 10, 2020

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Jul 10, 2020

Fixes #19312.

If we use props.something and declare props.something in deps, this should not violate.

However, the existing logic for AssignmentExpressions caused it to be violating. This is because we forgot to check whether an expression is on the left or the right side. That logic should only have been active for the left side.

Added regression tests.

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jul 10, 2020
@gaearon gaearon requested review from rickhanlonii and bvaughn July 10, 2020 17:51
@codesandbox-ci
Copy link

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5811c92:

Sandbox Source
React Configuration

@sizebot
Copy link

sizebot commented Jul 10, 2020

No significant bundle size changes to report.

Size changes (experimental)

Generated by 🚫 dangerJS against 5811c92

@sizebot
Copy link

sizebot commented Jul 10, 2020

No significant bundle size changes to report.

Size changes (stable)

Generated by 🚫 dangerJS against 5811c92

@gaearon gaearon merged commit 47915fd into facebook:master Jul 10, 2020
@jeffvandyke
Copy link

I'm using the latest version of this plugin, but I think this might still be a problem in some scenarios: This code sandbox: https://codesandbox.io/s/cocky-wind-0kumj?file=/src/App.js still reports an error of a missing props dependency when it only uses props.onExit and props.onExit is indeed declared in the dependency array for useCallback. Is there something I'm missing, or is this bug still a problem?

@bvaughn
Copy link
Contributor

bvaughn commented Aug 6, 2020

The way your code is currently structured could cause a problem:

const onExit = React.useCallback(() => {
  // Because you're reading `onExit` off of an object,
  // that object may have been mutated since the last render.
  props.onExit("some stuff");
}, [props.onExit]);

The lint rule is trying to tell you to do something like this to avoid that problem:

const { onExit: onExitProp } = props;
const onExit = React.useCallback(() => {
  // Because you're reading a local variable,
  // it is guaranteed not to have changed since the last render.
  onExitProp("some stuff");
}, [onExitProp]);

@gaearon
Copy link
Collaborator Author

gaearon commented Aug 6, 2020

You're getting this because you're calling a function:

props.onExit()

In JavaScript, this is equivalent to:

props.onExit.call(props)

In other words, you're passing props itself as this so technically props itself needs to be a dependency.

The recommended fix is to use destructuring, as the message says:

Screenshot 2020-08-06 at 14 33 28

export default function App({ onExit }) {
  const handleExit = React.useCallback(() => onExit("some stuff"), [onExit]);

  return (
    <div className="App">
      <h1>Hello CodeSandbox</h1>
      <button onClick={handleExit}>Exit</button>
      <h2>Start editing to see some magic happen!</h2>
    </div>
  );
}

https://codesandbox.io/s/wandering-wood-soc63?file=/src/App.js:51-364

@jeffvandyke
Copy link

That explanation makes sense, and it explains why I'm getting this warning for callback props only, and not for reading simple value props. I do question how often the difference between calling props.onExit and destructuring and calling a propsOnExit actually shows up in the overwhelming majority of codebases. It might be helpful if there were an option to ignore the props this parameter when calling properties like this to avoid forcing existing code to need to convert to a destructuring approach (which has pain points in some areas). I'm curious if this would be a good enough idea to look into further.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug: Exhaustive deps lint rule mistakingly flags an assignment
6 participants