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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(useQueries): search for unused observers if keepPreviousData (#2680) #2866

Merged

Conversation

@jvuoti
Copy link
Contributor

@jvuoti jvuoti commented Nov 1, 2021

closes #2680

When keepPreviousData is on, search for a query observer no longer used instead of relying on observer order and indexing. Trying to match observers by index could cause infinite request loops when switching queries. See #2680 for more

This does complicate the getOptimisticResult and updateObservers calls a bit. Now they both essentially do the same search for unused observers. The observers lists for the queriesObserver are only updated in the updateObservers, though.

This does cause some extra updates, as can be seen in the tests. If I understand correctly, they probably are due to the setOptions calls being called both on getOptimisticResult and updateObservers calls. I tried for quite a while to get rid of the extra updates, but couldn't find a way that would still always return previous data, so eventually just left it like this and edited the tests. Any ideas to get rid of the extra updates are very welcome 馃槉

@vercel
Copy link

@vercel vercel bot commented Nov 1, 2021

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/tanstack/react-query/2UFSK4PWKCxrbTt2GSp7rDN7cFJ3
Preview: https://react-query-git-fork-jvuoti-fixusequeriesinfini-ae7ee3-tanstack.vercel.app

@codesandbox
Copy link

@codesandbox codesandbox bot commented Nov 1, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 296dbea:

Sandbox Source
tannerlinsley/react-query: basic Configuration
tannerlinsley/react-query: basic-typescript Configuration

Copy link
Collaborator

@TkDodo TkDodo left a comment

thank you. Regarding the extra re-renders: Are these only happening if we also have keepPreviousData: true on the queries, or are all queries affected? Judging from the unchanged tests, things should stay the same for keepPreviousData: false, right?

src/core/utils.ts Outdated Show resolved Hide resolved
src/core/queriesObserver.ts Outdated Show resolved Hide resolved
@TkDodo
Copy link
Collaborator

@TkDodo TkDodo commented Nov 6, 2021

When keepPreviousData is on, search for a query observer no longer used instead of relying on observer order and indexing. Trying to match observers by index could cause infinite request loops when switching queries. See #2680 for more

I'm not yet fully understanding how you're actually matching the observers together if not by index 馃

also, prettier formatting is failing, please have a look 馃憖

@jvuoti
Copy link
Contributor Author

@jvuoti jvuoti commented Nov 7, 2021

Thank you for the comments, good points, I now fixed the namings and formatting.

And oh yeah, you are right that this still fulfills the observer for previous data by index 馃槄 Just now the indexing is done from the list of observers that are no longer being used.

Previously the code fulfilled the observer by index from the main observer list, assuming the order of the queries would have stayed the same. However, that was not always the case when e.g. toggling queries off and on. This could then cause the same observer being returned first to an existing query (by hash) and then to a new query as previous data (by index), causing the infinite request loop. The new test should test for this issue.

The matching could be more advanced, to e.g. try to find the nearest match of the query key parts. However, this now at least should fix the biggest issue of infinite request loops. It also fulfills our use case of toggling between similar queries, with only an identifier being changed.

The extra re-renders do happen only when keepPreviousData is turned on. If I have understood correctly, this happens because now the getOptimisticResults also needs to call the same search and update code as updateObservers. Because getOptimisticResults does not update the internal observer map caches, firstObserver.setOptions gets called twice. This then seems to trigger the listeners also twice, even if the getOptimisticResults has left the notifyOptions undefined - or even when explicitly setting all the options false.

@codecov
Copy link

@codecov codecov bot commented Nov 7, 2021

Codecov Report

Merging #2866 (296dbea) into master (7948e7a) will increase coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2866      +/-   ##
==========================================
+ Coverage   96.41%   96.44%   +0.03%     
==========================================
  Files          45       45              
  Lines        2229     2248      +19     
  Branches      638      639       +1     
==========================================
+ Hits         2149     2168      +19     
  Misses         77       77              
  Partials        3        3              
Impacted Files Coverage 螖
src/core/queriesObserver.ts 100.00% <100.00%> (酶)
src/core/queryObserver.ts 100.00% <0.00%> (酶)

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 2714343...296dbea. Read the comment docs.

src/core/queriesObserver.ts Outdated Show resolved Hide resolved
@TkDodo
Copy link
Collaborator

@TkDodo TkDodo commented Nov 12, 2021

can you please update with the latest master, as we've added some more tests. And please make sure that the test coverage does not drop (it currently does, see the codecov report)

src/core/queriesObserver.ts Outdated Show resolved Hide resolved
src/core/queriesObserver.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@TkDodo TkDodo left a comment

can you please add a useQueries test for the initial issue, the infinite render loop? Thanks.

src/core/queriesObserver.ts Outdated Show resolved Hide resolved
jvuoti added 6 commits Nov 17, 2021
tannerlinsley#2680)

When keepPreviousData is on, search for a query observer no longer used
instead of relying on observer order and indexing.
Trying to match observers by index could cause infinite request loops
when switching queries.
You apparently can skip giving a queryKey, when given via options.
It will also work, somewhat.  All falsy queryKeys will match the same observer,
so if you only have a single query with undefined key, it will return the
expected value
This removes extra calls to setOptions from getOptimisticResult,
and gets rid of extra renders in tests.
setOptions is now explicitly only called in updateObservers.
This way no extra utils need to be added.
@jvuoti
Copy link
Contributor Author

@jvuoti jvuoti commented Nov 17, 2021

can you please add a useQueries test for the initial issue, the infinite render loop? Thanks.

The "should keep previous data when switching between queries" test added to useQueries.test does test the infinite render loop issue as a side effect: if you run that test against master, the expect on line 296 will fail, because there are going to be over 250 result states in the list 馃槵

But good point, it would be nice to have a simpler test explicitly for that issue. If needed, I can probably look at that over the weekend.

@TkDodo
Copy link
Collaborator

@TkDodo TkDodo commented Nov 17, 2021

But good point, it would be nice to have a simpler test explicitly for that issue. If needed, I can probably look at that over the weekend.

that would be great, thanks

@jvuoti
Copy link
Contributor Author

@jvuoti jvuoti commented Nov 21, 2021

Now added a test explicitly for the infinite render loop when toggling queries on and off, i.e. the initial issue from the bug report. It is still a bit cumbersome to set up and validate the issue, unfortunately couldn't find a way to get it much simpler than that.

TkDodo
TkDodo approved these changes Nov 26, 2021
Copy link
Collaborator

@TkDodo TkDodo left a comment

Thank you, I'm going to trust the test and merge this. Also went a bit through the logic and I think it's pretty readable now 馃殌

@TkDodo TkDodo merged commit c51498a into tannerlinsley:master Nov 26, 2021
8 checks passed
@tannerlinsley
Copy link
Owner

@tannerlinsley tannerlinsley commented Nov 26, 2021

馃帀 This PR is included in version 3.33.6 馃帀

The release is available on:

Your semantic-release bot 馃摝馃殌

return this.getObserver(defaultedOptions, index).getOptimisticResult(
defaultedOptions
)
})
return this.findMatchingObservers(queries).map(match =>
match.observer.getCurrentResult()
)
Copy link
Collaborator

@TkDodo TkDodo Dec 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

quick follow-up question here @jvuoti : previously, we called getOptimisticResult of each observer, and now we just call getCurrentResult(). I'm surprised that this doesn't seem to make a difference 馃 ...

Copy link
Contributor Author

@jvuoti jvuoti Dec 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you are right, it should call getOptimisticResult there, so that is a bug. Sorry, must have been some lazy copy-pasting on my part when refactoring the code around. 馃う

Testing this out quickly, the change does not seem to cause any visible effects - but it does now fix the issue I had with some of the existing tests still returning isFetching: false. So yeah, I probably should have noticed the issue from those tests. But at least now I can revert the tests back to the original, correct form 馃槄

So good catch, thank you so much! I'll prep a PR to fix the call and tests shortly.

Copy link
Collaborator

@TkDodo TkDodo Dec 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 馃檹

jvuoti added a commit to jvuoti/react-query that referenced this issue Dec 4, 2021
Fix queriesObserver to call getOptimisticResult as before.
This was a bug introduced in PR tannerlinsley#2866
TkDodo pushed a commit that referenced this issue Dec 4, 2021
鈥vers (#3052)

* fix(queriesObserver): fix getOptimisticResult call

Fix queriesObserver to call getOptimisticResult as before.
This was a bug introduced in PR #2866

* fix(queriesObserver): return observers in same order as queries

Sort returned observer entries to match the order of queries.
Previously, with keepPreviousData: true, the repurposed observers
would be returned last, potentially in different order than the queries.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants