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

QueryErrorResetBoundary not rest when i have enabled option #2930

Closed
WeiShengv99 opened this issue Nov 12, 2021 · 6 comments · Fixed by #2933
Closed

QueryErrorResetBoundary not rest when i have enabled option #2930

WeiShengv99 opened this issue Nov 12, 2021 · 6 comments · Fixed by #2933
Labels
bug Something isn't working released

Comments

@WeiShengv99
Copy link

Describe the bug
QueryErrorResetBoundary rest dont work with useQuery has changable 'enabled' option,
I don't know if this is the correct behavior?

To Reproduce
just click try again button
demolink
image

Expected behavior
i hope queryfn recall when i reset

Desktop (please complete the following information):

OS: MacOS
Browser: Chrome
Version: latest
Smartphone (please complete the following information):

Smartphone (please complete the following information):

  • Device: [e.g. iPhone6]
  • OS: [e.g. iOS8.1]
  • Browser: [e.g. stock browser, safari]
  • Version: [e.g. 22]

Additional context
Add any other context about the problem here.

@TkDodo TkDodo added the bug Something isn't working label Nov 12, 2021
@TkDodo
Copy link
Collaborator

TkDodo commented Nov 12, 2021

I think what happens is the following:

  • the component re-mounts, so the useState is set back to false
  • the query is still in error state
  • usually, mounting the query would make it go to loading state, but it doesn't because of enabled
  • because the query is in error state, the error is thrown.
  • because of that, react stops execution and the effect never runs
    --> you see the error boundary again

it's a bit tricky. I think what should happen is that the query goes from error to idle state because it's disabled and has no data yet. That way, we wouldn't re-throw the error. It could also potentially stay in error state, but I think with useErrorBoundary, we don't want queries in error state to actually render "normally" ...

@tannerlinsley
Copy link
Collaborator

🎉 This issue has been resolved in version 3.32.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@WeiShengv99
Copy link
Author

@TkDodo
This change made me think for some time,version 3.32.2 did solve my problem about this issue ,
But i find other problem in ohter case,sorry,i am not sure is this behavior right now ?

  1. enbale: false with reftch function
    demo
    in fact ,I have some code like this, because i need to get something when i click button😞,am I using reftch function incorrectly?
    or i should use useMutation when i click button

  2. change back to false
    if i change enable to true, I don't know if I can change back enable to false, seems like dependent-queries example!!userId fasle => ture => false, (if projects query's querykey dont include useId )I dont know what is right behavior if i change back to false,this one is a little confusing。

sorry ,my English is bad,i am a litter confusing about this.😵

@TkDodo
Copy link
Collaborator

TkDodo commented Nov 12, 2021

what you would need to do is:

  • have local state
  • on button click, set that state
  • disable the query when the state is falsy

I have forked your example here: https://codesandbox.io/s/refetch-with-enabled-false-forked-mmc9d?file=/src/index.js

However, you are right, the fix did introduce another "bug". Now, disabled queries that you run with refetch will never throw errors to an error boundary.

I need to think about how to solve that, because in the original issue, we explicitly did not want disabled queries to throw, but now we do :/

@WeiShengv99
Copy link
Author

WeiShengv99 commented Nov 12, 2021

what you would need to do is:

  • have local state
  • on button click, set that state
  • disable the query when the state is falsy

I have forked your example here: codesandbox.io/s/refetch-with-enabled-false-forked-mmc9d?file=/src/index.js

@TkDodo
Thanks for your help, i can do this to replace my refetch.
But i use refetch beacause

  1. queryKey: ['foo'],i need run queryFn when i click Button ,then queryFn is finish
  2. second click Button,i can use queryInstance.state === 'succeed' was directly get cache data
  3. queryKey change to queryKey: ['bar']
  4. then i click Button ,i can use queryInstance.state === 'idle' to tell me should i refetch query or get cache data

so if i use local state to do this, i should let enable change back to false when query is succeed, but we dont have api for get data callback(from cache or remote,i think i should't do effect in onSelect callback)
because i only need refetch when i click button, but if i use refetch method to do this, queryInstance always be disable.

it seems like side effect....
useMutation dont have cache ,so i use useQuery to implement this feather

then i found reset not work with error cache. this behavior is really strange.

However, you are right, the fix did introduce another "bug". Now, disabled queries that you run with refetch will never throw errors to an error boundary.

I need to think about how to solve that, because in the original issue, we explicitly did not want disabled queries to throw, but now we do :/

I have no idea about this, not only refetch , now enable:false behavior is never throw error, but we just want not throw error when we run reset, so i think the problem is we should clear error cache when we run reset function?

I edit demo code .you can see SecondCpt get error state in mount,but enable is false,the query state should be idle ? I think this behavior is most better .

btw: should i open a new issue or a discussions about this?

@TkDodo
Copy link
Collaborator

TkDodo commented Nov 12, 2021

@WeiShengv99 can you test with this PR please: #2935

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants