Should query functions be pure (can I call setState in them)?

[kachkaev]

We have a list of item ids. To display the list, we make an API call which gives us the list of item details. It may be the case that some of the original item ids are non-existing, so once we’ve received the query result, we might want to call setState and thus reduce the list of original items. This will trigger useQuery again, but let’s assume it's cheap.

Here is my question: is it ‘legal’ to call useState right from the query body? Or should this be done in useEffect, thus ensuring that query function itself has no side-effects?

const [itemIds, setItemIds] = React.useState(["a", "b", "c"]);

const { data, refetch } = useQuery(async () => {
  const items = await myApi.resolveItems(itemIds);
  if (items.length !== itemIds.length) {
    setItemIds(items.map((item) => item.id)); // <——————— 👀 legal or not?
  }
  return items;
});

React.useEffect(() => {
  refetch();
}, [itemIds]);

// Let’s assume that setItemIds is also used in component callbacks etc.

Here is how this might if queries must be pure:

const [itemIds, setItemIds] = React.useState(["a", "b", "c"]);

const { data, refetch } = useQuery(async () => {
  const items = await myApi.resolveItems(itemIds);
});

React.useEffect(() => {
  refetch();
}, [itemIds]);

const itemIdsRef = React.useRef([]);

React.useEffect(() => {
  itemIdsRef.current = itemIds;
}, [itemIds]);

React.useEffect(() => {
  if (!items) {
    return;
  }
  if (items.length !== itemIdsRef.current.length) {
    setItemIds(items.map((item) => item.id));
  }
}, [items]);

[TkDodo]

counter question: why would you need to do that? To get the real itemIds, you can just map over the returned data and get the item ids. if you really must, I would do it in the onSuccess callback of useQuery and keep the setState out of the queryFn.

[kachkaev]

The reason why I want to call setItemIds as a side-effect of useQuery is because React.useState(["a", "b", "c"]) actually lives further up the tree and I access it via a context provider. Consider itemsIds are a representation of some selection. I pick ["a", "b", "c"] via some other UI components, they get shown in a list. Then imagine c is deleted from the server and I click a button to refetch all queries. What I want is to end up with is ["a", "b"] in the provider, because some other part of the UI needs to say: items selected: 2 (instead of 3).

Probably onSuccess is better, but it comes with its own problems. If the actual call to useQuery happens inside a simple custom hook, for me to start using onSuccess, I need to make the API of my custom hook more complex. Doing so increses the probability of its misuse in the team.

Calling setItemIds inside useQuery is the easiest, considering the number of lines I need to write. It does feel wrong, so I wonder if this subjective feeling is backed by some real engineering problems or not.

[TkDodo]

Probably onSuccess is better, but it comes with its own problems. If the actual call to useQuery happens inside a simple custom hook, for me to start using onSuccess, I need to make the API of my custom hook more complex. Doing so increases the probability of its misuse in the team.

Not sure I completely get that. What I am always advocating for is using custom hooks as an abstraction over useQuery, so that you can specifically add things like an onSuccess listener at a central place:

const useItemsQuery = (itemIds) => return useQuery(
    ['items', itemIds],
    () => myApi.resolveItems(itemIds),
    {
        onSuccess: (items) => {
            if (items.length !== itemIds.length) {
                setItemIds(items.map((item) => item.id))
            }
        }
    }
});

[kachkaev]

What I mean by a custom query in my case is roughly this:

export const useWorkshopQuery = <TResult>(
  workshopQueryFunction: WorkshopQueryFunction<TResult>,
  deps: React.DependencyList,
  label: string = "unlabelled",
): WorkshopQueryResult<TResult> => {
  const [uniqueId] = React.useState(
    () => `${label} [${generateShortId({ length: 10 })}]`,
  )

  const logger = useLogger()

  React.useEffect(() => {
    const initialLabel = uniqueId.slice(0, -13)
    if (initialLabel !== label) {
      // To avoid the below warning, consider making the label less specific or create
      // a different instance of the whole component via the JSX key attribute.
      // Conceptually, by changing the label we say that we are now asking for the data
      // on something new (e.g. a different property). Thus, it is wrong to display
      // previous query result because it is completely irrelevant rather than just stale.
      logger.warning(
        `A change in useWorkshopQuery label is a noop: "${initialLabel}" → "${label}"`,
      )
    }
  }, [label, logger, uniqueId])

  const isFetchingRef = React.useRef(false)

  const {
    data,
    refetch,
    isFetching,
    remove,
  } = useQuery(
    ["workshop", uniqueId],
    () => workshopQueryFunction(),
    { useErrorBoundary: true },
  )

  isFetchingRef.current = isFetching

  React.useEffect(() => {
    // If the query is called multiple times while it is still fetching
    // we will want to remove the previous pending query to allow refetch to work
    if (isFetchingRef.current) {
      remove()
    }
    refetch()
    // eslint-disable-next-line react-hooks/exhaustive-deps
  }, [refetch, ...deps])

  return { data, isFetching }
}

Usage:

const { data: items } = useWorkshopQuery(
  async () => await myApi.resolveItems(itemIds),
  [itemIds] // ✨ powered by `eslint-plugin-react-hooks` → `react-hooks/exhaustive-deps` → `additionalHooks`
);

Rather than creating a specific custom query for resolving items, I have a general abstraction for all queries on a certain page of a bigger app. There are three reasons for this abstraction: to refetch all ["workshop", ...] queries when useWorkshopMutation is called, to not bother about label uniqueness on the workshop page and also to avoid manual calls to refetch() when query dependencies change.

What I mean by making "the API of my custom hook more complex" is exposing onSuccess from useWorkshopQuery just to avoid setItemIds in one workshopQueryFunction body.