fix: reduce the number of renders

[boschni]

Hi there! This PR includes the following changes:

• Make sure the hook return values match the public APIs.
• Make sure the hooks only re-render when something has changed.
• Make sure the hook return values are stable / memoized.
• Batch the isStale and succes update together when possible.
• Batch the failureCount and error update together when possible.

With these changes, the number of renders for successful and unsuccessful queries can be reduced from 4 to 2.

PS: While working on the tests, I noticed the canFetchMore and isFetchingMore properties are sometimes set to undefined. I guess this is not correct? For now I matched the assertions with the current behaviour.

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/tannerlinsley/react-query/knvumyz9i
✅ Preview: https://react-query-git-fork-boschni-feature-reduce-renders.tannerlinsley.vercel.app

[tannerlinsley]

Wow, this is a lot to take in all at once. I'll try and get to this asap.

[tannerlinsley]

I love these bits where we're just being smarter about dispatching and fixing some of the funky timing issues. I would really like to merge these asap too, since they're non-breaking changes that will help everyone.

On the topic of all of the useBaseQuery and related changes, I'm not sure I'm on board yet with the implementation. I totally agree with you on the problems that need to be solved, but a have a few concerns:

• There's a lot of ref wizardry going on where I feel it could be more of just useMemo with the right dependencies.
• Despite the changes being correctly aligned to the docs now, this would technically be a breaking change. I would like to avoid that at all costs. I'm okay if some of the publicly accessible API is undocumented. When we start considering a v3, we could then actually remove some of the public API to be in line with the docs (or just update the docs :)
• I'm not a fan of increase in library size that this is resulting in. It looks like 0.4kb or something similar. Not crazy, but i would expect that for this size, we're adding some massive features, not fixing some bugs. Which kinda points me to my last point.
• I think this same outcome could be achieved with less code. I'm not sure what that is yet, but I think it can. It might involved stripping away into the core a bit more, which I'm fine with.

I think what I'd like to see happen from this point:

• This PR becomes a subset of the quick wins that I commented on so we can get them merged asap.
• A new PR for the tests that ensure value stability (so we can experiment with the current version and try to get the tests to pass with other implementations
• A sibling PR containing the changes you've made to achieve value stability.

From there I'd like to use that second PR as a jumping off point and take a shot at combining some of the tactics you've implemented in your changes with a few lower level changes I have in mind (but haven't validated) yet.

[boschni]

Happy to help! I have adjusted the PR with these changes:

• Removed the API normalisation in useBaseQuery. It would indeed be better to do this in a major version.
• Removed the state equality checks in useBaseQuery and moved them into query to make sure the state is only updated and dispatched when something has changed.

With these changes the number of renders is still reduced to 2 and I think this is actually a simpler and more performant solution :) But if you have any doubts about the implementation then I don’t mind splitting it up.

PS: I removed the queryInfoRef because with this implementation the reference would not get updated when a value inside the query info object changes. The idea with memoization is that the reference remains the same unless something has changed. This is more of a nice to have anyway because in most cases consumers will destructure the query info object instead of doing checks on the object itself.

[tannerlinsley]

It’s looking better! So do you really think we need the shallow equal? If we are using a reducer we should be able to do an identity check instead, right?

[boschni]

Currently the state always changes when an action is dispatched, regardless of whether some value has actually changed or not.

We could add manual checks in the reducer (or before dispatching actions) to prevent this.

For example:

case actionFetch:
  const status = typeof state.data !== 'undefined' ? statusSuccess : statusLoading
  const isFetching = true
  const failureCount = 0

  if (state.status === status && state.isFetching === isFetching && state.failureCount === failureCount) {
    return state
  }

  return {  ...state, status, isFetching, failureCount }

But this would need to be done in multiple places and is a bit prone to mistakes.

Doing a shallow check after the state update is probably the simplest and most bullet-proof solution.

[tannerlinsley]

Alright, thats fair. How about we do this instead and just get rid of the shallow equal?

// Do nothing if the state did not change
    if (deepIncludes(query.state, newState)) {
      return
    }

It passes your tests! ;)

[boschni]

The deepIncludes function would be a bit slower because it does a deep comparison (including response data). Also, because it checks if b is a subset of a it does not really check if the objects are equal. One way to solve this would be to run the check two times like deepIncludes(query.state, newState) && deepIncludes(newState, query.state) but that would slow it down even more. Think the shallowEqual function is more suited for the job :)

[tannerlinsley]

Okay. The primary motivation for that suggestion was just bundle size. So I’m good if that’s the only way to do it right.

…

On Jul 16, 2020, 10:13 AM -0600, Niek Bosch ***@***.***>, wrote: The deepIncludes function would be a bit slower because it does a deep comparison (including response data). Also, because it checks if b is a subset of a it does not really check if the objects are equal. One way to solve this would be to run the check two times like deepIncludes(query.state, newState) && deepIncludes(newState, query.state) but that would slow it down even more. Think the shallowEqual function is more suited for the job :) — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or unsubscribe.

[tannerlinsley]

Can you pull in the latest from master?

[boschni]

Updated 👍

[boschni]

Updated the branch again and removed the getBaseQueryInfo part

[tannerlinsley]

🎉 This PR is included in version 2.5.5 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[kamranayub]

@tannerlinsley @boschni Hey folks! I am suspecting this PR caused some regressions in our test suite regarding when isStale is set, to help us avoid re-fetching a query if it isn't stale. I still have to dig into why the test is failing, but the suite was passing with 2.5.4. It could be I need to adjust our logic that digs into the cache to check isStale but I'll let you know what I find.

update: I am wrong, it was the opposite! This seemed to fix prefetchQuery respecting immediate staleness of 0, which was the default in our tests. The following test case would pass in 2.5.4, which should have actually been failing according to the new semantics in 2.x

it('should not call query if data is cached', () => {
  queryCache.setQueryData('mock', { stale: true });

  const mockQueryFn = jest.fn(() => ({ test: true }));
  const data = await queryCache.prefetchQuery('mock', mockQueryFn);

  expect(mockQueryFn).not.toHaveBeenCalled();
  expect(data).toEqual({ test: true });
})

Adjusting our stale time for the test will fix the issue, as per the new prefetchQuery semantics:

it('should not call query if data is cached', () => {
- queryCache.setQueryData('mock', { stale: true });
+ queryCache.setQueryData('mock', { stale: true }, { staleTime: 100 });

  const mockQueryFn = jest.fn(() => ({ test: true }));
  const data = await queryCache.prefetchQuery('mock', mockQueryFn);

  expect(mockQueryFn).not.toHaveBeenCalled();
  expect(data).toEqual({ test: true });
})