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

Cached data bug after page navigation with next-urql + exchange-graphcache #3160

Closed
3 tasks done
dizlexik opened this issue Apr 18, 2023 · 4 comments · Fixed by #3165
Closed
3 tasks done

Cached data bug after page navigation with next-urql + exchange-graphcache #3160

dizlexik opened this issue Apr 18, 2023 · 4 comments · Fixed by #3165
Labels
bug 🐛 Oh no! A bug or unintented behaviour.

Comments

@dizlexik
Copy link

Describe the bug

I've created a reproduction of an issue where useQuery is returning data with __typename fields before page navigation and the same data without __typename fields after navigating. All cached data returned from useQuery after any navigation has occurred on the site is missing __typenames, which isn't a problem in itself since we would never rely on those values without explicitly including that field in the query, but what is having a negative effect is that the cached data coming out of useQuery does not match the previous data by reference. And this is true on every render so when any other component on a page fetches data that touches the cache it results in rerendering everything everywhere. This is bad for performance and also is breaking part of our application where we are feeding an array of objects out of useQuery into a table component which replaces rows when object references change, resulting in any row state (selected checkboxes in our case) being wiped out and replaced with default state.

Our workaround at the moment is to always include __typename in all of our queries. This makes Graphcache behave predictably across page navigation and cached data is always returned with the same references as previous results so things are working as expected/desired. Of course we would rather not have to include this unnecessary field in all of our queries, and there's now a chance that team members may forget to do this and introduce subtle bugs and performance issues. Ideally the results from useQuery would either always include __typename or never include it, and in either case references should always match previous results.

In troubleshooting this we came across a number of issues/PRs that were somewhat related to our issue here, but not exactly, and I couldn't find anything calling out page navigation in Next causing this strange change in how Graphcache behaves. Here are some links here for reference: #1268 #1366 #2701 #2718

Reproduction

https://codesandbox.io/p/sandbox/condescending-resonance-pndq8r

Urql version

urql v4.0.0
@urql/exchange-graphcache v6.0.1
next-urql v5.0.0

Validations

  • I can confirm that this is a bug report, and not a feature request, RFC, question, or discussion, for which GitHub Discussions should be used
  • Read the docs.
  • Follow our Code of Conduct
@dizlexik dizlexik changed the title Navigation bug with next-urql + exchange-graphcache Cached data bug after page navigation with next-urql + exchange-graphcache Apr 18, 2023
@kitten kitten added the bug 🐛 Oh no! A bug or unintented behaviour. label Apr 18, 2023
@kitten
Copy link
Member

kitten commented Apr 18, 2023

I think this is likely related to the regression that made this change to this section of code necessary:

const originalOperation = operations.get(result.operation.key);
const operation = originalOperation
? makeOperation(
originalOperation.kind,
originalOperation,
result.operation.context
)
: result.operation;
if (operation.kind === 'mutation') {
if (result.operation.context.originalVariables) {
operation.variables = result.operation.context.originalVariables;
delete result.operation.context.originalVariables;
}

So, this may take a bit of time to untangle, but should be trivial for us to fix. Basically, I'm currently assuming that the issue here is that the original query document isn't being restored correctly

@dizlexik
Copy link
Author

Hey @kitten, thanks for resolving this so quickly! ❤️

After upgrading to the latest release I'm now getting stable references for cached data. However, I'm experiencing a new bug and I'm not sure if it has anything to do with these changes or not. I'm using requestPolicy: 'cache-and-network' on my query and it is no longer fetching from the network when a cache hit is returned so data is becoming stale.

I updated the previous reproduction of the original bug to include requestPolicy: 'cache-and-network' and you can see in devtools that a network request is sent each time you navigate to the index page:
https://codesandbox.io/p/sandbox/next-urql-navigation-bug-pndq8r

I forked the sandbox and updated only the package.json with the new versions of urql/core and graphcache, and you can see in devtools that the index data is only ever fetched once:
https://codesandbox.io/p/sandbox/cache-and-network-bug-jiuz0w

@dizlexik
Copy link
Author

I see that the cache-and-network thing was resolved in @urql/core v4.0.6, and it's working again for me now 👍

@kitten
Copy link
Member

kitten commented Apr 20, 2023

Awesome! 🙌 Glad it's all working now. Refactoring some of Graphcache and Core to fix some regressions from the recent major releases did take two attempts in this case, but I hope it's worth it for you in the end ✌️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 Oh no! A bug or unintented behaviour.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants