-
Notifications
You must be signed in to change notification settings - Fork 69
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
refactor!: replace oidc-client-js with oidc-client-ts #860
Conversation
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.
@@ -142,7 +142,7 @@ export const AuthProvider: FC<PropsWithChildren<AuthProviderProps>> = ({ | |||
return ( | |||
<AuthContext.Provider | |||
value={{ | |||
signIn: async (args: unknown): Promise<void> => { | |||
signIn: async (args: any): Promise<void> => { |
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.
Are we sure we want to use any
here? If the args
are not know, they should probably stay unknown
. It would however be nicer to have some types here, is that something oidc-client-ts
now provides?
authority: authority || '', | ||
client_id: clientId || '', | ||
client_secret: clientSecret, | ||
redirect_uri: redirectUri, | ||
redirect_uri: redirectUri || '', |
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.
Why are these being defaulted to an empty string? It's been a while since I worked with this code base, so I'm a bit rusty 😅
@@ -87,7 +87,7 @@ export interface AuthProviderProps { | |||
* | |||
* defaults to 'location=no,toolbar=no,width=500,height=500,left=100,top=100' | |||
*/ | |||
popupWindowFeatures?: string; | |||
popupWindowFeatures?: PopupWindowFeatures; |
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.
Is this a union now?
I just realized we already have a pending v2 release |
I think this would be a good migration to make for oidc-react. We moved to MSAL recently. It's maintained by Microsoft and is finally recently compatible with IdentityServer. Once I saw oidc-client dying off I started looking for alternatives. With that said, I probably won't be paying close attention to patches for oidc-react. Don't wait for me to approve anything - I'll be slow to do so. |
fixes #652