Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

combine returns a new instance each time when useQueries is called without queries #6369

Closed
HarmonicTons opened this issue Nov 14, 2023 · 3 comments · Fixed by #6431
Closed
Assignees
Labels
bug Something isn't working has workaround help wanted Extra attention is needed package: query-core wontfix This will not be worked on

Comments

@HarmonicTons
Copy link

Describe the bug

When useQueries is used with an empty array [] for queries, the combine function will return a new instance each time the component carrying the hook is rendered. This can cause loop-rendering issues. It is possible to end up with an empty array for queries when the queries are determined dynamically. For instance:

export const useGetDevices = ({ ids }: { ids: string[] }) => {
 return useQueries({
   // queries are dynamically determined from the given ids
   // when ids is empty, queries will be []
   queries: ids.map((id) => {
     return {
       queryKey: ["get-device", id],
       queryFn: async () => getDevice(id)
     };
   }),
   combine: (results) => {
      // here we simply reduce the results into a Record
      // when ids is not empty the reduced record will be the same instance each time
      // but when ids is empty the reduced record will be a new instance of an empty object each time
      return results.reduce<Record<string, Device[]>>((acc, result) => {
        const { data } = result;
        if (!data) return acc;
        return {
          ...acc,
          [data.id]: data.content,
        };
      }, {});
    }
 });
};

More details in this discussion

Your minimal, reproducible example

https://codesandbox.io/s/react-query-combine-empty-array-7n29nr?file=/src/useGetDevices.ts:523-773

Steps to reproduce

  1. In app.tsx: set ids to a non-empty array (for instance ["1"])
  2. Click on "render" with the console opened
  3. See that the useEffect devicesRecord changed is NOT called when the component is rendered
  4. Edit app.tsx to set ids to an empty array []
  5. Click on "render" again
  6. See that now the useEffect is called each time the component is rendered

Expected behavior

The useEffect should not be triggered each time the component is rendered

How often does this bug happen?

Every time

Screenshots or Videos

No response

Platform

  • OS: macOS
  • browser: chrome, FF

Tanstack Query adapter

react-query

TanStack Query version

5.8.3

TypeScript version

No response

Additional context

No response

@TkDodo TkDodo added bug Something isn't working package: query-core labels Nov 14, 2023
@TkDodo TkDodo self-assigned this Nov 14, 2023
TkDodo added a commit that referenced this issue Dec 29, 2023
The initial fix for #6369 (#6431) introduced a regression where we'd always call combine with an empty array initially.

Turns out it was actually on purpose that we didn't call combine in the constructor, but delayed that until the `setQueries` call - because there, we already have complete query result structures available

However, there is an optimization that bails out early without calling this.#setResult if "nothing changed"; however, if we call useQueries with an empty array, that means nothing every changes, and the #combinedResult is actually never set, leading to structural sharing never having anything to share with, thus yielding a new reference every time (this is the root issue for #6369).

This fix reverts those changes (to avoid calling combine with an empty array every time, thus fixing #6612) and makes sure #setResult is always invoked before bailing out, so that #combinedResult is set correctly even for this case
TkDodo added a commit that referenced this issue Dec 29, 2023
…queries (#6614)

* fix(core): make sure to call setResult before the early bail-out

The initial fix for #6369 (#6431) introduced a regression where we'd always call combine with an empty array initially.

Turns out it was actually on purpose that we didn't call combine in the constructor, but delayed that until the `setQueries` call - because there, we already have complete query result structures available

However, there is an optimization that bails out early without calling this.#setResult if "nothing changed"; however, if we call useQueries with an empty array, that means nothing every changes, and the #combinedResult is actually never set, leading to structural sharing never having anything to share with, thus yielding a new reference every time (this is the root issue for #6369).

This fix reverts those changes (to avoid calling combine with an empty array every time, thus fixing #6612) and makes sure #setResult is always invoked before bailing out, so that #combinedResult is set correctly even for this case

* fix: add a special length check to the bail-out case

we only want to stop bailing out if we have no queries; moving the #setResult call unconditionally before the bail-out leads to more re-renders for other useQueries calls

now, we only stop the bail-out when the special empty case sets in, which is what fixes #6612
@TkDodo TkDodo reopened this Jan 5, 2024
@TkDodo
Copy link
Collaborator

TkDodo commented Jan 5, 2024

sorry I had to revert both fixes that I merged for this, because it introduced regressions for other cases. Right now, I think this issues is the least problematic, as it requires:

  • empty array of queries
  • combine function

and then, it "only" yields a new referential identity on re-renders, which shouldn't be that problematic because you have no queries anyways.

Still, I'll keep this open in case someone wants to look into it. There's a commented out test here that should pass when fixed.

it.skip('should not return new instances when called without queries', async () => {

I've also added tests for the other cases that broke when I tried to fix this so that we don't re-introduce those regressions again.

@TkDodo TkDodo added the help wanted Extra attention is needed label Jan 5, 2024
@HarmonicTons
Copy link
Author

HarmonicTons commented Jan 12, 2024

Thank you for the informations @TkDodo :)

If anyone with the same issue gets here from google looking for a solution: you can avoid having a new reference at each render by defining the empty object / array outside of the hook. For instance:

const emptyRecord = {};
export const useGetDevices = ({ ids }: { ids: string[] }) => {
 return useQueries({
   queries: ids.map((id) => {
     return {
       queryKey: ["get-device", id],
       queryFn: async () => getDevice(id)
     };
   }),
   combine: (results) => {
      return results.reduce<Record<string, Device[]>>((acc, result) => {
        const { data } = result;
        if (!data) return acc;
        return {
          ...acc,
          [data.id]: data.content,
        };
      }, emptyRecord); // here replace "{}" with an empty object defined outside of the hook.
    }
 });
};

@TkDodo
Copy link
Collaborator

TkDodo commented Feb 17, 2024

I don't think we will be fixing this, but given that it has a workaround and the issue is minor, I'll close it.

@TkDodo TkDodo closed this as not planned Won't fix, can't repro, duplicate, stale Feb 17, 2024
@TkDodo TkDodo added wontfix This will not be worked on has workaround labels Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working has workaround help wanted Extra attention is needed package: query-core wontfix This will not be worked on
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants