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

Feature request: Enable caching for manual requests #19

Closed
8enSmith opened this issue Jul 17, 2019 · 8 comments · Fixed by #20
Closed

Feature request: Enable caching for manual requests #19

8enSmith opened this issue Jul 17, 2019 · 8 comments · Fixed by #20

Comments

@8enSmith
Copy link
Contributor

When using manual mode (i.e. when the manual option is set to true) the execute function returned does not use the cache i.e.

return [
    state,
    configOverride => {
      return executeRequestWithoutCache(
        { ...config, ...configOverride },
        dispatch
      )
    }
  ]

It would be great if a user could choose to use the manual execute function which does have caching enabled.

The scenario I have is where a user can undo an operation, which manually triggers execute, but I'd like to use a cached result instead of going back to the server.

Thoughts?

@simoneb
Copy link
Owner

simoneb commented Jul 17, 2019

Not sure I see understand the scenario. The assumption is that if you execute the query manually, then surely you want to execute it, right?

@8enSmith 8enSmith changed the title Feature Request: Enable caching for manual requests Feature request: Enable caching for manual requests Jul 17, 2019
@8enSmith
Copy link
Contributor Author

8enSmith commented Jul 17, 2019

Not sure I see understand the scenario. The assumption is that if you execute the query manually, then surely you want to execute it, right?

So I have a page using a input control. When I enter data into the control it triggers an onChange event where the handler then makes use of execute (returned from useAxios). If a user deletes text from the control then execute will correctly get called, however I'd like to use a cached result instead of going back to the server to recalculate the result.

@simoneb
Copy link
Owner

simoneb commented Jul 17, 2019

Okay that makes sense now. So this would be a breaking change if we change the manual query to use the cache by default. Let me think if there is a non-breaking way to introduce it, otherwise I'll have to bump major.

@8enSmith
Copy link
Contributor Author

Yes, either the behaviour of manual option would need to change or you'd need to introduce an additional option to control the caching.

The minor criticism I have (the work you've done on this repo is great!) is that I'd expect consumers (e.g. me!) would expect that the manual option just controls whether the hook runs automatically or not. I was surprised when I looked at the code to find that as well as doing this is also implicitly controls the caching behaviour. Hence a re-think in this area maybe warranted.

@simoneb
Copy link
Owner

simoneb commented Jul 17, 2019

@8enSmith fair point. the reason why it behaves like that is that historically that function was called refetch and it was used to, well, refetch the data of a query, and for that reason no caching in there was the only sensible behavior. Once the manual option was introduced that's not the only sensible behavior anymore though, but that's where it's coming from.

@simoneb
Copy link
Owner

simoneb commented Jul 17, 2019

@8enSmith can you have a look at #20 and let me know what you think? briefly, I extended the signature of the "refetch" function to accept an optional second parameter (the first is axios options overrides and needs to stay there) which allows to use the cache. it's off by default to avoid breaking changes and because I still think that it's the safe default

@8enSmith
Copy link
Contributor Author

I'd suggest giving users an option to opt in to caching for the standard or refetch behaviour, but its your call; I won't be surprised if someone asks for caching to be switchable for the standard hook behaviour.

I'll review the PR as it sounds like it'll fix the problem I have in my scenario.

@verekia
Copy link
Contributor

verekia commented Sep 29, 2019

Opened an issue at #33 for that scenario :)

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

Successfully merging a pull request may close this issue.

3 participants