Skip to content

[Fix-5538]: Assign observer's current Result when optimistic read occurs - #5573

Merged
TkDodo merged 6 commits into
TanStack:mainfrom
incepter:fix-data-double-ref
Jul 14, 2023
Merged

[Fix-5538]: Assign observer's current Result when optimistic read occurs#5573
TkDodo merged 6 commits into
TanStack:mainfrom
incepter:fix-data-double-ref

Conversation

@incepter

@incepterincepter commented Jun 13, 2023

Copy link
Copy Markdown
Contributor

fixes#5538

@vercel

vercelBot commented Jun 13, 2023

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
NameStatusPreviewCommentsUpdated (UTC)
query⬜️ Ignored (Inspect)Jun 23, 2023 2:38pm

@incepter
incepterforce-pushed the fix-data-double-ref branch from f1187b9 to 125a52aCompareJune 13, 2023 14:32

@TkDodoTkDodo left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you so much 🙌

Comment threadpackages/query-core/src/queryObserver.ts
Comment threadpackages/react-query/src/__tests__/useQuery.test.tsx Outdated
@nx-cloud

nx-cloudBot commented Jun 13, 2023

Copy link
Copy Markdown

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 2ef9021. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 2 targets

Sent with 💌 from NxCloud.

@incepter
incepterforce-pushed the fix-data-double-ref branch from 0d68f6d to 3702e9bCompareJune 13, 2023 17:57
@codesandbox-ci

codesandbox-ciBot commented Jun 13, 2023

Copy link
Copy Markdown

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 2ef9021:

SandboxSource
@tanstack/query-example-react-basic-typescriptConfiguration
@tanstack/query-example-solid-basic-typescriptConfiguration
@tanstack/query-example-svelte-basicConfiguration
@tanstack/query-example-vue-basicConfiguration
wandering-sky-pn3h6lIssue #5538
dazzling-https-rrsdghIssue #5538

@incepter

Copy link
Copy Markdown
ContributorAuthor

Here a codesandbox forked from the issue using the version from this PR, I think it solves it

https://codesandbox.io/s/serene-keller-h34qgm?file=/src/App.js

@TkDodo

Copy link
Copy Markdown
Collaborator

I think we can ship this, but could you please also make the same PR to the alpha branch? I think this would be better than to merge main -> alpha and try to address the conflicts there

@codecov-commenter

codecov-commenter commented Jun 17, 2023

Copy link
Copy Markdown

Codecov Report

Patch coverage: 17.64% and project coverage change: -0.70⚠️

Comparison is base (df37d35) 92.19% compared to head (2ef9021) 91.50%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files
@@ Coverage Diff @@## main #5573 +/- ##
==========================================
- Coverage 92.19% 91.50% -0.70% 
==========================================
Files 112 101 -11 Lines 4294 3921 -373 Branches 1127 989 -138 ==========================================
- Hits 3959 3588 -371 + Misses 314 308 -6 - Partials 21 25 +4 
Impacted FilesCoverage Δ
packages/query-core/src/queryCache.ts100.00% <ø> (ø)
packages/query-core/src/queryObserver.ts81.28% <0.00%> (-3.22%)⬇️
packages/query-core/src/notifyManager.ts95.12% <100.00%> (+5.12%)⬆️

... and 11 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@incepter

Copy link
Copy Markdown
ContributorAuthor

I think we can ship this, but could you please also make the same PR to the alpha branch? I think this would be better than to merge main -> alpha and try to address the conflicts there

Yes sure, I ported the changes to alpha branch in this PR.

Comment threadpackages/query-core/src/queryObserver.ts Outdated
Comment threadpackages/query-core/src/queryObserver.ts
@TkDodo

Copy link
Copy Markdown
Collaborator

there is one test failing now @incepter:

useQuery › should reuse same data object reference when queryKey changes and placeholderData is present

@incepter

Copy link
Copy Markdown
ContributorAuthor

there is one test failing now @incepter:

Yes indeed @TkDodo , I fixed it. (I was using the same queryKey and same client for both tests, which resulted in them passing individually, then failing in ci.)

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Why useEffect triggers twice after the API response is cached?

3 participants

@incepter@TkDodo@codecov-commenter