Skip to content

Conversation

@Aiglobelam
Copy link
Contributor

Fix #48

As I never seen or worked with code generation before, I took help from ChatGPT Claude 3.5 Sonnnet (Preview) to do this PR. And the suggestions seems legit to me, but... yeah... I think you need to check them 😄
BeforeAfter

contextNotOptional

What?

Makes the context parameter required in onSuccess callback to match React Query's types.
This ensures type safety by properly matching React Query's API where context is:

  • Required in onSuccess
  • Optional in onError and onSettled

Note that ChatGPT suggested to revert the order of the changelog.md file... so I went with that, can undo that if you want.

  • I have added or updated unit tests for this change.
  • I have updated the changelog with info about the changes in this PR

react-query rapini-mutations useRapiniMutations body mutationOptions onSuccess param context remove optional
Update test for useRapiniMutation to be ok with onSuccess TContext not beeing optional any more
Added a fix issue under header Unreleased

### Fixed
- rametta#48: Make context parameter required in onSuccess callback to match React Query's types
Make issue 48 a clickable link to issue
undefined,
ts.factory.createIdentifier("context"),
ts.factory.createToken(ts.SyntaxKind.QuestionToken),
undefined,

Choose a reason for hiding this comment

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

lgtm :-)

@rametta
Copy link
Owner

rametta commented Feb 24, 2025

Nice, thank you for the PR!
2 comments

  • Please update the changelog file to just include the new change and the next version number, there seems to be a lot of changes to that file but we just need the new changes.
  • Does this change force every use of onSuccess to pass in a context as the third argument? Or it fully hidden from the consumers? Been awhile since I used this so I can't remember if removing the optionality of the last argument will force everyone to start passing in a context whenever they want to use onSuccess. let me know!

Thanks!

Revert order back to initial and add fix for issue rametta#48 in bottom as release 3.5.3
@Aiglobelam
Copy link
Contributor Author

I first located where in react-query release log this change was made

TAG: https://github.com/TanStack/query/releases/tag/v5.14.1
PR: TanStack/query#6355

Then I also consulted the AI once more:

rapiniResp1

rapiniResp2

Conclusion

This change matches React Query's own type changes from this commit.

The commit removed optionality from context in onSuccess because it's only called when a mutation succeeds, and in that case React Query always provides the context that was passed to mutate.

Looking at our test file, we're already expecting this behavior:

onSuccess: (data: TData, variables: TVariables, context: TContext) => {
    conf?.onSuccess?.(data, variables, context);
    onSuccess?.(data, variables, context);
}

This change:

  1. Aligns our types with React Query's
  2. Reflects the actual runtime behavior (context is always available in onSuccess)
  3. Maintains optional context for onError/onSettled where it might be undefined
  4. Doesn't change the API surface, just makes types more accurate

Users won't need to provide context explicitly - React Query handles that internally. The type change just ensures we correctly model what React Query guarantees.

@jens-nuanxed
Copy link

@rametta What do you think? Is there something more I should look into?

@rametta rametta merged commit 0f20510 into rametta:main Mar 17, 2025
3 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.

Generated code incompatible from 5.14.6 to 5.17.9 of @tanstack/react-query

4 participants