Skip to content
This repository has been archived by the owner on Apr 13, 2023. It is now read-only.

3.1.0 [regression] stopPolling cleanup in effect #3482

Closed
mpgon opened this issue Sep 10, 2019 · 3 comments · Fixed by #3485
Closed

3.1.0 [regression] stopPolling cleanup in effect #3482

mpgon opened this issue Sep 10, 2019 · 3 comments · Fixed by #3485
Assignees

Comments

@mpgon
Copy link

mpgon commented Sep 10, 2019

Intended outcome: stopPolling should be correctly called on effect cleanup

Actual outcome: Error: Uncaught [TypeError: Cannot read property 'stopPolling' of undefined]

How to reproduce the issue:
When using 3.0.1 the following component was successfully unit tested

const TestComponent = () => {
  const { .., startPolling, stopPolling } = useQuery(..);

  useEffect(() => {
    startPolling(..);
    return () => stopPolling();
  }, [startPolling, stopPolling]);

  // ..
};

however, after upgrading to 3.1.0 I get the following error in my unit tests
Error: Uncaught [TypeError: Cannot read property 'stopPolling' of undefined]

Reproduction example https://github.com/mpgon/stopPollingError

@hwillson
Copy link
Member

Thanks for reporting this @mpgon. This is because of #3273, which automatically takes care of stopping polling when a component unmounts. You no longer have to call stopPolling in the function returned from your useEffect. That being said, your issue brings two important things to light:

  1. I forgot to mention Fix useQuery keeps polling bug from #3272 #3273 in the Changelog 🤦‍♂.
  2. We should probably throw a more helpful error, or noop the call, if stopPolling is called like you're calling it. That error isn't very helpful.

I'll get these changes in place, but for now you should be okay to just remove your stopPolling call. Thanks!

@mpgon
Copy link
Author

mpgon commented Sep 11, 2019

Alright, I'll do that then. Thank you!

@mpgon mpgon closed this as completed Sep 11, 2019
hwillson added a commit that referenced this issue Sep 11, 2019
Polling is automatically stopped when a component is unmounted, so
it isn't necessary to call `stopPolling` during component cleanup.
If it is called though, we don't want to throw an exception, so
these changes make it (and `startPolling`) a no-op if component
cleanup has already happened.

Fixes #3482
hwillson added a commit that referenced this issue Sep 11, 2019
Polling is automatically stopped when a component is unmounted, so
it isn't necessary to call `stopPolling` during component cleanup.
If it is called though, we don't want to throw an exception, so
these changes make it (and `startPolling`) a no-op if component
cleanup has already happened.

Fixes #3482
hwillson added a commit that referenced this issue Sep 11, 2019
Polling is automatically stopped when a component is unmounted, so
it isn't necessary to call `stopPolling` during component cleanup.
If it is called though, we don't want to throw an exception, so
these changes make it (and `startPolling`) a no-op if component
cleanup has already happened.

Fixes #3482
@muhanad40
Copy link

muhanad40 commented Oct 3, 2019

Hi, this bug is not fully fixed. Here's another case for you to consider and hopefully fix. I have a query hook that I pass skip: true. Then based on some condition, I call startPolling and based on another, I call stopPolling. When the component unmounts, it's still polling in the background. I believe it's because of the skip being set to true initially, but not sure how else to start/stop polling conditionally.

Also, please don't forget to log some sort of warning message because other people won't know why the stopPolling is not working silently.

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

Successfully merging a pull request may close this issue.

3 participants