[alpha][fix-5538] : sync Observer 'current' properties when optimistic reading occurs

[incepter]

This is a follow-up to #5573 for the v5-alpha version

[vercel]

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

1 Ignored Deployment

Name	Status	Preview	Comments	Updated (UTC)
query	⬜️ Ignored (Inspect)			Jul 3, 2023 11:00am

[codesandbox-ci]

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 a830410:

Sandbox	Source
@tanstack/query-example-react-basic-typescript	Configuration
@tanstack/query-example-solid-basic-typescript	Configuration
@tanstack/query-example-svelte-basic	Configuration
@tanstack/query-example-vue-basic	Configuration

[incepter]

When I run tests locally, I get a weird behavior of failures, but this one seems to be consistent: https://cloud.nx.app/runs/W8l5UK3y5A

• The test in vueQueryPlugin.test.ts seems to regress, not sure why .. I tried seeing the observer part and it seems to work okay.
• useQuery tests seem weird, some tests when ran independently pass, and do not pass under pnpm test locally.

/cc @TkDodo do you know why tests did not get executed for this PR ? I think we should give it extra care.

Also, when running tests locally from my IDE, I needed to rebuild the query-core manually to take changes.

[nx-cloud]

☁️ Nx Cloud Report

CI is running/has finished running commands for commit a830410. 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 1 target

• nx affected --targets=test:eslint,test:lib,test:types,test:build

---

Sent with 💌 from NxCloud.

[TkDodo]

do you know why tests did not get executed for this PR ? I think we should give it extra care.

because GitHub sees you as a first time contributor and thus doesn't run the pipeline until someone approves :/ Have a look at the nx cloud report now: #5611 (comment)

Also, when running tests locally from my IDE, I needed to rebuild the query-core manually to take changes.

yeah this is a recent change by @lachlancollins in the alpha branch, I also stumbled upon it.

You'd need to use the pnpm run watch command in the root to automatically pick up changes.

[TkDodo]

@incepter ignore the vue-query tests, seems to fail on alpha everywhere. but the react-query test is probably an issue:

• https://cloud.nx.app/runs/cpF8yZtCAu

[incepter]

Hey @TkDodo , sorry for the late reply.

You are right, in v5, placeHolderData acts like keepPreviousData too and has been enhanced.

The fix I found is to completely remove the need of testing on placeholder data, just assign the current properties whenever we have a new result.

The Tests fail in my windows machine which makes me always question whether the failure is real or not. Im not sure if we can improve this behavior

[TkDodo]

thanks. The missing errors are just flakiness I guess. I'll likely merge both solutions (main + alpha) today

[TkDodo]

@incepter the vue test fails locally for me as well on this PR.

@DamianOsipiuk could you maybe have a look at this here?

[TkDodo]

the test in question works fine locally on the alpha branch, so maybe we are regressing here after all

[incepter]

the test in question works fine locally on the alpha branch, so maybe we are regressing here after all

Yeah it is surely a regression, since leaving the properties alone and not assigning anything in the observer would make the test pass.

I fact, this test was about to drive me crazy today since I do not know vue. But managed to finally find the culprit:

When we assign the current properties of the observer, say via combineResult, then calling updateResult won't actually result in notifying the component, because updateResult would call again createResult and get an object that shallow equal the previous result, then it won't get notified because the updateResult function just quits when objects are shallowly equal.

I think I've hit the end of the road this way. Any solution would I think of using the globals to make and would not be predictable and easy.

Do you guys think of a solution ? I think the v5 would follow new mechanics, I will be thinking of a new solution that would work and not break anything.

[incepter]

Adding this seems to fix the failing vue test. Im not sure of the used approach but it seems that it s broken in that function and we could solve it this way.

@TkDodo can I somehow run the CI before the approval ? It kind of annoys me not to be able to have the same behavior locally with the tests.

[TkDodo]

can I somehow run the CI before the approval ? It kind of annoys me not to be able to have the same behavior locally with the tests.

sorry, it's a github thing for first time contributors :/ I also think its annoying

[codecov-commenter]

Codecov Report

Patch coverage has no change and project coverage change: +0.60 🎉

Comparison is base (f57cebe) 89.03% compared to head (a830410) 89.64%.

❗ 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             @@
##            alpha    #5611      +/-   ##
==========================================
+ Coverage   89.03%   89.64%   +0.60%     
==========================================
  Files          72       63       -9     
  Lines        2390     2134     -256     
  Branches      622      530      -92     
==========================================
- Hits         2128     1913     -215     
+ Misses        216      189      -27     
+ Partials       46       32      -14     

Impacted Files	Coverage Δ
packages/query-core/src/queryObserver.ts	83.50% <0.00%> (-2.67%)	⬇️

... and 10 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.

[DamianOsipiuk]

Seems to be working fine