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

Always call onSuccess even with deduping? #1580

Open
Linksku opened this issue Oct 22, 2021 · 12 comments
Open

Always call onSuccess even with deduping? #1580

Linksku opened this issue Oct 22, 2021 · 12 comments
Labels
bug Something isn't working
Milestone

Comments

@Linksku
Copy link

Linksku commented Oct 22, 2021

Bug report

Description / Observed Behavior

In some scenarios, I'd expect onSuccess to be called, but it doesn't. E.g. a route "/foo" populate the state inside SWR's onSuccess. If I quickly navigate /foo -> /other -> /foo before the request from the first /foo finishes fetching, the onSuccess from the second /foo won't run. The result is that the state in the second /foo remains the initial state.

Expected Behavior

Since the first /foo already unmounted, I'd expected the onSuccess from the second /foo to run. Alternatively, there should be a way for configure SWR so onSuccess always runs, even if the request was deduped.

Additional Context

SWR version: 1.0.1

@Linksku Linksku closed this as completed Oct 23, 2021
@Linksku Linksku reopened this Oct 23, 2021
@shuding
Copy link
Member

shuding commented Oct 23, 2021

Thanks for opening this! By definition onSuccess and onError are only called with actual requests, because there’re just too many deduplicated revalidations, so these give the “global” events. But there’s another onDiscarded callback for deduped requests (added in latest beta in #1523), I think you can use it together with onSuccess to get the “local” loading state.

@Linksku
Copy link
Author

Linksku commented Oct 28, 2021

Thanks for the suggestion! I updated to 1.1.0-beta.6 and tried onDiscarded. It looks like onDiscarded is only called if shouldStartNewRequest is true. I can make shouldStartNewRequest false by disabling deduping, but if I disable deduping, then I don't need onDiscarded in the first place.

I feel like this is a common enough use-case that there should be a separate callback that always gets called, even if it was deduped.

@shuding
Copy link
Member

shuding commented Dec 24, 2021

I realized that I misunderstood your original message and it’s indeed a problem. Will figure out a way to fix it!

@shuding shuding added the bug Something isn't working label Dec 24, 2021
@michaelwschultz
Copy link

Running into this issue still. Any update on when or how it might be resolved?

@mko4444
Copy link

mko4444 commented Jul 15, 2022

@shuding is there any update on this? I'm getting a similar issue here and it's breaking my whole app.

@MarkoIvanetic
Copy link

MarkoIvanetic commented May 15, 2023

Issue is still happening. code sandbox to reproduce:
https://codesandbox.io/s/sweet-benz-ib4krx?file=/src/App.js

@isBatak
Copy link

isBatak commented May 15, 2023

React Query claims that onSuccess and onError are abd API design https://tkdodo.eu/blog/breaking-react-querys-api-on-purpose#a-bad-api and their plan is to remove those callbacks. Maybe SWR should do the same?!
cc @shuding

@BertrandBordage
Copy link

I faced the same issue.

In short, the solution for me was to disable deduping globally. I put this inside my <SWRConfig value={…}>:

// Disables deduping. Mandatory for requests that modify data, otherwise we can end up
// with broken frontend states when users are interacting quickly with the application.
dedupingInterval: 0,

In my case, there is no valid case for deduping. The "read" requests I do are cached and rarely revalidated, but the "write" requests can happen in a quick succession. An example is that you can log in, log out and log in again in less than 2 s, the default dedupingInterval. In my case, it lead to a state where it was impossible to log in again, unless you did a full page refresh.

I honestly think that deduping should be disabled by default, as it can be very misleading. Caching is already enabled by default, for most users it will be enough.

I do not have an strong opinion on whether onSuccess should be called when deduping. There could be harmful consequences with both behaviours.

Note that I tried applying @isBatak’s suggestion of using useEffect, but it is not reliable. onSuccess/onError should be kept. The problem with useEffect is that there is no universal way of checking that a query just occurred. In my case, I had a useMutation call to a /logout API endpoint that is returning true when the logout is successful. If you use something like:

useEffect(() => {
  if (data) {
    actualLogoutLogicLikeClearingAnInMemoryToken();
  }
}, [data]);

then once a first logout was triggered, data will always have the same value and never run again the useEffect callback, which also leads a broken state. There can be mitigations to clear data once the useEffect callback was called, but it is unnecessarily complicated and fragile.

@joshkel
Copy link
Contributor

joshkel commented Jan 21, 2024

@BertrandBordage - From our perspective, we rely on deduping fairly heavily. For example, three different components that are all mounted at different places in the component tree might need the same information; it's convenient to assume / rely on deduping and treat useSWR as a generic "ensure that this information is available", without having to worry about this generating extra requests.

If I understand your issue correctly, the rapid sequential requests are getting deduped even though there are mutation calls interspersed? If so, that sounds like it may be worth raising as a separate issue - mutation should override any dedupe interval.

@BertrandBordage
Copy link

@joshkel Yes, it's somewhat related to mutations, but not necessarily. It's related to queries that modify data on the backend, so roughly POST/PUT/PATCH requests, which can be done with useSWR, even though it's a bad practice.

In my case it's always through useMutation calls that I do POST/PUT/PATCH, so your approximation would work. I will open a new issue, then.

@BertrandBordage
Copy link

Sorry that I went slightly off topic… The problem I faced was not onSuccess not being called nor useSWRMutation being deduped.

It was a bug with the way the cache gets "cleared" that dedupes useSWR calls, returning undefined and of course not calling onSuccess. Full report and Codebase example here: #2877

@mikokofuyu
Copy link

I have also encountered the same issue where if a user does a back button journey to the page within the 2 second deduping interval then onSuccess is not called.

Is there any other alternative onFunction we can rely on to avoid using a useEffect to update state based on the response?

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

No branches or pull requests

10 participants