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

fix(query-core): fix context type error in onSuccess #6355

Merged
merged 4 commits into from
Dec 17, 2023

Conversation

Mini-ghost
Copy link
Contributor

@Mini-ghost Mini-ghost commented Nov 11, 2023

Description:

When defining types in our code, it seems that the type on onSuccess for mutate is different from the one on useMutation:

interface Context {
  // define
}

const { mutate } = useMutation<Data, Variables, Context>({
  mutationFn: () => fetch('url') // POST API
})

mutate(data, {
  onSuccess(data, variables, context) {
    // context is `undefined` but type is `Context`
  }
})

Perhaps there should be consistent type definitions here:

onSuccess?: (data: TData, variables: TVariables, context: TContext) => void

I've also made adjustments to the documentation.


Copy link

vercel bot commented Nov 11, 2023

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 Dec 17, 2023 2:17pm

Copy link

nx-cloud bot commented Nov 11, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 39d092d. 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


🟥 Failed Commands
nx affected --targets=test:eslint,test:lib,test:types,test:build,test:bundle

Sent with 💌 from NxCloud.

Copy link

codesandbox-ci bot commented Nov 11, 2023

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

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

@TkDodo
Copy link
Collaborator

TkDodo commented Nov 11, 2023

hm, I think the types are the way they are on purpose. Context is usually inferred from what onMutate returns. The only reason why it's TContext | undefined in onError and onSettled is that the onMutate function itself could throw, in which case there is no context. But since onSuccess is only called if the request (and thus also onMutate) was successful, there is no way that context is undefined when onSuccess is reached.

That is, of course, if you have onMutate implemented. If you don't, then TContext itself is undefined, and it doesn't matter if we join it with undefined or not.

so, by setting generics manually with:

useMutation<Data, Variables, Context>

you're basically setting Context to a specific type, even though you don't have onMutate implemented, which seems wrong.

@Mini-ghost
Copy link
Contributor Author

My apologies for missing some details. If you set it up this way on useMutation, the result might be different:

const { mutate } = useMutation<Data, Variables, Context>({
  mutationFn: () => fetch('url'), // POST API
   onSuccess(data, variables, context) {
     // Here, the `context` type is `Context | undefined`
   }
})

mutate(data, {
  onSuccess(data, variables, context) {
    // 'context' is `undefined`, but the type is `Context`
  }
}

Shouldn't both onSuccess instances have the same type hint for context?

@TkDodo
Copy link
Collaborator

TkDodo commented Nov 11, 2023

you're right, but I think onSuccess of useMutation should have context typed as Context instead of Context | undefined for the reasons I outlined above.

@Mini-ghost
Copy link
Contributor Author

Got it! I've learned a lot from your explanation. I'll adjust the type of onSuccess for useMutation to be Context.

Thanks a bunch for your patient clarification!

@Mini-ghost
Copy link
Contributor Author

@TkDodo Pardon me! Is there anything else that needs to be adjusted in this PR?

Copy link

nx-cloud bot commented Dec 17, 2023

☁️ Nx Cloud Report

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

@codecov-commenter
Copy link

Codecov Report

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

Comparison is base (c38c1cb) 0.00% compared to head (d66b7a8) 88.34%.

Files Patch % Lines
packages/query-core/src/mutationObserver.ts 0.00% 5 Missing ⚠️

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

Additional details and impacted files
@@            Coverage Diff            @@
##           main    #6355       +/-   ##
=========================================
+ Coverage      0   88.34%   +88.34%     
=========================================
  Files         0       75       +75     
  Lines         0     2592     +2592     
  Branches      0      684      +684     
=========================================
+ Hits          0     2290     +2290     
- Misses        0      257      +257     
- Partials      0       45       +45     

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

@TkDodo TkDodo merged commit 320cf9b into TanStack:main Dec 17, 2023
10 checks passed
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