-
-
Notifications
You must be signed in to change notification settings - Fork 3.5k
fix: don't update dataUpdateCount when setting initial data #9743
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 478f5d0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
WalkthroughFix query success handling so observer-provided Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor UI as Component
participant Obs as QueryObserver
participant QC as QueryClient
participant Q as Query
UI->>Obs: Mount with initialData
Obs->>QC: Subscribe to query key
QC->>Q: Create/attach observer
Note right of Q #d6f5d6: Apply initialData as manual\n(no dataUpdateCount increment,\nisFetchedAfterMount=false)
UI->>Obs: Trigger refetch
Obs->>Q: fetch()
Q->>Q: Resolve to fetched data
Note right of Q #f5e6d6: Update data, increment dataUpdateCount\nisFetchedAfterMount=true
Q-->>Obs: Notify result
Obs-->>UI: Render with fetched data
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Command | Status | Duration | Result |
---|---|---|---|
nx affected --targets=test:sherif,test:knip,tes... |
❌ Failed | 4m 49s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1m 18s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-10-12 15:57:37
UTC
packages/query-core/src/query.ts
Outdated
dataUpdateCount: action.manual && state.data === undefined | ||
? state.dataUpdateCount // Don't increment for initial data | ||
: state.dataUpdateCount + 1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m surprised that this doesn’t break any existing tests. For example, we would still want the count to increase if you call queryClient.setQueryData()
manually while e.g. a prefetch is in-flight. The exception should likely only be for initialData
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it did fail on CI, not sure why locally it didn't run those tests. I'll mess with it today as work allows (although this is blocking us from another fix so I'd like to tackle it asap)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the fix should not change code here, but instead dispatch setState
directly with the state we want for initialData rather than calling setData
🎯 Changes
This introduces a check when updating
dataUpdateCount
so that it isn't updated when setting initial data. This in turn fixes theisFetchedAfterMount
bug discussed in #9656fixes #9656
✅ Checklist
pnpm test:pr
🚀 Release Impact
Summary by CodeRabbit
Bug Fixes
Tests
Chores