[feat] Disallow a function wrapped in `useCallback` yet only used in `useEffect` and only in one `useEffect`

[SukkaW]

Problem Description

This code should be considered invalid:

const Component = () => {
  const doSomeTask = useCallback(() => { /***/ }, [/***/]);
  useEffect(() => {
    // some condition
    doSomeTask();
  }, [doSomeTask]);
  return <div />;
};

This is because doSomeTask is only used inside one useEffect, thus the entire function declaration should be moved inside this one useEffect instead (and merge the useCallback's dependency array into useEffect's dependency array):

// Correct version of the above code

const Component = () => {
  useEffect(() => {
    const doSomeTask = () => { /***/ };

    // some condition
    doSomeTask();
  }, [/***/]);

  return <div />;
};

However, when the function wrapped by useCallback is used more than once (i.e. two useEffect), then it should be valid. E.g., those two examples should both be valid.

// useCallback in two useEffect

const Component = () => {
  const doSomeTask = useCallback(() => { /***/ }, [/***/]);

  useEffect(() => {
    // some condition
    doSomeTask();
  }, [doSomeTask]);

  useEffect(() => {
    // some condition
    doSomeTask();
  }, [doSomeTask]);

  return <div />;
};

// useCallback in both useEffect and JSX

const Component = () => {
  const doSomeTask = useCallback(() => { /***/ }, [/***/]);

  useEffect(() => {
    // some condition
    doSomeTask();
  }, [doSomeTask]);

  return <div onClick={doSomeTask} />;
};

Callback ref, by its nature, should be valid as well:

// useCallback in two useEffect

const Component = () => {
  const doSomeTask = useCallback(() => { /***/ }, [/***/]);

  return <div ref={doSomeTask} />;
};

We will allow useCallback with JSX's event handlers as well:

// useCallback in two useEffect

const Component = () => {
  const doSomeTask = useCallback(() => { /***/ }, [/***/]);

  return <div onClick={doSomeTask} />;
};

Alternative Solutions

N/A

Feature Details

Maybe we could call this rule: no-useless-use-callback-for-use-effect

Examples

See above.

Evaluation Checklist

• [x] I have had problems that this feature would solve
• [x] I could not find a way to solve the problem with existing features or workarounds
• [x] I have thought very hard about potential edge cases and downsides, and they are acceptable
• [x] I think the feature is well-defined and would provide clear value to users

[Rel1cx]

I suggest enhancing the no-unnecessary-use-callback rule to cover the case outlined in this issue, and updating the rule's docs to include new examples and explanations.

[possum-enjoyer]

I suggest enhancing the no-unnecessary-use-callback rule to cover the case outlined in this issue, and updating the rule's docs to include new examples and explanations.

If you want I could have a look into it :)

[Rel1cx]

I suggest enhancing the no-unnecessary-use-callback rule to cover the case outlined in this issue, and updating the rule's docs to include new examples and explanations.

If you want I could have a look into it :)

Yes, that's great. Thank you.

[possum-enjoyer]

@Rel1cx here is a WIP / first draft. If you want to have a look. The test cases need some formatting and maybe one or two extra invalid cases with different importSources :) But all of the ones inside the issue are working. I am just a little bit anxious if this code is fitting inside the rest of the codebase and if I forgot anything.

My knowledge of eslint rule development is unfortunately very limited aside from some smaller things here and there. So any criticism is very much welcome and encouraged.

https://github.com/possum-enjoyer/eslint-react/commit/dd2e632540983f60c72693cb18668531929ba71f

Edit: link and some explanation

[possum-enjoyer]

Also as I come to think of it. Couldn't the same logic be applied to the same useMemo lint rule for memorized values? If they are only used inside 1 effect they are in the dep array, if it exists, and therefore the effect always runs if the useMemo ran.

[SukkaW]

Also as I come to think of it. Couldn't the same logic be applied to the same useMemo lint rule for memorized values? If they are only used inside 1 effect they are in the dep array, if it exists, and therefore the effect always runs if the useMemo ran.

Actually, yes!

here is a WIP / first draft. If you want to have a look. The test cases need some formatting and maybe one or two extra invalid cases with different importSources :) But all of the ones inside the issue are working. I am just a little bit anxious if this code is fitting inside the rest of the codebase and if I forgot anything.

My knowledge of eslint rule development is unfortunately very limited aside from some smaller things here and there. So any criticism is very much welcome and encouraged.

possum-enjoyer@dd2e632

Edit: link and some explanation

You can actually create a "draft" PR where we can add reviews. And once it is polished, you can convert the PR to "ready to merge".

https://github.blog/news-insights/product-news/introducing-draft-pull-requests/

[possum-enjoyer]

You can actually create a "draft" PR where we can add reviews. And once it is polished, you can convert the PR to "ready to merge".

https://github.blog/news-insights/product-news/introducing-draft-pull-requests/

Yeah sorry my bad I forgot about the draft prs, I use them barely 😅 will do that after some touch ups and the useMemo stuff :)

Edit: Draft PR is opened https://github.com/Rel1cx/eslint-react/pull/1321 :)

[silverwind]

This is hitting on code like this:

const onKeydown = useCallback((event) => {
  console.log(event);
}, []);

useEffect(() => {
  window.addEventListener("keydown", onKeydown);
  return () => { window.removeEventListener("keydown", onKeydown); };
}, [onKeydown]);

I guess the fix is

useEffect(() => {
  function onKeydown(event) {
    console.log(event);
  }
  window.addEventListener("keydown", onKeydown);
  return () => { window.removeEventListener("keydown", onKeydown); };
}, []);

but I'm not sure I like it. I prefer to avoid nesting functions like that. Maybe there should be a opt-out option for the new rule functionality of no-unnecessary-use-callback.

[possum-enjoyer]

This is hitting on code like this:

const onKeydown = useCallback((event) => {
  console.log(event);
}, []);

useEffect(() => {
  window.addEventListener("keydown", onKeydown);
  return () => { window.removeEventListener("keydown", onKeydown); };
}, [onKeydown]);

I guess the fix is

useEffect(() => {
  function onKeydown(event) {
    console.log(event);
  }
  window.addEventListener("keydown", onKeydown);
  return () => { window.removeEventListener("keydown", onKeydown); };
}, []);

but I'm not sure I like it. I prefer to avoid nesting functions like that. Maybe there should be a opt-out option for the new rule functionality of no-unnecessary-use-callback.

For the extact code you showed the error message is still the "old" message i.e. it says you should move it outside the component scope not inside the useEffect. If there is more code inside the useCallback with something inside the dep array and it's only used inside the useEffect then it's a different "issue".

Is the Problem for you that the function is defined inside the useEffect and doesn't "pollute" the component scope?

[silverwind]

There's nothing wrong with the rule, it's doing it's job fine. The fixed code also works as expected. I think I will get used to writing inline functions inside useEffect.

Still I guess a option to control this may have been nice to provide more control over the functionalities of the rule. I think it would have been better to create a separate rule.

[possum-enjoyer]

There's nothing wrong with the rule, it's doing it's job fine. The fixed code also works as expected. I think I will get used to writing inline functions inside useEffect. Still I guess a option to control this may have been nice.

Yeah sorry i agree that an option could be a good idea. Something like "allowSingleEffectUsage: boolean" and it defaults to false

[silverwind]

The fixed variant is also how react's docs recommend using addEventListener, so it is definitely the right way. So I'm happy with the rule as-is and I think I don't even need this opt-out.

[SukkaW]

For this specfic case you can always do

This is hitting on code like this:

const onKeydown = useCallback((event) => {
  console.log(event);
}, []);

useEffect(() => {
  window.addEventListener("keydown", onKeydown);
  return () => { window.removeEventListener("keydown", onKeydown); };
}, [onKeydown]);

but I'm not sure I like it. I prefer to avoid nesting functions like that. Maybe there should be a opt-out option for the new rule functionality of no-unnecessary-use-callback.

For this specific code, you can always do:

function onKeydown(event) {
  console.log(event);
}

function Comp() {
  useEffect(() => {
    window.addEventListener("keydown", onKeydown);
    return () => { window.removeEventListener("keydown", onKeydown); };
  }, []);
}

On the other hand, if your onKeydown is slightly more complex and relies on the component's state/props, you should take a look at useEffectEvent instead (which allows event handlers can get the latest state snapshot during their invocation, w/o useEffect re-evaluation and triggering a re-subscribe), and I am pretty sure this rule won't cause trouble w/ useEffectEvent.