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

Clear dedupingInterval of an unmounted key #828

Closed
lorenzomigliorero opened this issue Dec 17, 2020 · 17 comments · Fixed by #1498
Closed

Clear dedupingInterval of an unmounted key #828

lorenzomigliorero opened this issue Dec 17, 2020 · 17 comments · Fixed by #1498
Assignees
Labels
discussion Discussion around current or proposed behavior

Comments

@lorenzomigliorero
Copy link

lorenzomigliorero commented Dec 17, 2020

Bug report

Description

I am developing a simple CRUD application with two views:

App.jsx

<Route path="/" component={User} />
<Route path="/edit" component={Edit} />

User.jsx

const { data } = useSWR('/api/user', fetcher, { dedupingTime: 60000 });

return (
  <h1>{data.name}</h1>
)

Edit.jsx

const editName = (name) => {
  fetch('/api/user/edit', { method: 'post', name })
  mutate('/api/user') // here User.jsx is unmounted
}

return (
  <button onClick={editName}>EDIT NAME</button>
)

If the user follows these steps:

  • Navigate to '/'
  • Navigate to '/edit'
  • Click on edit name button
  • Come back to '/' after 1 second

/api/user data contains an old cached value.

I've tried to manually delete the cache value instead of calling mutate, but the issue remains.
CONCURRENT_PROMISES remains fullfilled with the previous payload.

const editName = (name) => {
  fetch('/api/user/edit', { method: 'post', name })
  cache.delete('/api/user')
}

Expected Behavior

After calling mutate on Edit.jsx, I expect that a revalidation occurs when User.jsx is mounted, ignoring dedupingInterval value.
In other words, dedupingInterval should be cleared if mutate is called on an unmounted key.

@lorenzomigliorero lorenzomigliorero changed the title Mutate a non mounted key Force a revalidation of an unmounted key Dec 17, 2020
@lorenzomigliorero lorenzomigliorero changed the title Force a revalidation of an unmounted key Force revalidation of an unmounted key Dec 17, 2020
@lorenzomigliorero lorenzomigliorero changed the title Force revalidation of an unmounted key Clear dedupingInterval of an unmounted key Dec 18, 2020
@smddzcy
Copy link

smddzcy commented Apr 5, 2021

Any workaround for this? It's really important to be able to cache some requests with long dedupingIntervals, but also mutate them when required (e.g. after the user updates some setting on another page). Currently, we cannot use long dedupingIntervals because of this issue.

@shuding shuding self-assigned this Apr 6, 2021
@shuding shuding added the bug Something isn't working label Apr 6, 2021
@MoonBall
Copy link
Contributor

MoonBall commented Apr 9, 2021

@lorenzomigliorero @smddzcy I made a demo https://codesandbox.io/s/relaxed-hooks-chjyi?file=/src/App.js.
I didn't reproduce this issue. PTAL

@smddzcy
Copy link

smddzcy commented Apr 13, 2021

@MoonBall Because the prop is not dedupingTime, it's dedupingInterval. Probably the name has been changed since Dec 2020 :)

See: https://codesandbox.io/s/summer-waterfall-7j93r

@MoonBall
Copy link
Contributor

I would like to fix it ASAP

@promer94
Copy link
Collaborator

This problem is a duplicate of #751

You could update the cache data in this way

mutate('/api/user', fetch('/api/user/edit', { method: 'post', name })) 

@kylemh
Copy link

kylemh commented Apr 26, 2021

I think he was merely providing a workaround, not saying this isn't a bug.

@promer94
Copy link
Collaborator

fetch function is already provided in the global provider, why do we have to give the 2nd parameter here again

The global mutate function lives outside the react component but you could only access the global fetcher from the SWRConfigContext. So you have to tell mutate how you would like to change your cache.

The global mutate function does two things

  • It will update the cache based on the second parameter
  • It will update the mounted hook with the latest cache data and force it to revalidate the cache based on hook's fetcher

So if you call mutate('key')

  • It will not update the cache because there is no data or function could be used to update the cache
  • It will try to update the mounted hook but if hook is unmounted, it could do nothing

Personally when i use swr for crud i would prefer Bound Mutate. In this way you might have batter DX because the typescript intellisense

  • useUser.js
funciton useUser() {
  return useSWR('/api/user', fetcher)
}
  • User.jsx
function User() {
  const { data } = useUser()
  return <div>{data}</div>
}
  • EditUser.jsx
function EditUser() {
  // if you dont access data here this hook would not cause rerender here
 // like a magic
  const { mutate } = useUser()
  const editName = async (name) => {
    await fetch('/api/user/edit', { method: 'post', name })
    mutate()
 }
  return <div>{data}</div>
}

I hope this could help people who have similar problems.

The global mutate function might be more suitable for prefetching or building some custom hooks like #1041

@kylemh
Copy link

kylemh commented Apr 26, 2021

If I use bound mutates, I opt into renders/fetches in areas where I might not need it. I actually usually avoid bound mutates for this reason.

@promer94 the problem is even if I set the 2nd argument, nothing changes if a relevant SWR hook isn't mounted... UNLESS the interval has passed. I think it would make sense to have mutate clear the interval so that the next time a relevant component mounts, it revalidates.

@promer94
Copy link
Collaborator

promer94 commented Apr 27, 2021

if I set the 2nd argument, nothing changes if a relevant SWR hook isn't mounted

@kylemh
Could you provide a codesandbox to reproduce this problem ? It would be really helpful.

@MoonBall
Copy link
Contributor

The mutate() has the third param shouldRevalidate, But calling mutate('key', undefined, true) doesn't clean previous request result. I think it is the root cause of this issue.

Although there is no mounted hook, we can simply delete the previous request result. It seems like that the revalidate() has happened for specific key.

@kylemh
Copy link

kylemh commented Apr 27, 2021

I ran into a bug making the repro 😅 codesandbox/codesandbox-client#5704

I'll get it to you as soon as I can.

@MoonBall
Copy link
Contributor

I have a better a design idea for shouldRevalidate parameter. This parameter should be used to revalidate specific key rather than mounted hooks.

@shuding shuding added discussion Discussion around current or proposed behavior and removed bug Something isn't working labels Jul 21, 2021
@muqg
Copy link

muqg commented Sep 25, 2021

I am currently just using a simple workaround, where I manually fetch the resource and directly insert it into cache like this:

function useRefresh() {
  const {cache, fetcher} = useSWRConfig()

  return (key) => {
    const shouldRefetch = !!cache.has(key)

    if(shouldRefetch) {
      return mutate(key, fetcher(key))
    }
  }
}

Just use this hook, instead of mutate directly and will have your resource refreshed, if it was previously present in cache. Seems to be working fine on stable version 1.0.1

What I wonder is why can't this behaviour be the default one for a simple mutate call? I haven't found a single case where I would rather call mutate than my refresh implementation above.

@shuding
Copy link
Member

shuding commented Sep 26, 2021

What I wonder is why can't this behaviour be the default one for a simple mutate call?

@muqg If there's a global fetcher specified, you solution looks good but it's the same as doing mutate(key, fetcher(key)) (known fetcher). But sometimes fetcher isn't specified and SWR can't fetch the data but mark the key as invalidated.

@muqg
Copy link

muqg commented Sep 26, 2021

@shuding Right, but then we can just fallback to a regular mutate(key) call in case that a global fetcher is not provided, yet still have this behavior by default when there is a configured global fetcher.

What's more doing a mutate(key, fetcher(key)) opts out of some built-in features such as deduping, but that one in particular I think is irrelevant, since I can't think of a single case where one would do mutate(key, fetcher(key)) multiple times following a single mutation request to the server.

However, when mutate(key) is called in a case where not mounted component has called useSWR(key) I would expect the behavior logic to follow something like this:
image

This I think illustrates the bigger issue with my expectations about what a mutate call should do when no mounted component is calling useSWR(key) at this very moment. Doing a mutate(key) should at the very least remove the entry from cache in order to allow the useSWR(key) call to fetch a fresh resource. In this way, even if useSWR is using a locally defined fetcher, we are still not going to be left with a stale entry. Yes, if we remove an entry from cache, this is probably going to pop up the loading indicator once again the next time a given component is mounted, but isn't stale (and possibly inconsistent) data display worse than a janky loading indicator?

Currently calling mutate(key) when no mounted component is calling useSWR(key) leaves us with stale data as described in my other issue #751.

@shuding
Copy link
Member

shuding commented Sep 26, 2021

Thank you @muqg for taking time making that diagram, it's very helpful!

I completely agree with this:

CleanShot 2021-09-26 at 18 12 27@2x

And I've opened #1498 to address this bug. But my main concern is cache.delete(key). Because of the nature of stale-while-revalidate, even if the key is marked as stale, SWR should still return the stale data while revalidating.

@muqg
Copy link

muqg commented Sep 27, 2021

@shuding Agreed, cache.delete is out of question! Still, as long as revalidation happens the next time a component with useSWR(key) is mounted, after a mutate(key) was issued to mark the key stale, I believe the meaning behind stale-while-revalidate will then be complete.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion Discussion around current or proposed behavior
Projects
None yet
7 participants