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

Shallow route change refetches data #73

Open
FINDarkside opened this issue Aug 16, 2022 · 15 comments
Open

Shallow route change refetches data #73

FINDarkside opened this issue Aug 16, 2022 · 15 comments

Comments

@FINDarkside
Copy link
Contributor

FINDarkside commented Aug 16, 2022

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. 🤔

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

@FINDarkside FINDarkside changed the title Shallow route refetches data Shallow route change refetches data Aug 16, 2022
@rrdelaney
Copy link
Member

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
Copy link
Contributor Author

FINDarkside commented Aug 18, 2022

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

FINDarkside commented Aug 19, 2022

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
Copy link
Member

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
Copy link
Contributor Author

FINDarkside commented Aug 20, 2022

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
Copy link
Member

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
Copy link
Contributor Author

FINDarkside commented Aug 22, 2022

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
Copy link
Member

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
Copy link
Contributor Author

FINDarkside commented Aug 22, 2022

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
Copy link
Member

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
Copy link
Contributor Author

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
Copy link

+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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants