-
Notifications
You must be signed in to change notification settings - Fork 3
feat(oidc-client): improving types and completing userinfo #363
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
Conversation
🦋 Changeset detectedLatest commit: 6c06e70 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 packages
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 |
View your CI Pipeline Execution ↗ for commit 6c06e70
☁️ Nx Cloud last updated this comment at |
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (33.33%) is below the target coverage (40.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #363 +/- ##
==========================================
+ Coverage 52.26% 60.42% +8.15%
==========================================
Files 20 32 +12
Lines 1588 2044 +456
Branches 188 290 +102
==========================================
+ Hits 830 1235 +405
- Misses 758 809 +51
🚀 New features to boost your workflow:
|
Deployed 943b676 to https://ForgeRock.github.io/ping-javascript-sdk/pr-363/943b676a92368e29712bfb59020adf8ee6b86cad branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔺 @forgerock/oidc-client - 17.9 KB (+4.3 KB, +31.3%) 📊 Minor Changes📉 @forgerock/davinci-client - 34.1 KB (-0.2 KB) ➖ No Changes➖ @pingidentity/protect - 108.4 KB 11 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
config: OidcConfig, | ||
options: GetAuthorizationUrlOptions, | ||
) { | ||
): Micro.Micro<never, AuthorizeErrorResponse, never> { |
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 may have spoken about this but why do we only return an error here? it feels odd?
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.
I believe we spoke about this very piece of code in our meeting, but let me know if you have any follow up questions.
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.
Looks good. Left a few comments just for typos.
* @param code - The authorization code received from the authorization server. | ||
* @param state - The state parameter from the authorization URL creation. | ||
* @param options - Optional storage configuration for persisting tokens. |
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.
params are missing documented types
* @param code - The authorization code received from the authorization server. | ||
* @param state - The state parameter from the authorization URL creation. | ||
* @param options - Optional storage configuration for persisting tokens. | ||
* @returns {Promise<TokenExchangeErrorResponse | OauthTokens>} |
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.
Looks like the return type here should be updated to Promise<TokenExchangeResponse | GenericError | TokenExchangeErrorResponse>
/** | ||
* @function info - Retrieves user information using the userinfo endpoint from the wellknown configuration. | ||
* It requires an access token stored in the configured storage. | ||
* @returns {Promise<AuthorizeErrorResponse | any>} - Returns a promise that resolves to user information or an error response. |
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.
return type should be Promise<GenericError | TokenExchangeResponse>
* @param {WellKnownResponse} wellknown - The well-known configuration for the OIDC server. | ||
* @param {OidcConfig} config - The OIDC client configuration. | ||
* @param {OptionalAuthorizeOptions} options - Optional parameters for the authorization request. | ||
* @description Builds the authorization options for the OIDC client. |
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.
duplicate. already a description above
config: OidcConfig, | ||
options: GetAuthorizationUrlOptions, | ||
) { | ||
): Micro.Micro<[string, OidcConfig, GetAuthorizationUrlOptions], AuthorizeErrorResponse, never> { |
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.
Nothing wrong here. Just wanted to mention that I really like when you include the return types for effects on functions like you did here. It helps me better understand the return type without having to hover over for intellisense.
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.
just two small comments
), | ||
).pipe( | ||
Micro.map(({ data, error }) => { | ||
if (error) { |
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.
Should we not fail
here? I know RTK doesn't error, but i'm curious if there's any benefit to us manually handling it this way instead?
The benefit I see is more explicit handling of error paths rather than inter-mixing it as much in the success.
Small benefit perhaps. Nothing major.
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.
Yeah, I can make this change.
if ('status' in error) { | ||
status = error.status; | ||
} | ||
return { |
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.
I am curious if we added readonly _tag
property if we would get the benefit of Error discrimination without using the Micro.TaggedError
class
JIRA Ticket
SDKS-4251
Description
Complete userinfo endpoint work and improve error typing across SDK.