-
Notifications
You must be signed in to change notification settings - Fork 130
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: Allow using the cache when triggering requests manually #20
Conversation
} | ||
|
||
interface Options { | ||
manual: boolean | ||
} | ||
|
||
interface RefetchOptions { | ||
useCache: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed in the Issue, I think it would be better to allow a user to choose to use caching for the standard useAxios behaviour or refetch. If that was decision was made, useCache could move to the Options interface. As it stands though, having useCache in this new RefetchOptions interface works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not convinced we should have that option there to be honest. I mean the hook will execute whenever the component renders, meaning that having that option there and choosing to not use the cache will trigger a request whenever the component re-renders, which I see no reason why anybody would want to do. I think we should stick to the option being provided to the "refetch" function, with cache off by default, as in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"I see no reason why anybody would want to do" - I guess it depends how volatile the data is on the server. TBH I am happy with what you have proposed so I reckon you should go ahead and merge the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really, because you shouldn't rely on the frequency with which a component renders to carry out any application logic. That should be transparent for the developer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, fair enough. It's just that cache invalidation is always a nightmare! Good work @simoneb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know. If you've used Apollo you know that it's really not easy to implement and use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment. Other than that looks good.
Released in 1.2.0 |
Closes #19