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

[WIP] New useQueryState Hook #6758

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

[WIP] New useQueryState Hook #6758

wants to merge 9 commits into from

Conversation

crutchcorn
Copy link
Member

@crutchcorn crutchcorn commented Jan 23, 2024

Closes #6756

TODO:

  • Fix any broken tests
  • Migrate useQueryState logic to useMutationState
  • Add working tests for useQueryState
  • Mark useIsFetching as deprecated
  • Mark useIsMutating as deprecated
  • Update docs to note deprecated fields
  • Add reference docs

Copy link

vercel bot commented Jan 23, 2024

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

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
query ⬜️ Ignored (Inspect) Visit Preview Feb 3, 2024 11:23am

Copy link

nx-cloud bot commented Jan 23, 2024

☁️ Nx Cloud Report

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

📂 See all runs for this CI Pipeline Execution


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

Copy link

codesandbox-ci bot commented Jan 23, 2024

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 05e18d3:

Sandbox Source
@tanstack/query-example-angular-basic Configuration
@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

@crutchcorn
Copy link
Member Author

There appear to be two failing tests on this PR and I can't seem to figure out why they're failing. Namely, they are the useFetching API that's been changed.

While debugging, I've noticed the following:

  • notifyManager.schedule(onStoreChange) in useQueryState is called properly with the single item
  • onStoreChange is not ever called

I can't seem to figure out why this is the case or why it works in the other tests, but not these.

It also appears that there's some timing issues with sleep(10) that I've resolved by replacing with new Promise changes.

the value of the ref can get out of sync if we only write the ref in the subscribe function
@codecov-commenter
Copy link

codecov-commenter commented Jan 24, 2024

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (c190f68) 41.78% compared to head (05e18d3) 85.09%.

Files Patch % Lines
packages/react-query/src/useQueryState.ts 90.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #6758       +/-   ##
===========================================
+ Coverage   41.78%   85.09%   +43.30%     
===========================================
  Files         178       24      -154     
  Lines        7017      322     -6695     
  Branches     1421       83     -1338     
===========================================
- Hits         2932      274     -2658     
+ Misses       3722       39     -3683     
+ Partials      363        9      -354     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Comment on lines +75 to +77
// Memo to avoid other `useMutation` hook causing a re-render
const IsMutating = React.memo(IsMutatingBase)

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's also fine to change the assertion here to 0,1,1,0 instead.

Sign up for free to 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.

None yet

3 participants