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

Bug: react-hooks/exhaustive-deps false positive when function is casted with TypeScript #20750

Closed
0x24a537r9 opened this issue Feb 5, 2021 · 4 comments · Fixed by #28202
Closed
Labels
Component: ESLint Rules Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Bug

Comments

@0x24a537r9
Copy link

React version: 17.0.1

Steps To Reproduce

Setup eslint with @typescript-eslint/parser as parser
Cast a function passed to useEffect

import {useCallback, useEffect} from 'react';

type F = (...args: unknown[]) => void;

function MyComp() {
	const foo = useCallback(() => {}, []);

	// OK
	useEffect(() => {
		foo();
	}, [foo]);

	// WARNS?
	useEffect((() => {
		foo();
	}) as F, [foo]);

	return 'Hello, world'
}

Link to code example: https://github.com/0x24a537r9/exhaustive-deps-bug

The current behavior

The following error was reported

  14:2  warning  React Hook useEffect received a function whose dependencies are unknown. Pass an inline function instead  

The expected behavior

The rule should interpret the function argument correctly and know that it is already inline.

@0x24a537r9 0x24a537r9 added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Feb 5, 2021
@vkurchatkin
Copy link

It looks like it is something that would be easy to support, but the question is: why would you do that?

@0x24a537r9
Copy link
Author

Heh, a fair question. In my case, I needed it in order to support generics for a hook that helps manage state for asynchronous calls like mutations:

export function useAsyncAction<F extends (...args: unknown[]) => Promise<R>, R>(
  doAction: F,
): [
  doActionWithState: F,
  isDoingAction: boolean,
  errorDoingAction: unknown | null,
  clearErrorDoingAction: () => void,
] {
  const [isDoingAction, setIsDoingAction] = useState<boolean>(false);
  const [errorDoingAction, setErrorDoingAction] = useState<unknown | null>(null);

  // https://github.com/facebook/react/issues/20750
  // eslint-disable-next-line react-hooks/exhaustive-deps
  const doActionWithState = useCallback(
    (async (...args: unknown[]): Promise<R> => {
      setIsDoingAction((prevIsDoingAction) => {
        if (prevIsDoingAction) {
          throw new ReentrancyError(doAction);
        }
        return true;
      });
      setErrorDoingAction(null);

      try {
        return await doAction(...args);
      } catch (e: unknown) {
        setErrorDoingAction(e);
        throw e;
      } finally {
        setIsDoingAction(false);
      }
    }) as F,
    [doAction],
  );

  const clearErrorDoingAction = useCallback(() => setErrorDoingAction(null), []);

  return [doActionWithState, isDoingAction, errorDoingAction, clearErrorDoingAction];
}

Without the cast, TS doesn't know that the shape of the args object is the same. On the return statement, TS warns:

Type '(...args: unknown[]) => Promise<R>' is not assignable to type 'F'.
  '(...args: unknown[]) => Promise<R>' is assignable to the constraint of type 'F', but 'F' could be instantiated with a different subtype of constraint '(...args: unknown[]) => Promise<R>'.ts(2322)

This remains true even if I make the args a generic type too, so I think the issue is unavoidable.

@Shrugsy
Copy link

Shrugsy commented Apr 5, 2021

@0x24a537r9 TS is correct there. A function that has the same input type & return type as F is not assignable to F. F might have other properties attached to it as well, which won't be attached to doActionWithState.

For your particular situation, the type for doActionWithState can be a function that has the same argument & return types as F, but should not be F. Casting it as F is lying to typescript.

@0x24a537r9
Copy link
Author

Ahhh. Thanks @Shrugsy! With that hint I was able to fix my types and obviate the cast.

That said, I would recommend keeping this issue open, since there will always be cases where folks will have to resort to casts like these, whether due to limitations in TS's expressiveness or (as in this case and I suspect most cases) one's ability to understand the type complexities and specify the types correctly. Sadly, not everyone has access to a @Shrugsy to help them out when the types get complex.

eps1lon pushed a commit that referenced this issue Feb 1, 2024
…ck (#28202)

## Summary

Closes #20750

## How did you test this change?

Added a test case
github-actions bot pushed a commit that referenced this issue Feb 1, 2024
…ck (#28202)

## Summary

Closes #20750

## How did you test this change?

Added a test case

DiffTrain build for [2efa383](2efa383)
gaearon pushed a commit that referenced this issue Feb 3, 2024
…ck (#28202)

## Summary

Closes #20750

## How did you test this change?

Added a test case
EdisonVan pushed a commit to EdisonVan/react that referenced this issue Apr 15, 2024
…ck (facebook#28202)

## Summary

Closes facebook#20750

## How did you test this change?

Added a test case
bigfootjon pushed a commit that referenced this issue Apr 18, 2024
…ck (#28202)

## Summary

Closes #20750

## How did you test this change?

Added a test case

DiffTrain build for commit 2efa383.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: ESLint Rules Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants