[eslint-plugin-react-hooks] false positive with useFocusEffect

[leethree]

Upgraded eslint-plugin-react-hooks to 4.0.0, it's showing errors on React-Navigation's useFocusEffect hook.

import { useFocusEffect } from '@react-navigation/native';

  useFocusEffect(
    React.useCallback(() => {
      const unsubscribe = API.subscribe(userId, user => setUser(user));
      return () => unsubscribe();
    }, [userId])
  );

(example copied from React-Navigation documentation)

The current behavior

Lint error:

React Hook useFocusEffect received a function whose dependencies are unknown. Pass an inline function instead.

The expected behavior

The rules shouldn't check the function because it's not a React useEffect hook.

[gaearon]

The rules shouldn't check the function because it's not a React useEffect hook.

Sigh. People have been asking for the opposite, so we added this for every function ending with Effect.

[vkurchatkin]

Admittedly, useFocusEffect is weird. It could easily work exactly like useEffect instead of requiring useCallback. Also you could probably just use useIsFocused + useEffect to avoid this issue

[leethree]

Sigh. People have been asking for the opposite, so we added this for every function ending with Effect.

😢 I completely understand it. But it's difficult to ask all library authors to follow the same rules. Maybe adding an option would help.

[gaearon]

Options just cause further fragmentation. I think ideally we'd reserve Effect suffix for things that mirror the dependency API. And then e.g. react navigation could use useOnFocus or such.

[leethree]

Sounds reasonable to me.

we'd reserve Effect suffix for things that mirror the dependency API.

Would be great if this could be added to documentation. I would file an issue to react-navigation asking them to change the hook name 😄

[surgeboris]

Sigh. People have been asking for the opposite, so we added this for every function ending with Effect.

It is very nice indeed that you're implementing features people were asking for, but it's really worth to describe those features in the changelog at least (because not everyone is aware what others were asking for).

[gaearon]

@surgeboris When I added this changelog, I took twenty minutes to go through all the past PRs, remember the context behind them, and reconstruct it. I'm sorry that I missed one of them.

[gaearon]

Fixed changelog: 14e554b.

I think we'll need to ask react-navigation to offer an alternative naming for this one to avoid breaking the convention. I'm sorry I didn't catch this earlier. The upside is that people seeing the Effect naming might be a bit confused anyway and try to pass the deps, so this is probably a net positive overall.

[gaearon]

We're going to revert this heuristic, so no changes necessary to React Navigation.

[gaearon]

Removed in eslint-plugin-react-hooks@4.0.3.

[leethree]

@gaearon thanks!