facebook / relay

Relay is a JavaScript framework for building data-driven React applications.
https://relay.dev
MIT License
18.37k stars 1.82k forks source link

Data not being refetched when released from cache. Getting `fragmentresource.missing_data` when trying to access released data. #4278

Open comatory opened 1 year ago

comatory commented 1 year ago

This is a TL;DR of the problem, you can also find the explanation in this repository which contains runnable example. Please refer to that repository for more screenshots and images if you find the explanation below unsatisfactory.

I am describing this problem with Github API but I have run into the same problem when using my company's internal API, it does not seem to be related to server at all.

I have a component that uses useLazyLoadQuery hook with combination of usePaginationFragment hook to search through repositories. I use <input> as a filter. When I type into the input box value 123456789abc somewhat "quickly" I fire off multiple requests.

At this point all works as expected but as soon as I start deleting characters from the input box and I get to just one character 1 being left into the input box, instead of Relay refetching the data I get undefined.

This is what UI looks like:

video01.webm

My code contains logging function which in this situation prints out fragmentresource.missing_data. Looking at the source code it points to this part of the codebase.

The generated Typescript types seem to be unreliable at this point since the result should be data or null but in my case it's undefined.

I'm using default settings for my environment. This means the garbage collector retains 10 items and my network policy is store-or-network. The issue is that the missing data should be re-fetched as far as I know and not end up undefined.

I tried debugging this problem with Relay dev tools and I can see older data being released as soon as I get to 10th or 11th character typed into the input box so I confirmed that the data is indeed not there.

This does not happen if I let the requests finish (!). This means that if I type character one-by-one and then start deleting the characters, the data is actually refetched, in UI it looks like this:

video03.webm

Notice that when I get to single 1 character, the loading UI is shown.

I realize that in real world scenario one would add throttle function to the input but in any case, I would expect Relay to be more robust in this regard. I also know that I could increase the limit of items being retained in the garbage collector but that does not really solve the problem as it would appear sooner or later (I'm using constrained example here on purpose).

I could use defensive code and check !data.search but that feels like a work-around and my UI will end up in inconsistent state. Since this is not "error" it will not bubble up to error boundary.

I could replace useLazyLoadQuery with combination of useQueryLoader + usePreloadedQuery which according to documentation makes sure to retain data in the cache as long as the component is mounted. However in this scenario it might not be the best solution as it could lead to higher memory consumption when user is using the search for too long and again, it doesn't really explain why data is not being re-fetched.

After researching this for couple of days I concluded that this is most likely a bug. If I'm using Relay in a wrong/non-idiomatic way, could you please point it out? I tried reaching to community via Discord but got no answer there.

Please refer to my example in the repository I mentioned above: https://github.com/productboard/relay-missing-data-bug

comatory commented 1 year ago

This problem is also being discussed in this Discord thread.

I got a suggestion to wrap setSearch setter with startTransition so that's what I did in branch try-using-start-transition in the example repository (here), it looks like this now:

  const handleInputChange = useCallback((e: React.ChangeEvent<HTMLInputElement>) => {
    startTransition(() => {
      setSearch(e.target.value);
    });
  }, []);

This helps with the problem. For some reason it retains some queries for longer so you you're less likely to miss the cache but even if I try to search for a term that has already been cleared from cache, it refetches the data.

It still doesn't help me much though because the codebase I'm working on does not have concurrent mode enabled yet, I assume startTransition is only compatible with that.

Relay documentation doesn't mention anywhere that using these optimization functions/hooks are required so I still this consider as unresolved problem.

Why does this happen?

PedroBern commented 1 year ago

I haven't tested it, but I'm curious if using the refetch function would produce the same issue. I added the refetch function to your code, can you try it?

export const RepoSearch = ({ initialQuery }: { initialQuery: string }) => {
  const [search, setSearch] = useState(initialQuery)

  const queryRef = useLazyLoadQuery<repoSearchQuery>(repoSearchQueryString, { query: initialQuery })

  return (
    <>
      <input onChange={(e) => setSearch(e.target.value)} />
      <Suspense fallback="Loading results...">
        <ResultList
          queryRef={queryRef}
          search={search}
        />
      </Suspense>
    </>
  )
}
export const ResultList = ({ queryRef, search }: {
  queryRef: repoSearchRepositories$key;
  search: string
}) => {
  const { data, loadNext, hasNext, refetch } = usePaginationFragment(
    repoSearchRepositoriesFragmentString,
    queryRef
  );

  useEffect(() => {
    refetch({ search })
  }, [refetch, search])

  invariant(data.search !== undefined, "Missing data.")

 // ...
}
comatory commented 1 year ago

@PedroBern Hey thanks for the response 😉

I've tried what you suggested, the code is in this branch and it does seem to work.

Moreover I can leave out refetch completely and it will also circumvent the issue, simply passing prop search fixes the issue. I still don't understand this problem though. Most likely passing search is forcing ResultList component re-render.

MartinNuc commented 1 month ago

Hello, thank you @comatory for such a great description of the issue. I was actually dealing with exactly this issue!

In my case, it manifested as undefined value of a connection from usePaginatedFragment. The rest of the data was there and only the connection field was undefined.

This is quite unfortunate as Typescript marks this field as always defined which resulted in a crash in a runtime.

comatory commented 1 month ago

Yes it's frustrating because you can't catch these during build time! I wish there'd be a way to instrument this issue better to uncover the root cause more easily.