🦋 Changeset detected

Latest commit: 5abba2b

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Walkthrough

• Adds a patch changeset describing a fix to the useExhaustiveDependencies rule: when a method is invoked inside a closure, the dependency becomes the object (e.g., [props]) instead of an empty array.
• Updates get_whole_static_member_expression to return the object when the ancestor is followed by a call expression, introducing an early return in that case.
• Adds tests (issue7704.js) covering empty deps, function-prop deps, and object-prop deps for useEffect with props.foo().

Possibly related PRs

• fix(lint/useExhaustiveDependencies): correctly fix the dependency list from a shorthand object member #7498: Also adjusts useExhaustiveDependencies to better capture dependencies; both touch member-expression handling logic.

Suggested labels

A-Linter, L-JavaScript

Suggested reviewers

• arendjr

Pre-merge checks and finishing touches

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

CodSpeed Performance Report

Merging #7709 will not alter performance

_{Comparing fix/GH-7704 (5abba2b) with main (3eeb4df)}

Summary

✅ 53 untouched
⏩ 85 skipped¹

Hi @siketyan ,

Thanks for this PR. I agree with the rest of the changes, but can you please explain why granular dependencies are now invalid? I'm referring to the following example:

function InvalidComponent2(props) {
	useEffect(() => {
		props.foo();
	}, [props.foo]);
}

This is a very common pattern, and without this pattern, the effect would run when any prop changes, and not just props.foo.

@AfrazHussain Actually calling props.foo() references the props variable itself as the this parameter, not only props.foo. Thus we should declare it as a dependency of the useEffect hook. This is the same behaviour as eslint-plugin-react-hooks - see the related discussion: facebook/react#16265

Agree with @AfrazHussain. The "fix" caused a regression in behavior.
The main object reference can change but that should not cause the effect to rerun.

// render 1
const [foo] = useState(() => ({ bar: 42 }));
useEffect(() => { console.log('foo.bar', foo.bar); }, [foo]); // runs, first render.

And after modifying foo.bar:

// render 2a - we now pretend foo.bar=21 was called (ie foo was not mutated but bar was), so "foo" remains the same reference
const [foo] = useState(() => ({ bar: 42 }));
useEffect(() => { console.log('foo.bar', foo.bar); }, [foo]); // Error: Does not re-run as required

But it should have been:

// render 2b - we now pretend foo.bar has been modified directly, but "foo" remains the same object
const [foo] = useState(() => ({ bar: 42 }));
useEffect(() => { console.log('foo.bar', foo.bar); }, [foo.bar]); // Re-runs as intended!

In render 2a, it's assumed that the code author is using immutability.
In render 2b, no immutability is assumed, and code re-runs as intended.

This is backed up by ESLint's behavior as seen here on this playground link:
ESLint Online Plyground

You'll notice that it will be silenced if you add either foo or foo.bar, however foo.bar is the safer option here and foo is only safe if the developer knows it is. Thus, the rule will suggest foo.bar, not foo.

@scarlac I don't think your case is related to @AfrazHussain's one. Biome correctly adds foo.bar to the dependency list: https://biomejs.dev/playground/?code=aQBtAHAAbwByAHQAIAB7AHUAcwBlAEUAZgBmAGUAYwB0ACwAIAB1AHMAZQBTAHQAYQB0AGUAfQAgAGYAcgBvAG0AIAAiAHIAZQBhAGMAdAAiADsACgAKAGYAdQBuAGMAdABpAG8AbgAgAEMAbwBtAHAAbwBuAGUAbgB0ACgAKQAgAHsACgAgACAAYwBvAG4AcwB0ACAAWwBmAG8AbwBdACAAPQAgAHUAcwBlAFMAdABhAHQAZQAoACgAKQAgAD0APgAgACgAewAgAGIAYQByADoAIAA0ADIAIAB9ACkAKQA7AAoACgAgACAAdQBzAGUARQBmAGYAZQBjAHQAKAAoACkAIAA9AD4AIAB7ACAAIAAKACAAIAAgACAAYwBvAG4AcwBvAGwAZQAuAGwAbwBnACgAJwBmAG8AbwAuAGIAYQByACcALAAgAGYAbwBvAC4AYgBhAHIAKQA7AAoAIAAgAH0ALAAgAFsAXQApADsACgB9AA%3D%3D&ruleDomains.react=all

Thank you. Perhaps my report is misplaced then. I will open a new ticket, since I'm seeing regressions related to what was changed here, although it may not be exactly the same (This hook specifies a dependency more specific that its captures: foo.bar[lint](https://biomejs.dev/linter/rules/use-exhaustive-dependencies))

Opened a new issue here: #7782

@scarlac , Thanks for creating your issue. That is in fact what my comment was referring to as well like you correctly pointed out initially.

@siketyan The issue (facebook/react#16265) that you've linked is an open issue for the past 6 years, and even there people have mentioned work-arounds for this behaviour, so I don't think that if its the behaviour of eslint-plugin-react-hooks that it is correct. My issue is the same as the one that @scarlac mentions in #7782. The solution for me now is to downgrade to 2.2.5.

I’ve mentioned this previously in the Biomejs Discord, but it’s worth documenting here for future reference.

This is a concrete example of why methods can be unsafe as hook dependencies. I've encountered it a couple days ago at my workplace.

function Test() {
  const [values, setValues] = useState<string[]>([]);

  const computed = useMemo(
    () => values.map((v) => v.toUpperCase()),
    [
      // Expected: `values`, since it is used to derive a new array.
      // Proposed by Biome:
      values.map
      // This would always evaluate as stable because
      // `Array.prototype.map === Array.prototype.map`,
      // even when `values` changes.
    ],
  );
}