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

feat: add swrNativeMiddleware to support refreshInterval option #25

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

javascripter
Copy link

@javascripter javascripter commented Nov 17, 2022

This PR introduces a new middlware API named swrNativeMiddleware that integrates with react-navigation's isFocused state and enables proper refreshInterval support in nested screens which improves performance as described in #22.

This PR still exports previous useSWRNative and useSWRNativeRevalidate API for backward compatibility. The new middleware will be the preferred way and will be stated as such in README.md.

This PR also supports 'swr/infinite' and 'swr/immutable' automatically if you use the middleware API because those hooks use middleware under the hood.

I've updated README.md (my best effort) but I'm not a native English speaker. Please feel free to edit it as you like.

@javascripter javascripter force-pushed the feat/swr-react-native--middlware branch 2 times, most recently from 4ec0654 to fe99d49 Compare November 17, 2022 17:30
@javascripter javascripter changed the title feat: add swrNativeMiddleware to support refreshInterval option WIP: feat: add swrNativeMiddleware to support refreshInterval option Nov 18, 2022
@javascripter javascripter force-pushed the feat/swr-react-native--middlware branch from 1fa276b to 6b7f9ac Compare November 18, 2022 11:05
@javascripter javascripter changed the title WIP: feat: add swrNativeMiddleware to support refreshInterval option feat: add swrNativeMiddleware to support refreshInterval option Nov 18, 2022
@javascripter javascripter marked this pull request as ready for review November 18, 2022 11:09
@nandorojo
Copy link
Owner

hey, i haven’t had time to review, sorry. my one concern is that this could be a big jarring for existing users of the library who are using the revalidation at a hook level. I wonder if there’s a way to smooth the transition over to avoid double-calling the revalidation events?

@danieldunderfelt
Copy link

Thanks, I added this to a project and it seems to work well. The only issue is that it needs to be added inside React-navigation contexts, which is probably unavoidable, but it's something to keep in mind. It works very different from how this library worked before.

@javascripter
Copy link
Author

javascripter commented Feb 7, 2023

@nandorojo
For double-calling caused by combination of swrNativeMiddleware and useSWRNative together like below, there isn't an easy way to detect double-calling without changing the middleware API, because we can't add React contexts arbitrarily to detect it in hook-based APIs.

function Component() {
  // INCORRECT! use useSWR imported from 'swr' directly here.
  // If you use useSWRNative here, it causes revalidation calls to be duplicated by internally calling
  // the same useSWRNativeRevalidate in both useSWRNative and swrNativeMiddleware
  const { data } = useSWRNative()
  return null
}
const App =  <SWRConfig value={{ use: [swrNativeMiddleware] }}>
  <Component />
</SWRConfig>

With that said, I believe most people are using it with a custom shared swr hook like below so the actual changes needed to adopt the new middleware API are probably straightforward and won't take much time.

function useSWRInfiniteNative(...args) {
  const [key, fn, _config] = normalize<any, any>(args)
  const swr = useSWRInfiniteRaw(key, fn, config)

  useSWRNativeRevalidate({
    mutate: swr.mutate,
    revalidateOnFocus: config.revalidateOnFocus,
    revalidateOnReconnect: config.revalidateOnReconnect,
    focusThrottleInterval: config.focusThrottleInterval,
  })
  return swr
}

In the case people where people are using useSWRNativeRevalidate per hook like below,

const { data, mutate } = useSWR(key, fetcher)

useSWRNativeRevalidate({
  // required: pass your mutate function returned by SWR
  mutate

  // optional, defaults copied from SWR
  revalidateOnFocus: true,
  revalidateOnReconnect: true,
  focusThrottleInterval: 5000,
})

The actual change we need to make is straightforward as well, and can be migrated incrementally as well by doing like below.

const { data, mutate } = useSWR(key, fetcher, {
  use: [swrNativeMiddleware]
})

This per-hook middleware usage also means we don't need react-navigation contexts in every swr hook, so nothing changed in that regard. cc @danieldunderfelt

So, I think this should be solved by documentation. I'm going to add a link to this comment in README so people can migrate to new APIs easily.
Update: addressed in c2dcde2

@javascripter javascripter force-pushed the feat/swr-react-native--middlware branch from 11e0131 to c2dcde2 Compare February 7, 2023 13:04
@javascripter
Copy link
Author

As I cannot reproduce the issue described in this comment (which can be addressed later without changing API surface if a reproduction is provided), I think there isn't any other remaining issue and the PR is ready for review to merge.

@danieldunderfelt
Copy link

danieldunderfelt commented Feb 10, 2023

I found an issue with this, where it won't revalidate data that was already activated. Scenario:

React-navigation mounts a stack of screens A and B, and the user enters A. B is also mounted but not focused. At this point, the SWR hook for B is set to paused by the middleware. When the user navigates to B, the paused status is not updated because SWR does not re-evaluate it. The data is only properly fetched when the refreshInterval is triggered.

You need to trigger SWR to re-evaluate the paused status when the screen is focused. I'll try various solutions in the app I am using this in.

Subscribing to React-navigation focus events should do the trick but requires testing to check for update loops:

useEffect(
  () =>
    // Ensure the unsubscribe function from addListener is returned.
    navigation.addListener('focus', () => {
      swr.mutate()
    }),
  [swr, navigation]
)

@javascripter
Copy link
Author

javascripter commented Feb 10, 2023

@danieldunderfelt
Thanks for the detailed explanation. It's probably the same issue described in #22 (comment)

Your workaround would ignore focusThrottleInterval etc. which I think is not suitable as a general-use library like this, so maybe we have to resort to the refreshInterval workaround anyway. It triggers a re-render when focus changes but I think it's a decent trade-off.
Could you try the below approach and check if it resolves the issue?

// @ts-ignore
import { useIsFocused } from '@react-navigation/native'

export const swrNativeMiddleware: Middleware = (useSWRNext) => {
  return (key, fetcher, config) => {
    const isFocused: boolean = useIsFocused()
    const shouldOverrideRefreshInterval = !config.refreshWhenHidden && !isFocused

    const swr = useSWRNext(key, fetcher, {
      ...config,
      refreshInterval: shouldOverrideRefreshInterval ? 0 : config.refreshInterval,
    })

    useSWRNativeRevalidate({
      mutate: swr.mutate,
      revalidateOnFocus: config.revalidateOnFocus,
      revalidateOnReconnect: config.revalidateOnReconnect,
      focusThrottleInterval: config.focusThrottleInterval,
    })

    return swr
  }
}

Jackman3005 added a commit to questmate/swr-react-native that referenced this pull request Jul 19, 2023
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 this pull request may close these issues.

3 participants