-
Notifications
You must be signed in to change notification settings - Fork 68
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: add args to props of provider #748
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would update some of our documentation based on your changes. You can run |
We need this to handle callback URL, mean user want to go a route after we redirected from identitie server, I want to redirect user to her wanted root. Could you merge this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening this pull request!
Not sure you need this to do what you want.
You could use the hooks (e.g. onSignIn
to redirect the user).
I would love to see a simple test for this before landing 👍
Thank you!
@@ -77,6 +77,7 @@ export const AuthProvider: FC<AuthProviderProps> = ({ | |||
onSignIn, | |||
onSignOut, | |||
location = window.location, | |||
args, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably be more descriptive with the naming of this argument. args
seem to generic, would you agree?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
callbackState may be more appropriate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you could get it with a callback instead of state like getSigninState
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it not be better to add return value for onBeforeSignIn so you can create the args object for each signin? This way, you could e.g. create different state during login.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The goal is to give the parameter to signinRedirect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, I have the same issue. But if row 120, the call to onBeforeSignIn (which is a method that can be implemented) could return a args object, it can be passed in the next row to signinRedirect.
Something like:
let args = {};
if(onBeforeSignIn) {
args = onBeforeSignIn();
}
userManager.signinRedirect(args);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The onBeforeSignIn method is only called here as far as I can see so it seems like it is only used fo the autoSignIn flow. Hence, I think it might make sense to implement it with a possible return value since it will always be called before the signinRedirect and only if the autoSignIn is true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hosseinmd Any progress with this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The onBeforeSignIn method is only called here as far as I can see so it seems like it is only used fo the autoSignIn flow. Hence, I think it might make sense to implement it with a possible return value since it will always be called before the signinRedirect and only if the autoSignIn is true.
Do you think other users will understand this easily?
Thank you and other contributors about this good module.
By this PR we are able to add args to signinRedirect when autoSignin is true