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.

Sign up for GitHub

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

Jump to bottom

fix: not clear promise of revalidate after mutating #1139

Closed
wants to merge 2 commits into from
Closed

fix: not clear promise of revalidate after mutating #1139

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
800a882
fix: not clear promise of revalidate after mutating
MoonBall Apr 25, 2021
7e07e30
feat: should respect `shouldRevalidate` of `trigger`
MoonBall Apr 26, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions src/use-swr.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ const trigger: Trigger = (_key, shouldRevalidate = true) => {

const updaters = CACHE_REVALIDATORS[key]

if (key && updaters) {
if (key && updaters && updaters.length) {
const currentData = cache.get(key)
const currentError = cache.get(keyErr)
const currentIsValidating = cache.get(keyValidating)
Expand All @@ -82,6 +82,11 @@ const trigger: Trigger = (_key, shouldRevalidate = true) => {
}
// return new updated value
return Promise.all(promises).then(() => cache.get(key))
} else if (shouldRevalidate) {
// `updaters` would trigger revalidate if `updaters.length > 0`.
// So only clear promise of revalidate if `!updaters.length && shouldRevalidate`
delete CONCURRENT_PROMISES[key]
delete CONCURRENT_PROMISES_TS[key]
}
return Promise.resolve(cache.get(key))
}
Expand Down Expand Up @@ -168,7 +173,7 @@ async function mutate<Data = any>(
// enter the revalidation stage
// update existing SWR Hooks' state
const updaters = CACHE_REVALIDATORS[key]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be batter if we delete the promise conditionally

Suggested change
const updaters = CACHE_REVALIDATORS[key]
const updaters = CACHE_REVALIDATORS[key]
if (updaters && updaters.length > 0) {
const promises = []
for (let i = 0; i < updaters.length; ++i) {
promises.push(
updaters[i](!!shouldRevalidate, data, error, undefined, i > 0)
)
}
// return new updated value
return Promise.all(promises).then(() => {
if (error) throw error
return cache.get(key)
})
} else {
// clear dedupingInterval of an unmounted key
delete CONCURRENT_PROMISES[key]
delete CONCURRENT_PROMISES_TS[key]
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be batter if we delete the promise conditionally

Thanks for your suggestions.

I deleted the promise conditionally at first. I think the conditional statement should be !!shouldRevalidate && (updaters && updaters.length > 0) if deleting it conditionally. This conditional statement is little of complicated and hard to understand.

However, Deleting the promise after mutating is easier to understand. I would like to modify my comment to express it clearly.

if (updaters) {
if (updaters && updaters.length) {
const promises = []
for (let i = 0; i < updaters.length; ++i) {
promises.push(
Expand All @@ -180,6 +185,11 @@ async function mutate<Data = any>(
if (error) throw error
return cache.get(key)
})
} else if (shouldRevalidate) {
// `updaters` would trigger revalidate if `updaters.length > 0`.
// So only clear promise of revalidate if `!updaters.length && shouldRevalidate`
delete CONCURRENT_PROMISES[key]
delete CONCURRENT_PROMISES_TS[key]
}
Copy link
Collaborator

@promer94 promer94 Apr 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if hook is unmounted

mutate('key') // This will not clear the deduplicatedInterval

mutate("key", () => undefined) // This will clear the deduplicatedInterval
mutate("key", () => Promise.resolve(undefined)) // This will also clear the deduplicatedInterval

swr/src/use-swr.ts

Line 107 in f787624

if (typeof _data === 'undefined') return trigger(_key, shouldRevalidate)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if hook is unmounted, the above three cases will not clear the deduplicatedInterval because updaters && updaters.length is true.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I has an irrelevant problem. How did you embed code like this:

image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://docs.github.com/cn/github/managing-your-work-on-github/creating-a-permanent-link-to-a-code-snippet

// throw error or return data to be used by caller of mutate
if (error) throw error
Expand Down
105 changes: 103 additions & 2 deletions test/use-swr-revalidate.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { act, fireEvent, render, screen } from '@testing-library/react'
import React from 'react'
import useSWR from '../src'
import React, { useEffect, useState } from 'react'
import useSWR, { mutate, trigger } from '../src'
import { createResponse, sleep } from './utils'

const waitForNextTick = () => act(() => sleep(1))
Expand Down Expand Up @@ -136,4 +136,105 @@ describe('useSWR - revalidate', () => {
await act(() => sleep(70))
screen.getByText('count: 2')
})

it('should respect `shouldRevalidate` of `mutate` even though there is no mounted hook', async () => {
const swrKey =
'should respect `shouldRevalidate` of `mutate` even though there is no mounted hook'
function A() {
const { data } = useSWR(
swrKey,
async () => {
return 'A'
},
{
dedupingInterval: 6000
}
)
return <>{data}</>
}

function B() {
useEffect(() => {
mutate(
swrKey,
async () => {
return 'B'
},
true
)
}, [])
return null
}

function Page() {
const [isShowA, setIsShowA] = useState(true)

return (
<button
onClick={() => {
setIsShowA(!isShowA)
}}
>
click me {isShowA ? <A /> : <B />}
</button>
)
}

render(<Page />)

await act(() => sleep(20))
fireEvent.click(screen.getByText('click me A'))
await act(() => sleep(20))
fireEvent.click(screen.getByText('click me'))
await act(() => sleep(20))
screen.getByText('click me A')
})

it('should respect `shouldRevalidate` of `trigger` even though there is no mounted hook', async () => {
const swrKey =
'should respect `shouldRevalidate` of `trigger` even though there is no mounted hook'
let count = 0
function A() {
const { data } = useSWR(
swrKey,
async () => {
return ++count
},
{
dedupingInterval: 6000
}
)
return <>{data}</>
}

function B() {
useEffect(() => {
trigger(swrKey, true)
}, [])
return null
}

function Page() {
const [isShowA, setIsShowA] = useState(true)

return (
<button
onClick={() => {
setIsShowA(!isShowA)
}}
>
click me {isShowA ? <A /> : <B />}
</button>
)
}

render(<Page />)

await act(() => sleep(20))
fireEvent.click(screen.getByText('click me 1'))
await act(() => sleep(20))
fireEvent.click(screen.getByText('click me'))
await act(() => sleep(20))
screen.getByText('click me 2')
})
})