RevereCRE / relay-nextjs

⚡️ Relay integration for Next.js apps
https://reverecre.github.io/relay-nextjs/
MIT License
251 stars 30 forks source link

Shallow route change refetches data #73

Open FINDarkside opened 1 year ago

FINDarkside commented 1 year ago

When replacing route with something using shallow option, it seems like relay-nextjs still lets relay refetch the data. I'm not sure if that's intended as as far as I know, the only reason to use shallow routing would be to not trigger data fetching again. :thinking:

The usecase is to avoid refetching all of the data when you use filters in our site. We store these filter options in the url (using shallow replace), but it still triggers relay to refetch all of the data.

Relevant issues, but I made a new issue since the old ones don't really explain this particular issue: #47 and #45

rrdelaney commented 1 year ago

Hey! I just published a new version with updated logic for detecting if query variables have changed, this should help prevent unrelated query params causing a re-render.

FINDarkside commented 1 year ago

Our query vars do actually change, so I don't think it will fit my usecase. We're changing the query vars so that SSR works correctly when you refresh the page. But we'd rather not fetch everything the page needs, so we're also manually refetching the affected fragment when the filter is changed. This isn't a big issue at least for now, removing the manual refetch and letting relay handle the logic does remove some code from our end. But I can still think of scenarios where this would be useful. If there's some data that's heavy to load then it would be refetched every time even if it would be enough to fetch just one fragment.

If it ever becomes a big enough problem for us I can probably come up with some nice solution and make a pr :)

rrdelaney commented 1 year ago

Ah, we do something similar for table filters. Have you tried using useRefetchableFragment along with @arguments? That works for us and doesn't refresh the entire page

FINDarkside commented 1 year ago

We're using usePaginationFragment and we can use that to fetch only the data we need after filters have changed. But the problem is that when the user changes filter, we also update the url to match this new filter. And our variablesFromContext reads the filter from url so that SSR works correctly with filter. This means that at the moment we're first fetching only the data we need and the relay-nextjs will refetch all of the page data because the url has changed and the url change changed query variables.

rrdelaney commented 1 year ago

That's pretty much exactly what we do, but we pass the variables through to usePagination fragment. Here's an obfuscated example from our codebase:

  const { data, loadNext, hasNext, isLoadingNext } = usePaginationFragment<
    PaginatedDataQuery,
    lazyLoadedList_PaginatedData$key
  >(
    graphql`
      fragment lazyLoadedList_PaginatedData on DataRoot
      @argumentDefinitions(
        cursor: { type: "String" }
        count: { type: "Int", defaultValue: 50 }
        filter: { type: "String" }
      )
      @refetchable(queryName: "PaginatedDataQuery") {
        dataPages(
          first: $count
          after: $cursor
          filter: $filter
        ) @connection(key: "PaginatedData_dataPages") {
          totalCount
          edges {
            node {
              ...lazyLoadedList_DataRow
            }
          }
        }
      }
    `,
    query.dataRoot
  );
FINDarkside commented 1 year ago

Yes that's what we do, but the source of those variables are the url and changing the url will make relay-nextjs refetch the data.

That works for us and doesn't refresh the entire page

So you do update url to match the filter and also read the filter from url in variablesFromContext? We want SSR to work with filters as well, so we can't just drop these variables from variablesFromContext. So what I basically would want is that shallow route change wouldn't trigger relay refetch even if the variables have changed.

rrdelaney commented 1 year ago

So you do update url to match the filter and also read the filter from url in variablesFromContext?

Yep! Are you wrapping the component containing usePaginationFragment with a suspense boundary?

FINDarkside commented 1 year ago

Yep! Are you wrapping the component containing usePaginationFragment with a suspense boundary?

Yes, but we have useRouter hook inside _app which might be a problem if re-render is what would trigger the refetch on changed variables. I'll investigate this a bit more if this isn't the intended behavior. I'll come up with minimal project to repro next week if necessary.

rrdelaney commented 1 year ago

Sounds great! I’d be happy to help debug on a repro project. I do think the useRouter in _app may be causing some issues.

FINDarkside commented 1 year ago

Ok here's the repro: https://github.com/FINDarkside/relay-nextjs-issue-73-repro

Setup is what you'd expect, npm i && npm run dev. As can be seen from the network tab, when you click the button 2 requests are fired and the second one is made by relay despite the route replace being shallow. No useRouter inside _app in this one.

rrdelaney commented 1 year ago

Thank you for the repo, very easy to start debugging with this!

The issue causing the double fetch is this call to refetch. Relay will automatically refetch that data when the variables change, no need to call it manually yourself! This is due to relay automatically refetching when the variables from variablesFromContext change.

FINDarkside commented 1 year ago

Yes, but that's what I've been trying to explain and that's what this whole issue is about. You also told me to use refetchableFragment so now I'm quite confused. 🤔 The only usecase for useRefetchableFragment is to manually refetch data as far as I know. Anyway, if I just let relay auto-fetch, it'll mean that relay will fetch the whole page query. And as you can see that the first query only fetches the data that has changed, and the relay one fetches both variables.

rrdelaney commented 1 year ago

Yea it does seem like it's running the whole page query, but the correct components are suspending, I think I just assumed that only the correct queries were being made given that UI. There may be a way to have Relay only fetch the refetchable fragment, but I'm not sure.

FINDarkside commented 1 year ago

There may be a way to have Relay only fetch the refetchable fragment, but I'm not sure.

No I don't think there is, it's refetching the whole query on purpose even if some of the data already exists in cache. Right now with relay-nextjs I don't think there's any difference with shallow and normal navigation as both trigger refetch. I'm not sure if there's even any way to detect if something is shallow navigation or not. And then again I'm not sure if that'd even be the appropriate way to prevent nextjs from refetching stuff.

julioxavierr commented 1 year ago

+1

This feature would be very useful as it would allow us to use the query string from shallow routing to refetch data using a refetchable fragment, rather than re-rendering the entire page. I am interested in contributing if I can get some initial direction.