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-plugin-query: exhaustive-deps error not triggered when dependency is nested inside then/catch #6853

Open
DevHusariaSolutions opened this issue Feb 7, 2024 · 7 comments

Comments

@DevHusariaSolutions
Copy link

Describe the bug

exhaustive-deps triggers only when we use some dependency in non-nested code, but while using then/catch - it cannot detect used dependency.

Your minimal, reproducible example

Steps to reproduce

image
If we nest it in then/catch callbacks, error is not showing.
I know we can use "t" outside whole useQuery hook, it's just for demo. Any other function which would be part of fetchable logic won't trigger error.

Expected behavior

Error should be triggered like in example when we use "t" dependency outside then/catch clauses:
image

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

Windows, VSCode

Tanstack Query adapter

react-query

TanStack Query version

5.14.0

TypeScript version

5.1.3

Additional context

Version of eslint-plugin-query: 5.18.1

@TkDodo
Copy link
Collaborator

TkDodo commented Feb 13, 2024

@Newbie012 can you take a look please?

@IgorAufricht
Copy link

The same happens when there is a condition:

  return useSuspenseQuery({
    queryKey: [actionId], // <- eslint doesn't complain about missing baseUrl
    queryFn: () => {
      //const test = baseUrl; // <- if we uncomment this, eslint will start to complain about missing baseUrl in the queryKey
      if (actionId) {
        return baseUrl;
      }
    },
  });

This makes the rule ineffective for many non-trivial cases.

@Newbie012
Copy link
Collaborator

Sorry, I totally forgot about this issue.

@TkDodo I can implement that, but it would be a breaking change. Currently, we allow call expressions inside queryFn without explicitly set them in queryKey. For instance, given the following code:

function Component({ api }) {
  const t = useHook();

  useQuery({
    queryKey: ['test'],
    queryFn: () => api.get().then(() => t("foo"))
  })
}

the new behavior will require to pass both t and api.get (or api).

thoughts?

@TkDodo
Copy link
Collaborator

TkDodo commented Mar 22, 2024

I think both of these should error because you can easily run into stale closure issues. However, adding them to the queryKey is not ideal because they likely aren't serializable.

this is again where we either need to support defined exceptions for the rule, or users would have to disable the linter or rewrite their code. It is technically unsafe to do this.

@Newbie012
Copy link
Collaborator

So problem still persists. What's the point of passing a value that is not serializable into a query key? you would still get stale closures, since JSON.stringify(fn) would result in undefined if it doesn't have a proper toJSON.

@BenStirrup
Copy link

Hello 👋
I would like to inquire if a fix is planned for the non-functioning behavior of the plugin within try/catch/if blocks ?

@MartinDavi
Copy link

MartinDavi commented Apr 18, 2024

I think this might the same issue, but unrelated to try / catch, let me know if I should open a separate ticket.

This doesn't warn about the missing searchValue dependency:

useQuery({
    queryKey: ['someQuery'],
    queryFn: searchValue ? () => API.someMethod(searchValue) : skipToken,
  });

This does:

useQuery({
    queryKey: ['someQuery'],
    queryFn: () => API.someMethod(searchValue),
  });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants