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

useSWR calling fetcher sometimes too often in unit test #1874

Open
fabb opened this issue Feb 28, 2022 · 10 comments
Open

useSWR calling fetcher sometimes too often in unit test #1874

fabb opened this issue Feb 28, 2022 · 10 comments

Comments

@fabb
Copy link

fabb commented Feb 28, 2022

Bug report

Description / Observed Behavior

I'm using useSWR in a component that I test with react-testing-library. In most cases the unit test passes, but sometimes it fails when checking how often the fetcher of the useSWR hook has been called and with which parameters. The rendered dom looks fine though. Note that I change the key of the useSWR hook to represent filtering in my application. For unit tests I use the recommended <SWRConfig value={{ dedupingInterval: 0, provider: () => new Map() }}> to avoid flakiness. I don't use refreshInterval.

The unit test renders a component without a filter, then enters a search term into a filter input which updates the useSWR key, and after that, the search term is deleted again.

From the logs of one of the flaky failed tests I see this order of execution:

  • render component with useSWR with key ["some-endpoint", ""]
  • useSWR fetcher fetches with "searchText":""
  • render component with useSWR with key ["some-endpoint", ""]
  • render component with useSWR with key ["some-endpoint", ""]
  • render component with useSWR with key ["some-endpoint", ""]
  • render component with useSWR with key ["some-endpoint", ""]
  • searchText is changed to "my search text"
  • render component with useSWR with key ["some-endpoint", "my search text"]
  • render component with useSWR with key ["some-endpoint", "my search text"]
  • useSWR fetcher fetches with "searchText":"my search text"
  • render component with useSWR with key ["some-endpoint", "my search text"]
  • render component with useSWR with key ["some-endpoint", "my search text"]
  • render component with useSWR with key ["some-endpoint", "my search text"]
  • searchText is changed to ""
  • render component with useSWR with key ["some-endpoint", ""]
  • render component with useSWR with key ["some-endpoint", ""]
  • render component with useSWR with key ["some-endpoint", ""]
  • useSWR fetcher fetches with "searchText":"my search text"
    • ⚠️ that's weird - why does swr fetch this again, it just has fetched this in the last call to fetcher, also it mismatches the current key
  • render component with useSWR with key ["some-endpoint", ""]
  • useSWR fetcher fetches with "searchText":""

Expected Behavior

The fetcher function should not be called unnecessarily, and especially it should behave deterministically.

Repro Steps / Code Example

I do not yet have a reproduction repo yet, I'm still trying to make the error appear consistently in my own application's unit tests, but that's the issue with flakiness.

I'd be happy though to get hints on where I could add console.logs to find out the root cause. Or ideas on why swr could behave that way and call the fetcher with "searchText":"my search text" two times in a row (sometimes).

Additional Context

SWR version: 1.2.2

Note that the issue mostly happens on slower CI machines, so the theory is that the root cause is a race condition in the internal swr state. Maybe something like useSWR is rendered, puts in a request into its queue, then useSWR is rerendered and even though the deduping interval is 0, it does not yet see that the first request is enqueued because of some scheduling state issue, and enqueues another request. Could that be the case?

@fabb
Copy link
Author

fabb commented Feb 28, 2022

I tried downgrading swr to 0.5.6 during investigation, but the flaky behavior happened there too.

@icyJoseph
Copy link
Contributor

icyJoseph commented Mar 1, 2022

Hi,

This is indeed hard to trigger, I tried but couldn't.

A few observations:

  • Why does your fetcher, execute with ""? Even with [""] the fetcher should not be triggered.
  • Do you have any other hooks on the same rendering cycle?
  • Even if you can't share your code because it's private, perhaps you could translate to pseudo-code?
  • requestAnimationFrame, could sometimes misbehave depending on your test environment.

Good that you are trying to produce a test that should make things easier to test in case there's an actual bug. However, if you don't see this issue in production/development, then I'd point a finger at the testing environment.

@fabb
Copy link
Author

fabb commented Mar 1, 2022

Why does your fetcher, execute with ""? Even with [""] the fetcher should not be triggered.

I maybe over-simplified the pseudo-code. I have some more array items in the key.

Do you have any other hooks on the same rendering cycle?

There are a lot more hooks.

Even if you can't share your code because it's private, perhaps you could translate to pseudo-code?

It‘s too big, especially the rendered components. I‘m trying to compact it, but that takes a lot of time. I‘m hoping to rather find out more with console.logs in the swr code (happy for pointers).

However, if you don't see this issue in production/development, then I'd point a finger at the testing environment.

The rendered data looks fine, so the issue is just unnecessary calls to the fetcher which is not possible to spot in production. So I‘m not sure if these unnecessary api calls happen in production too. It‘s an issue because I have no other way of unit testing that the api is called with the correct parameters.

@fabb
Copy link
Author

fabb commented Mar 3, 2022

Ok, now I see an issue on production too, even though it's the opposite: the key of useSWR changes (and during debugging I see that useSWRHandler is called with the updated key multiple times), but the fetcher is not called with the updated array key.

@fabb
Copy link
Author

fabb commented Mar 3, 2022

I debugged through the reproducible issue on production (where the fetcher is not called even though the key updated), and it looks like the body of the useIsomorphicLayoutEffect that should react to the key change is not called, even though the key did change. useSWRHandler is called twice with the updated key. Further key changes work again, so I'm not sure why this happens, but it's consistent.

Screenshot 2022-03-03 at 13 02 12

Could this be a react bug in useLayoutEffect? I'm on 17.0.2.

@icyJoseph
Copy link
Contributor

Ah, and the SWR version?

I am not sure I see the problem here, please do help me understand.

key has the ALL_ACTIVE portion, while keyRef.current does not, isn't that what line 941 is showing? After the red marker, follows what keyRef.current is being assigned to.

I think we need @shuding on this one. However, I think we need a to start to think of a test case, or whether or not this is expected behavior, though I think not.

@fabb
Copy link
Author

fabb commented Mar 3, 2022

swr version is 1.2.2.

key has the ALL_ACTIVE portion, while keyRef.current does not, isn't that what line 941 is showing?

exactly. keyRef.current shows the last value where the closure of useIsomorphicLayoutEffect was executed, so it's also the last value of key in the dependency array of useIsomorphicLayoutEffect. but after that breakpoint in 972, the closure is not called, even though the key changed.

i could reproduce it both in a prod build and a dev build. in the dev build, i added console.logs to swr code, and saw the same behavior as in the browser debugger:

Screenshot 2022-03-03 at 16 43 23

i also logged the dependency array of useLayoutEffect in the react lib (not shown in the screenshot), and it updates correctly to the new key, though the closure is not executed. looks a lot like a react bug. i also tried replacing the useLayoutEffect with a useEffect, same result. i also tried updating react to 18.0.0.rc1, same behavior. i'm really confused what the issue here is and why it works in most other cases.

@fabb
Copy link
Author

fabb commented Mar 3, 2022

btw. my pseudocode is something like this (next.js app):

const useMySearch = () => {
  const router = useRouter()
  const filters = useMemo(()=>filterFromQuery(router.query),[router.query])
  const {data, error, mutate, isValidating} = useSWR(router.isReady ? arrayFromFilters(filters) : null, myFetcher)

  const updateFilters = (newFilters) => {
    // updating the url shallowly will update router.query which will implicitly update filters and trigger a fetch
    router.replace({
      href: urlFromFilters(router.asPath, newFilters, config),
      clientSideRouting: true,
      options: { shallow: true },
    })
  }

  return {data, error, mutate, isValidating, updateFilters}
}

this is vastly simplified though. note that executing updateFilters causes 2 rerenders in a row by next.js, one for the changed route and updated router objects, and another one for announcing the route to screen readers (small dom update).

@fabb
Copy link
Author

fabb commented Mar 4, 2022

ok, the useLayoutEffect error seems not to be connected to swr at all, i could reproduce it with a simple useEffect in my component. it seems to be related to a useRef that i use, but i don't see anything wrong with the usage. so no need to discuss the useLayoutEffect error here further. i'm not sure if the error from the original post is related though, so i'll keep this open, ok?

@fabb
Copy link
Author

fabb commented Mar 8, 2022

I created a reproducing example and a react bug ticket for the second issue: facebook/react#24042

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

No branches or pull requests

2 participants