eslint-plugin-query: exhaustive-deps error not triggered when dependency is nested inside then/catch

[DevHusariaSolutions]

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

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:

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]

@Newbie012 can you take a look please?

[IgorAufricht]

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]

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]

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]

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]

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]

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),
  });

[braeden]

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

@TkDodo I guess I'm curious what you mean when you said it's It is technically unsafe to do this. (other than the fact that it's a breaking change in the ESLint rule). We don't make a promise that any custom hashFns will be sent serializable values, right?

It feels like a worthwhile ESLint break to do this? Having this lint rule miss variables (even functions) that are captured by queryFn closure (even through nested scopes -- if, try, catch), and defined within the React lifecycle is wrong/antithetical to the intent of the ESLint rule.

With a ESLint fix to capture everything, the default behavior of the default hashFn for functions is fine (no worse than before) https://github.com/TanStack/query/blob/main/packages/query-core/src/utils.ts#L218-L227, as @Newbie012 mentioned, functions are just coerced into undefined/null

• I know the serializability of query keys is conceptually fairly core to react-query, but it's worth thinking about how easy it is to get stale closures here.
  • As a future optimization, I wonder if we could/should just change the implementation of hashFn to automatically handle
    non-serializable values by storing a WeakMap<ClosuredNonserializableValues, string> where we just put a UUID on the right-hand side. Instead relying on reference equality, but w/o needing to change a ton of downstream code.
• If we're worried about opting-out of functions in-particular, (assuming we don't want to rely on building a typescript-eslint variant), we should be able to infer when variables have a CallExpression applied, but still be able to special case .then/.catch/.finally and normal blocks like if, try, catch should just work.

[TkDodo]

I guess I'm curious what you mean when you said it's It is technically unsafe to do this.

look at e.g. this code:

function Component() {
  const api = useApi()

  useQuery({
    queryKey: ['test'],
    queryFn: () => api.get('/test')
  })
}

it looks innocent enough, but there is a stale closure issue in there, depending on how useApi is implemented. It could e.g. be a React Context that contains some reactive values, e.g. a cookie. Now if someone calls setCookie(newCookie) on that context, our useQuery might close over an old value of api, where it still sees the “old” cookie. That’s a stale closure.

Now we don’t know if useApi will do that or not. But there’s also no good solution here, as we can’t include api in the QueryKey unless its serializable (it likely is not), or a custom hashFn is used.

So widening the key to ['test', api] is pointless, but it's literally what the “solution” to the t problem would be. OP expects t('something') to be reported. Not sure how they would fix it tbh - it seems like translations are usually static, so its safe to exclude them.

---

So my proposal would be to include an allowList of allowed exceptions to the lint rule, where you could add api or t or whatever so that it doesn’t get reported by the linter.

On the other hand, it is potentially dangerous to do that, as the api example hopefully shows.

[braeden]

Thanks for the detailed reply -- I think I'm largely on the same page (and I think we're both in agreement that your example is functionally dangerous/wrong, but as you illustrated, it's currently trivial to write & remain undetected). Here's a couple thoughts, curious for your take, because the current state feels unideal/foot-gun-y 😅.

So my proposal would be to include an allowList of allowed exceptions to the lint rule, where you could add api or t or whatever so that it doesn’t get reported by the linter.

100% agree here, more optionality to "bail-out" is fine -- it's also harmless to include functions/non-serializable values using the current hashFn. Right now the lint rule is missing serializable values captured in additional scopes (like try/catch) anyways 😬.

So widening the key to ['test', api] is pointless, but it's literally what the “solution” to the t problem would be.

Agree this is hypothetically the right solution to the OP -- but imo this isn't pointless -- it's only pointless because we deem functions to be unserializable using the default hashFn.

---

In a perfect world, my proposal would be:

• The lint rule unconditionally adds the most narrow component scoped, closed-over values to the queryKeys (for all scopes inside the queryFn).
  • This should be identically modeled to React's exhaustive dep check.
  • The scope issue is the primary problem I'm concerned about.
• Inside the ESLint configuration for this rule, we add a allowList/ignoreList for sets of values that we want to ignore from our closure rules.
  • How you'd exactly describe these symbols TBD
• When the lint rule detects a queryKeys value that we suspect to be unserializable (eg non-pojos/primitives) we warn about the fact that these will get silently dropped by the default hashFn, and we suggest using 2x alternative hashFns that we ship as part of this library. (See below)
  • n.b. maybe this is a independent lint rule entirely, that can use type information -- it's so trivial to accidentally include Map that gets dropped silently.
  • n.b this could also be a runtime warning

---

Proposed additional hashFns pseudo-code:

const referenceCache = new WeakMap<object, string>();
function hashFnWithReferencesForNonSerializableValues(input: unknown) {
    if (typeof input === "function") { // also add Map, Set
        const val = referenceCache.get(input) ?? uuid()
        referenceCache.set(input, val)
        return val
    }
   // ...Normal recursive serialization implementation
}

const referenceCache = new WeakMap<object, string>();
function hashFnWithShallowReferenceEquality(input: unknown) {
    if (Array.isArray(input)) {
        return input.map(hashFnWithShallowReferenceEquality).join()
    }
   if (isPrimitive(input)) { 
        return input 
   }

   const val = referenceCache.get(input) ?? uuid()
   referenceCache.set(input, val)
   return val
}

[TkDodo]

The lint rule unconditionally adds the most narrow component scoped, closed-over values to the queryKeys

I’m fine with that.

Inside the ESLint configuration for this rule, we add a allowList/ignoreList for sets of values that we want to ignore from our closure rules.

yep.

When the lint rule detects a queryKeys value that we suspect to be unserializable (eg non-pojos/primitives) we warn about the fact that these will get silently dropped by the default hashFn

I would do this as a runtime warning (in dev mode only) at best. If custom hashFns are used, they are most likely defined globally as a defaultOption, so the lint rule won’t know about those.

@Newbie012 what do you think?

[Newbie012]

So my proposal would be to include an allowList of allowed exceptions to the lint rule, where you could add api or t or whatever so that it doesn’t get reported by the linter.

I could do that, but IIRC, today, the rule won't report any errors for <..>.call() pattern, so it would make sense to report it as an error if we introduce allowList option (which is somewhat a breaking change)

The lint rule unconditionally adds the most narrow component scoped, closed-over values to the queryKeys

In other words, you're expecting api to be added to queryKey if there's a usage of api.get()? what if I do api.get.call(null, id)?

I would do this as a runtime warning (in dev mode only) at best. If custom hashFns are used, they are most likely defined globally as a defaultOption, so the lint rule won’t know about those.

While it's (somewhat) possible to look for QueryClient in the codebase, in this rule it would be quite challenging since I don't have a direct reference for it for each check, so performance is a concern here.