setNormalizedData should keep old updatedAt for queries that are affected

[johanobergman]

With react-query, let's say there's a books query with a staleTime of 10s:

await client.prefetchQuery({
  queryKey: ['books'],
  staleTime: 10_000,
  queryFn: () =>
    Promise.resolve([
      {
        id: '1',
        name: 'Book 1',
        comments: [...],
      },
      {
        id: '2',
        name: 'Book 2',
        comments: [...],
      },
    ]),
});

After 10 seconds have passed, the query is considered stale. Let's say it's no longer on screen, so it should be refetched at the next mount.

A call to setNormalizedData due to a mutation, or manually like this:

normalizer.setNormalizedData({
  id: '1',
  name: 'Book 1 updated',
});

will update the dataUpdatedAt property of every query that it touches. Book 1 in the books query will be updated and the books query will not be considered stale anymore and will not refetch at mount.

---

I think this behaviour is wrong, as even though we got some partial data back for Book 1, the rest of the data and the other books were not refreshed, so the books query should still be considered stale.

One use case I have is where I (as someone discussed in another issue) automatically call setNormalizedData when queries receive new data (like Apollo which I have used before does). But when I call queryClient.invalidateQueries() on a mutation (I still sometimes do even with Normy), all queries will first invalidate, but then setNormalizedData causes queries to immediately become valid again, if a previous query updates their data in some way.

Cause/fix

In the Normy internal function updateQueriesFromMutationData, react-query is called like this, which causes dataUpdatedAt to update:

queriesToUpdate.forEach(query => {
  queryClient.setQueryData(
    JSON.parse(query.queryKey) as QueryKey,
    () => query.data,
  )
})

The latest dataUpdatedAt timestamp can instead be preserved like this:

queriesToUpdate.forEach(query => {
  let queryKey = JSON.parse(query.queryKey) as QueryKey
  queryClient.setQueryData(queryKey, () => query.data, {
    updatedAt: queryClient.getQueryCache().find({ queryKey })?.state
      .dataUpdatedAt,
  })
})

Relevant discussion: TanStack/query#4716 (comment)

I made this as an issue instead of a PR since it could be seen as a breaking change (even though the current behaviour is not documented), so I wanted to get some input on this first. Another option could be to add a parameter to setNormalizedData where the user can decide if the dataUpdatedAt timestamp should be touched or not.

[johanobergman]

On further investigation, that change is not enough to support my use case. A call to queryClient.invalidateQueries() sets a boolean isInvalidated to true on query instances. queryClient.setQueryData() always resets it to false, regardless of the dataUpdatedAt timestamp, so queries that are updated from Normy alwas get isInvalidated: false.

A solution could be like this (tested and works for my use case):

// normy-react-query/src/create-query-normalizer
const updateQueriesFromMutationData = (
  mutationData: Data,
  normalizer: ReturnType<typeof createNormalizer>,
  queryClient: QueryClient,
) => {
  const queriesToUpdate = normalizer.getQueriesToUpdate(mutationData);

  queriesToUpdate.forEach(query => {
    let queryKey = JSON.parse(query.queryKey) as QueryKey;
    let cachedQuery = queryClient.getQueryCache().find({ queryKey });

    // Remember the state of the isInvalidated boolean.
    let isInvalidated = cachedQuery?.state.isInvalidated;

    // Update the query based on mutation data or data from setNormalizedData().
    queryClient.setQueryData(queryKey, () => query.data, {
      updatedAt: cachedQuery?.state.dataUpdatedAt,
    });

    // Reset isInvalidated to its previous state.
    if (isInvalidated) {
      cachedQuery?.setState({ isInvalidated });
    }
  });
};

It gets a bit involved though. Another option entirely could be to allow the user to construct its own normalizer via createNormalizer passed to createQueryNormalizer, and then pass that instance to QueryNormalizerProvider.

That way I would get a lot more control and could implement my own setNormalizedData().

[klis87]

One use case I have is where I (as someone discussed in another issue) automatically call

I guess you mean this issue #19 ? I would love to add it to the core, but it needs being careful, as I am afraid to cause some infinite loops when you manually update data yourself.

Regarding this issue, it actually makes sense, if you set some staleTime, it should be probably not touched when you manually update your queries data manually in memory - staleTime looks like an attr related to syncing interval with the server and it has nothing to do with manual updates.

So I think your solution seems perfect, I would actually add it to the library and I do not think to even add any flag to conditionally turn it off/on. Do you see some danger? The worst what could happen is that when using staleTime, a query would be refetched sooner after query update by normy, unless I am missing a case when it would be not suitable.

[johanobergman]

Yes, #19 is what i meant. I'm currently using a syncing implementation like this:

import { useQueryNormalizer } from '@normy/react-query'
import { useQueryClient } from '@tanstack/react-query'
import { useEffect } from 'react'

export function UpdateNormalizedQueriesOnRead({ children }) {
  let queryNormalizer = useQueryNormalizer()
  let queryClient = useQueryClient()

  useEffect(() => {
    // Update other queries when new data is received.
    let unsubscribe = queryClient.getQueryCache().subscribe(event => {
      if (
        event.type === 'updated' &&
        event.action.type === 'success' &&
        event.action.data !== undefined &&
        (event.query.meta?.normalize ?? true)
      ) {
        const data = event.action.data

        if (!event.action.manual) {
          queryNormalizer.setNormalizedData(data)
        }
      }
    })

    return () => unsubscribe()
  }, [])

  return children
}

and then wrapping my component tree in <UpdateNormalizedQueriesOnRead />.

I believe the if (!event.action.manual) check would prevent an infinite loop. manual is false when data comes from the server, true when using client.setQueryData() (which is used by setNormalizedData()). But maybe there are more places where an infinite loop could happen?

---

Regarding this issue, I think you are right that it should be safe to implement the dataUpdatedAt and isInvalidated changes. Like you said, worst case scenario is that some query becomes stale a bit earlier (which is probably the correct behaviour anyway).

One example when the changes maybe would feel a bit weird:

await client.prefetchQuery({
  queryKey: ['book'],
  staleTime: 10_000,
  queryFn: () =>
    Promise.resolve({
      id: '1',
      name: 'Book 1',
      author: 'Author 1',
    }),
});

and then a mutation returns the data

{
  id: '1',
  name: 'Book 1 updated',
  author: 'Author 1',
}

maybe one would expect the book query dataUpdatedAt to be updated, since the mutation returned all the fields. But I think that's a case that could be solved manually by the user if they really want the dataUpdatedAt to be updated in that specific instance.

Also, maybe it's worth looking at if more query state should be preserved? This is the query state that's being set on setQueryData():

{
  ...state,
  data: action.data,
  dataUpdateCount: state.dataUpdateCount + 1,
  dataUpdatedAt: action.dataUpdatedAt ?? Date.now(),
  error: null,
  isInvalidated: false,
  status: 'success',
  ...(!action.manual && {
    fetchStatus: 'idle',
    fetchFailureCount: 0,
    fetchFailureReason: null,
  }),
}

in query.dispatch() https://github.com/TanStack/query/blob/main/packages/query-core/src/query.ts#L570
via query.setData() https://github.com/TanStack/query/blob/main/packages/query-core/src/query.ts#L211
from queryClient.setQueryData() https://github.com/TanStack/query/blob/main/packages/query-core/src/queryClient.ts#L191

Should for example the previous network status and error (if there was any) be preserved?

[klis87]

Should for example the previous network status and error (if there was any) be preserved?

I think that generally data and error are mutually exclusive. You get data for success response, and you get error for error response. So I guess it actually makes sense to clear error when setting data.

Anyway, when you have error, unless I am wrong, you will not have any data, so such a query will never be hit to be updated by normy, because empty data will not have any normalized objects inside. So I think we are pretty good to go with just isInvalidated and dataUpdatedAt

[johanobergman]

You can actually have both data and error - if the error happens on refetch, the previous data will still be available for components to render, so they don't have to show a broken UI.

So right now, if you have a query that's normalized, and then errors, a mutation that touches that query will reset the error even though the query itself never was retried with a successful response. So I think it makes sense to keep error as well.

[klis87]

If that is the case, then you are right. I wonder though, why react-query does such things. As you say, it is usually wrong and unexpected to clear errors etc on manual updates like we do, so this should not be the default behiaviour. So what extra attrs we would like to preserve? error, status? fetchStatus I guess can be left as it is not reset for manual updates?

[johanobergman]

I agree, it's weird that react-query resets those things. But at least in the case where the user manually calls setQueryData() they and can more easily reset things like error if needed.

I guess if we preserve error, status should be preserved as well, but like you said, I think we can leave the things that are not reset for manual updates.

That would mean we reset dataUpdatedAt, isInvalidated, error and status, does that sound good? I can make a PR with some tests to see that everything works as it should.

[klis87]

Yes, it is good that they provided an escape hatch :)

Correct, status should be preserved as well. So dataUpdatedAt, isInvalidated, error and status seem to be perfect.

If you could dedicate some time with the PR, I would be very grateful!

I will wait for you with #31 BTW, as I would create conflicts, because I will move most of react-query package code to query, because it will be then used as a base for other addons like vue-query

[klis87]

Thank you for the PR! Merged and released https://github.com/klis87/normy/releases/tag/%40normy%2Fcore%400.12.0

[johanobergman]

Great, thanks!