Fixed issue causing inactive queries to be executed when clearing or resetting the store #11925
Conversation
…resetting the store Closes apollographql#11914
👷 Deploy request for apollo-client-docs pending review.Visit the deploys page to approve it
|
🦋 Changeset detectedLatest commit: 187828a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| import { equal } from "@wry/equality"; | ||
| import type { DocumentNode, GraphQLError } from "graphql"; |
There was a problem hiding this comment.
It looks like your editor might have applied some kind of import reordering. Would you mind disabling that and reverting the changes to the imports? We'd love to keep the git blame as clean as possible 🙂
| @@ -1,53 +1,53 @@ | |||
| // externals | |||
| import { DocumentNode, GraphQLError } from "graphql"; | |||
There was a problem hiding this comment.
Same here. Would you mind reverting the change that reordered these imports?
| }); | ||
| // Recreate state where an observable query is dirty but has no observers in the query manager | ||
| // @ts-expect-error -- Accessing private field for testing | ||
| oq.queryInfo.dirty = true; |
There was a problem hiding this comment.
While this no doubt reproduces the issues, I'd still love to figure out how best the client gets into this state to begin with using user-facing APIs. User's aren't going to be touching QueryInfo or the dirty flag manually, so the more we can understand the situation that gets us here, the better. It will help us avoid a regression in the future knowing exactly the situation that got us here.
Since we are able to reproduce in CodeSandbox using useQuery with React's strict mode enabled, we should consider moving this test over to the useQuery tests. Maybe we start there to figure out exactly how this dirty flag gets set and work backwards?
Once we have a failing test for useQuery ("failing" means assuming the change you've added to QueryInfo is not applied), then we might figure out how best to backport the situation over to this test.
What do you think about that?
There was a problem hiding this comment.
My idea was to reproduce the bad state, regardless of what causes the bad state
I agree that it's better to do a fully functional test especially to identify the "real" cause of the issue, but I haven't had time to look into it this week because I was swamped and I'm going on vacations starting next week.
If you want to pick up the PR to wrap it up I don't mind.
Otherwise I'll try to look into it when I get a chance (it's still on my list 😅)
Closes #11914