false-positive with "static" dependencies

[TkDodo]

Sometimes, we have a static dependency that we read from e.g. react context, like an api:

const useFoo = () => {
  const api = useApi();
  return useQuery({
    queryKey: ['foo'],
    queryFn: () => api.fetchFoo(),
  })
}

this api is injected via react-context, but it's stable and not really changing. We mainly have it context to provide a different API for testing.

The eslint-plugin-query complains here rightfully that api should be added to the QueryKey, but we can't really do that - it's an axios instance that is not serializable. It's also not intended to be added to the key. For us, it's essentially the same as:

import api from 'api'

const useFoo = () => {
  return useQuery({
    queryKey: ['foo'],
    queryFn: () => api.fetchFoo(),
  })
}

Of course, the linter doesn't know that this dependency is "stable". Maybe we could have an option on the eslint plugin that defines known exceptions? @Newbie012 what do you think?

[Newbie012]

when I wrote the first POC, I actually supported some kind of ignoredKeys, but I eventually dropped it since I think the rule should ignore member expression nodes where the deepest node is a call expression (e.g. a.b.c.d(). In fact, I thought I wrote a test for it.

Hopefully I'll be able to raise a PR in The/Wed (on a trip so I won't be near my computer)

[am-maneaters]

i've run in to the same thing when i use constant values. i was expecting this to work the same way as a useEffect dependency list

const CONST_VAL = 1;

const useFoo = () => {

  // queryKey eslint error: The following dependencies are missing in your queryKey: CONST_VAL
  const testQuery = useQuery({
    queryKey: ['foo'],
    queryFn: () => CONST_VAL,
  });

  // no react-hooks error when using const
  useEffect(() => {
    const newVal = 1 + CONST_VAL;
  }, []);

};

[TkDodo]

@samMatenaer this is a different issue. constants defined outside the component should indeed be ignored.

[Newbie012]

@TkDodo Maybe I did something wrong, but I was unable to reproduce this issue (there's even a test to cover it). I also wrote another test to be sure - #5218

[TkDodo]

interesting @Newbie012. The question is, why doesn't this error? api coming from useApi could just as well be something that is a reactive dependency, which should be included. For example, something like this:

const useFilters = useSelector(store => store.filters)

const useFoo = () => {
  const filters = useFilters();
  return useQuery({
    queryKey: ['foo'],
    queryFn: () => fetchFoo(filters)
  })
}

This should definitely error. Is the difference in how those things are used? Like filters is passed to a function, while in the api example, we merely invoke a function on it?

[TkDodo]

oh yeah, that seems to be it. I wrote a test that errors on fetchFoo(filters), but succeeds on filters.fetchFoo(). I think that's fine? It basically means we're treating anything that returns functions as static, which might not be entirely true. Our api could be a function that's wrapped in useCallback and has itself a reactive dependency, like a baseURL that could change. But it's debatable if we'd want to error on those as well. I think the react exhaustive deps linter would error on those. If we want that as well, we'd need the "allowList" to list exceptions. If not, I think we are fine with the current implementation.

[Newbie012]

It would make sense that fetchFoo(filters) will error while filters.fetchFoo() won't, since filter is a meaningful value that we want to "react" when it changes, while it's not (mostly) the case with functions.

In rare cases such as changing the base URL or the Authorization header, I would prefer explicitly resetting the query cache.