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

feat: make sure paused queries resolve #909

Merged
merged 1 commit into from
Aug 24, 2020

Conversation

boschni
Copy link
Collaborator

@boschni boschni commented Aug 21, 2020

This PR addresses three issues:

  • Make sure existing promises also resolve when fetching is paused and resumed because of a window unfocus.
  • Make sure existing promises also reject when fetching is cancelled. They will be rejected with a CancelledError.
  • Make sure queries are still cached when the transport layer does not support cancellation.
  • Set CancelledError as error to allow users to check if the query was cancelled (the isCancelledError helper function can be used to check if an error is a cancelled error).

@vercel
Copy link

vercel bot commented Aug 21, 2020

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/2fzpd8qp7
✅ Preview: https://react-query-git-fork-boschni-feature-query-cancellation.tannerlinsley.vercel.app

@tannerlinsley
Copy link
Collaborator

🎉 This PR is included in version 2.11.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@kentcdodds
Copy link
Member

FYI, with this release, I started seeing new act warnings. I'll probably investigate eventually, but I thought I'd bring it up here in case anyone knew what it could be and where the fix would be.

@kentcdodds
Copy link
Member

kentcdodds commented Aug 28, 2020

Oh, and also, with this change coming to React Testing Library it's even worse. I'm not sure why yet, but yeah, there's an issue with this change for sure.

@boschni
Copy link
Collaborator Author

boschni commented Aug 28, 2020

Hi @kentcdodds! I'm not familiar with the exact details of act, but when a query is used, multiple asynchronous processes are started in the background. These processes are based on timers and will eventually trigger updates to the component. The nature of this might make it difficult to properly wrap inside act. Would it help if react-query provides a way to know if all processes are settled? What I did in the react-query tests to fix them is to wait for an assertion which indicates these processes are settled or wait for some time with waitFor.

Based on the number of renders:

await waitFor(() => expect(states.length).toBe(4))

Based on some time:

export function waitForMs(ms: number) {
  const end = Date.now() + ms
  return waitFor(() => {
    if (Date.now() < end) {
      throw new Error('Time not elapsed yet')
    }
  })
}

@kentcdodds
Copy link
Member

Ok, here's an update. This appears to only be a problem when using fake timers (this test is the culprit).

If I add a act(() => jest.runOnlyPendingTimers()) to my afterEach config (when using mock timers) then this upcoming change isn't a problem (which is good news). However, I still get act warnings in either case. Still not sure how to avoid the act warning, but I think that getting a way to know when the processes are all settled could help.

I think what would help more is to make sure that when I call queryCache.clear(), any timers are canceled so state updates don't happen. I have a hunch that they're not canceled and that's what's causing the issue.

@kentcdodds
Copy link
Member

Ok, I'm pretty sure I know what's going on.

It looks like what happens is:

  1. Test runs and schedules some work
  2. Test finishes
  3. I clear the cache
  4. The scheduled work finishes and updates state <-- causes act warning
  5. The auto-cleanup functionality of React Testing Library unmounts the components

I can get rid of the act warning by making sure the component is unmounted before the scheduled work finishes by manually calling cleanup after I clear the cache or by using an empty waitFor(() => {}) after I clear the cache which will wait until the next tick of the event loop. This is probably the technically better way to accomplish this of those two options so I can be notified of any issues that are triggered as a result of those scheduled tasks actually completing while the component is still mounted.

But even better than an empty waitFor (which isn't a great solution because it's non-deterministic) is to have react-query expose some API that I can actually wait for. Something like: waitFor(() => expect(queryCache.tasks).toHaveLength(0)) or something.

@boschni
Copy link
Collaborator Author

boschni commented Oct 1, 2020

Hi @kentcdodds! You might be interested in this v3 change: 0f7c2af

It adds the ability to wrap all React Query updates with a custom function, like act. Which means you can wait in a test until things are settled and then run some assertions. Any updates while waiting will be wrapped inside act.

@kentcdodds
Copy link
Member

kentcdodds commented Oct 1, 2020

That is interesting! Thanks for letting me know @boschni!

What are the breaking changes looking like in v3? Is it going to be very impactful?

@boschni
Copy link
Collaborator Author

boschni commented Oct 2, 2020

No problem! It should be pretty easy to upgrade. A WIP guide can be found here: https://react-query-next.tanstack.com/guides/migrating-to-react-query-3

@devdigital
Copy link

I'm finding I also have to waitFor before each test to avoid race conditions when using cacheTime: 0:

beforeEach(async () => {
  await waitFor(() => {});
});

afterEach(async () => {
  queryCache.clear();
  await waitFor(() => {});
});

@kentcdodds
Copy link
Member

I think I've finally fixed everything for my use cases. Here's what I've got: https://github.com/kentcdodds/bookshelf/blob/49d7bb9a706e7c0b63ab128c37e2b8a4bc20da3d/src/setupTests.js#L29-L52

Structuring things this way should make it so your tests will work with or without fake timers 🎉 🦸

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants