chore(oidc-client): re-export necessary types #495
Conversation
|
|
Caution Review failedThe pull request is closed. WalkthroughRemoved unused e2e dependencies and refactored oidc-client to move many runtime imports to type-only imports, add/re-locate public type aliases (ClientStore/RootState/AppDispatch), update function signatures to use ClientStore, and update README examples to use awaited API calls including token.revoke(). Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant Store as ClientStore
participant OidcAPI as oidcApi
participant Browser
App->>Store: call authorizeµ(...)
Note right of Store: type is ClientStore (type-only change)
Store->>OidcAPI: build authorization request / get url
OidcAPI->>Browser: open redirect / iFrame (runtime iFrameManager)
Browser-->>App: redirect / response
App->>OidcAPI: handle response -> returns AuthorizationSuccess / AuthorizationError
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (20)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit b89ad58
☁️ Nx Cloud last updated this comment at |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (18.79%) is below the target coverage (40.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #495 +/- ##
==========================================
+ Coverage 18.52% 18.79% +0.27%
==========================================
Files 138 140 +2
Lines 27402 27640 +238
Branches 963 980 +17
==========================================
+ Hits 5076 5195 +119
- Misses 22326 22445 +119
🚀 New features to boost your workflow:
|
@forgerock/davinci-client
@forgerock/oidc-client
@forgerock/protect
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/oidc-client/src/lib/authorize.request.ts (1)
15-36: Consider makingcreateClientStorea type-only import and updating JSDoc
createClientStoreis only used in thestore: ReturnType<typeof createClientStore>annotation, so it could be imported withimport typeto avoid an unnecessary runtime dependency. Also, theauthorizeµJSDoc does not mention thestoreparameter, which can be updated for clarity.packages/oidc-client/src/lib/authorize.request.utils.ts (1)
11-60: Align JSDoc return types withAuthorizationErrorThe functions here now use
AuthorizationErrorin their TypeScript signatures (buildAuthorizeOptionsµ,createAuthorizeErrorµ,handleResponseµ), but the JSDoc still refers toAuthorizeErrorResponsein some@returnsannotations. It would be good to update the JSDoc to useAuthorizationError(or otherwise match the actual types) to avoid confusion for consumers and tooling.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (17)
e2e/oidc-app/package.json(1 hunks)e2e/oidc-app/tsconfig.app.json(0 hunks)e2e/oidc-app/tsconfig.json(0 hunks)packages/oidc-client/README.md(1 hunks)packages/oidc-client/package.json(1 hunks)packages/oidc-client/src/lib/authorize.request.ts(1 hunks)packages/oidc-client/src/lib/authorize.request.types.ts(1 hunks)packages/oidc-client/src/lib/authorize.request.utils.ts(1 hunks)packages/oidc-client/src/lib/client.store.ts(2 hunks)packages/oidc-client/src/lib/client.store.utils.ts(1 hunks)packages/oidc-client/src/lib/exchange.request.ts(1 hunks)packages/oidc-client/src/lib/exchange.types.ts(1 hunks)packages/oidc-client/src/lib/exchange.utils.test.ts(1 hunks)packages/oidc-client/src/lib/logout.request.test.ts(1 hunks)packages/oidc-client/src/lib/oidc.api.ts(1 hunks)packages/oidc-client/src/lib/wellknown.api.ts(1 hunks)packages/oidc-client/src/types.ts(1 hunks)
💤 Files with no reviewable changes (2)
- e2e/oidc-app/tsconfig.app.json
- e2e/oidc-app/tsconfig.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-23T20:50:26.537Z
Learnt from: ancheetah
Repo: ForgeRock/ping-javascript-sdk PR: 417
File: packages/sdk-effects/storage/src/lib/storage.effects.ts:11-12
Timestamp: 2025-09-23T20:50:26.537Z
Learning: When the storage client API was changed from returning void to GenericError | null in SDKS-4361, call sites in DaVinci client and OIDC client were not updated to handle the returned errors, creating a follow-up task for proper error handling implementation.
Applied to files:
packages/oidc-client/src/types.ts
🧬 Code graph analysis (1)
packages/oidc-client/src/lib/authorize.request.types.ts (1)
packages/oidc-client/src/types.ts (1)
GetAuthorizationUrlOptions(13-13)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Mend Code Security Check
- GitHub Check: pr
🔇 Additional comments (12)
packages/oidc-client/src/lib/exchange.utils.test.ts (1)
10-11: Type-only imports here are appropriate and safe
OidcConfigandGetAuthorizationUrlOptionsare used purely as annotations, so switching these toimport typeis correct and has no runtime effect on the tests.packages/oidc-client/src/lib/client.store.utils.ts (1)
8-31: Value import forloggerFnmatchesReturnType<typeof loggerFn>usageSwitching
loggerFnto a runtime import is necessary soReturnType<typeof loggerFn>is valid; this keeps theloggerextra argument correctly typed while only passing the instance through Redux middleware.packages/oidc-client/src/lib/logout.request.test.ts (1)
11-36: MSW v2-style handlers and type-only imports look consistentUsing
http/HttpResponsefor the mocked endpoints aligns with the modern msw API, and convertingOauthTokens/OidcConfigtoimport typeis correct since they’re used only in annotations/generics.packages/oidc-client/src/lib/client.store.ts (1)
7-23: Runtime/type import split for logger and storage is soundUsing runtime imports for
loggerFnandcreateStoragewhile movingCustomLogger,LogLevel, andStorageConfigtoimport typecleanly separates value vs. type usage; theas StorageConfigcast remains purely compile-time, so this doesn’t affect runtime behavior.packages/oidc-client/src/lib/exchange.types.ts (1)
7-29: Type-only import ofOidcConfigmatches its usage
OidcConfigis referenced only inside theTokenRequestOptionsinterface, so converting it toimport typeis correct and avoids adding any runtime dependency on./config.types.js.packages/oidc-client/src/lib/wellknown.api.ts (1)
10-21:WellKnownResponsecorrectly converted to a type-only importSince
WellKnownResponseis used solely as a generic type for the RTK Query endpoint, importing it withimport typeis a safe, purely compile-time change.e2e/oidc-app/package.json (1)
12-16: E2E app dependency reduction is reasonable—verify no stale importsHaving the e2e app depend only on
@forgerock/oidc-clientfits the re-export strategy; just confirm there are no remaining@forgerock/javascript-sdkor@forgerock/sdk-typesimports in the e2e source before merging.packages/oidc-client/package.json (1)
40-42: Public publish configuration looks correct—align with release workflowAdding
"publishConfig": { "access": "public" }is the right knob for exposing@forgerock/oidc-clienton npm; just ensure this matches your registry permissions and any changeset/release process you follow for version bumps.packages/oidc-client/src/lib/exchange.request.ts (1)
15-17: Type-only imports for exchange typings look goodUsing
StorageConfigfrom@forgerock/storageandTokenExchangeErrorResponseas type-only imports keeps runtime lean while aligning with the public type surface; no behavior change implied here.packages/oidc-client/src/lib/authorize.request.types.ts (1)
7-10: Centralizing authorize option types looks consistentDefining
BuildAuthorizationDataandOptionalAuthorizeOptionshere, based onGetAuthorizationUrlOptions, matches their usage in the utils and cleanly exposes them as part of the public type surface.packages/oidc-client/src/types.ts (1)
11-18: Public type re-exports align with the package’s API goalsRe-exporting
GenericError,GetAuthorizationUrlOptions,WellKnownResponse, request-middleware, logger, and storage types as type-only exports from this barrel matches the PR’s goal of exposing all required external types viaoidc-clientwithout adding runtime weight.packages/oidc-client/src/lib/oidc.api.ts (1)
4-19: The logger is consistently wired in all relevant store configurations; no action needed.Verification confirms that all
createClientStorecalls found in the codebase pass a logger instance. SinceoidcApiis only used with stores created throughcreateClientStore, and the parameter is consistently provided at all call sites, the runtime concern does not apply. The code pattern is safe and correct.
|
Deployed e34580a to https://ForgeRock.github.io/ping-javascript-sdk/pr-495/e34580a78513f883adc4f07e1fa9cd9850ec3842 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/journey-client - 0.0 KB (-82.4 KB, -100.0%) 📊 Minor Changes📈 @forgerock/oidc-client - 23.4 KB (+0.3 KB) ➖ No Changes➖ @forgerock/device-client - 9.2 KB 13 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
packages/oidc-client/package.json
Outdated
| "publishConfig": { | ||
| "access": "public" | ||
| }, |
There was a problem hiding this comment.
This seems unnecessary, it doesnt hurt anything but publishConfig takes precedence when defined so we dont really need it if we want cli control.
ancheetah
force-pushed
the
oidc-release-4.8.3
branch
3 times, most recently
from
03b4e44 to
b51ea11
Compare
ancheetah
force-pushed
the
oidc-release-4.8.3
branch
from
b51ea11 to
b89ad58
Compare
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4532
Description
Re-exports needed types from packages external to oidc-client. Updates API description in README. Adds
access: publicto package.jsonNo changeset.
Summary by CodeRabbit
New Features
Documentation