fix(core): protect useLogin default success handler from custom onSuccess overrides

[mohammadxali]

PR Checklist

Please check if your PR fulfills the following requirements:

• The commit message follows our guidelines: https://refine.dev/docs/guides-concepts/contributing/#commit-convention

Bugs / Features

• Related issue(s) linked
• Tests for the changes have been added
• Docs have been added / updated
• Changesets have been added https://refine.dev/docs/guides-concepts/contributing/#creating-a-changeset

What is the current behavior?

What is the new behavior?

fixes #5888

Notes for reviewers

[changeset-bot]

🦋 Changeset detected

Latest commit: 01ed6a2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@refinedev/core	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[aliemir]

Hey @mohammadxali, thank you for the PR!

As you can see from the line here

refine/packages/core/src/hooks/auth/useLogin/index.ts

Line 35 in e5cad51

"mutationFn" | "onError" | "onSuccess"

We're omitting onSuccess handler from the mutationOptions on purpose. Your changes doesn't update that line and it will still cause type errors if passed properly 🤔

On all other Refine hooks, we've omitted onSuccess, onSettled, onError methods if we're using them in our implementation. If we're going to make them work without breaking the current implementation, we need to apply changes like this PR for all other hooks as well.

I'll also share this comment in the issue, let us know if you're willing to work on the further changes 🙏

[mohammadxali]

Hey 👋 Thanks for looking into this.

We're omitting onSuccess handler from the mutationOptions on purpose. Your changes doesn't update that line and it will still cause type errors if passed properly 🤔

On all other Refine hooks, we've omitted onSuccess, onSettled, onError methods if we're using them in our implementation. If we're going to make them work without breaking the current implementation, we need to apply changes like this PR for all other hooks as well.

That makes sense, however seems like even though the onSuccess handler is omitted from the types, but I can still use it:

Code_1naxEN4HAW.mp4

There's no errors and even going to definition takes me to react-query typings. It's super weird though :) I have no idea why's that the case.

---

At the same time, I believe allowing a custom onSuccess handler will significantly improve the consumer code. This way, we won't need to use useEffect calls to implement custom logic after a successful login or other operations.

let us know if you're willing to work on the further changes 🙏

I don't mind working on it :) But I'm not familiar with the overall codebase so I might be asking some questions around to find my way. I agree with your suggestion on adding it to all of refine hooks implementations. As a react-query user, I reach for onSuccess, onError and etc a lot during day-to-day work, so having that is definitely a good shift on a better DX.

[aliemir]

We'd love to help if you face any issues while working on this. If you want to continue working on this branch, let's convert it to draft. I can try to review your work while it's in progress and we can discuss here about any decisions to make or help if you face any issues 🚀

I'll also try to look what went wrong with the omit in useLogin 😅

[mohammadxali]

Awesome! 👍

Some questions though:

• Which hooks are needed to be updated to have onSuccess handler? (Ideally all of them, but asking to ensure before proceeding)
• Should I create a separate PR for each hook or should I do all of them in a single PR?
• Judging by our recent conversations this seems to be a feature now, should I rebase my branch to mention feat(core):?

[aliemir]

• Which hooks are needed to be updated to have onSuccess handler? (Ideally all of them, but asking to ensure before proceeding)

Actually, you can check for hooks which omits these callbacks in types. If you have any issues locating them or cannot decide, I can try coming up with a list but cannot promise today or this week 😅

• Should I create a separate PR for each hook or should I do all of them in a single PR?

Single PR will be enough I think. If you think you've made some additional changes that needs to be addressed in a separate PR we can always discuss it here.

• Judging by our recent conversations this seems to be a feature now, should I rebase my branch to mention feat(core):?

Yeah, it will be great actually 😅

[BatuhanW]

Hey @mohammadxali will you be able to continue this PR?

[mohammadxali]

Hey @mohammadxali will you be able to continue this PR?

Hey!

Sorry for the delay, and thanks for the reminder ping. I'll try to get to it in the weekend.

Estimations to be Saturday (15 June) or Sunday (16 June).