SDKS-5067: Standardize SDK Configuration#684
Conversation
🦋 Changeset detectedLatest commit: 2a4fee2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 12 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 dd2a363
💡 Dealing with memory or CPU issues? See memory and CPU details with the resource usage add-on ↗. ☁️ Nx Cloud last updated this comment at |
|
Warning Review limit reached
More reviews will be available in 47 minutes and 37 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (30)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
@forgerock/davinci-client
@forgerock/device-client
@forgerock/journey-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: |
Codecov Report❌ Patch coverage is Please upload reports for the commit 5379e6f to get more accurate results. Additional details and impacted files@@ Coverage Diff @@
## main #684 +/- ##
===========================================
+ Coverage 18.07% 66.93% +48.85%
===========================================
Files 155 133 -22
Lines 24398 7176 -17222
Branches 1203 1308 +105
===========================================
+ Hits 4410 4803 +393
+ Misses 19988 2373 -17615
🚀 New features to boost your workflow:
|
|
Deployed 0272a8d to https://ForgeRock.github.io/ping-javascript-sdk/pr-684/0272a8d1b6bab91b1ddd0a8c0973d6ba2f1e3eb0 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔺 @forgerock/sdk-utilities - 14.7 KB (+3.5 KB, +31.6%) 🆕 New Packages🆕 @forgerock/device-client - 10.0 KB (new) 📊 Minor Changes📈 @forgerock/sdk-types - 8.0 KB (+0.1 KB) ➖ No Changes➖ @forgerock/protect - 144.6 KB 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
feat(sdk-utilities): add pure unified config validation feat(sdk-utilities): add unified config mapping functions feat(sdk-utilities): export config module from sdk-utilities test(sdk-utilities): add unit tests for config mapping and validation feat(sdk-types): add OIDC authorize params to GetAuthorizationUrlOptions feat(sdk-oidc): wire OIDC authorize params into buildAuthorizeParams feat(oidc-client): add signOutRedirectUri to endSession URL feat(oidc-client): accept unified JSON config in oidc factory feat(journey-client): accept unified JSON config in journey factory feat(davinci-client): accept unified JSON config in davinci factory chore: update api-reports for unified config param widening feat(sdk-utilities): align unified config schema with new journey sub-object refactor(sdk-utilities): mapper functions return ConfigMappingResult with single error fix(sdk-utilities): map log field and use config.log in factories fix(sdk-utilities): tighten isUnifiedSdkConfig discriminant to require object values fix(oidc-client): use throw instead of Promise.reject in unified config error paths refactor(oidc-client): migrate logout.request.test.ts to it + Micro.runPromise pattern docs(oidc-client): add JSDoc to OidcConfig and all new fields
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nx Cloud has identified a possible root cause for your failed CI:
This CI failure appears to be related to the environment or external dependencies rather than your code changes.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
| let config: DaVinciConfig; | ||
|
|
||
| if (isUnifiedSdkConfig(rawConfig)) { | ||
| const validation = validateUnifiedSdkConfig(rawConfig, true); | ||
| if (!validation.success) { | ||
| const messages = validation.errors.map((e) => `${e.field}: ${e.message}`).join(', '); | ||
| throw new Error(`Invalid unified SDK config: ${messages}`); | ||
| } | ||
| const mapped = unifiedToDavinciConfig(validation.data); | ||
| if (!mapped.success) { | ||
| throw new Error(`Invalid unified SDK config: ${mapped.error.field}: ${mapped.error.message}`); | ||
| } | ||
| config = mapped.data as DaVinciConfig; | ||
| } else { | ||
| config = rawConfig as DaVinciConfig; | ||
| } | ||
|
|
||
| const log = loggerFn({ | ||
| level: logger?.level ?? config.log ?? 'error', | ||
| custom: logger?.custom, | ||
| }); |
There was a problem hiding this comment.
i'm not a huge fan of this here. I think we should maybe write a separate validation function if we need to do some validation but i'm concerned about the type of Record<string,unknown> it will break the entire purpose of typing configs.
| @@ -4,7 +4,7 @@ | |||
| * This software may be modified and distributed under the terms | |||
| * of the MIT license. See the LICENSE file for details. | |||
| */ | |||
There was a problem hiding this comment.
Let's use @effect/vitest if we're testing effect returning functions
| * This software may be modified and distributed under the terms | ||
| * of the MIT license. See the LICENSE file for details. | ||
| */ | ||
| import { it, expect } from '@effect/vitest'; |
| return level.toLowerCase() as MappedLogLevel; | ||
| } | ||
|
|
||
| const REQUIRED_OIDC_FIELDS_STRICT: ReadonlyArray<keyof UnifiedOidcConfig> = [ |
There was a problem hiding this comment.
Why do we want an array here? we'd rather an array than a union?
|
|
||
| const REQUIRED_OIDC_FIELDS_JOURNEY: ReadonlyArray<keyof UnifiedOidcConfig> = ['discoveryEndpoint']; | ||
|
|
||
| function validateType( |
There was a problem hiding this comment.
per our discussion today, this could be a worthwhile case for Either<_,ConfigValidationError]> and accumulate errors in the error channel
ryanbas21
left a comment
ryanbas21
left a comment
There was a problem hiding this comment.
I think we really need to think about this. I'm concerned about how safe some of our changes here are, breaking changes, and the path we're taking here. I personally feel like we should discuss this more as a team because i'm not positive that this is the right path
3 participants
Summary
https://pingidentity.atlassian.net/browse/SDKS-5067
Implements a unified cross-platform JSON configuration schema for the JS
SDK. All three factories (
oidc,journey,davinci) now accept theunified JSON config, map it to internal shapes via a new
sdk-utilitiesconfig module, validate required fields, and throw/reject on invalid input.
Non-breaking: existing internal config shapes still accepted.
Changes
packages/sdk-utilitiessrc/lib/config/module:config.types.ts(unified typedefinitions),
config.utils.ts(pure validation + mapping functions),config/index.tsbarrel exportvalidateUnifiedSdkConfig— discriminated-union validation with typedfield errors; strict mode for oidc/davinci, lenient for journey
isUnifiedSdkConfig— discriminator:'oidc' in input || 'journey' in inputunifiedToOidcConfig,unifiedToJourneyConfig,unifiedToDavinciConfig— pure mapping functions returning
ConfigValidationResult<T>; scopes[] →scope string, refreshThreshold (sec) → oauthThreshold (ms), log → logLevel
sdk-utilities/src/index.tspackages/oidc-clientclient.store.ts— detects unified JSON, validates, maps, throws onerror; forwards new OIDC params (
loginHint,state,nonce,display,prompt,uiLocales,acrValues,query) intogetAuthorizationUrlcallconfig.types.ts—OidcConfigextended withsignOutRedirectUri,loginHint,state,nonce,display,prompt,uiLocales,acrValues,query,logoidc.api.ts— appendspost_logout_redirect_urito endSession URL whensignOutRedirectUripresentpackages/journey-clientclient.store.ts— detects unified JSON, validates, maps, throws on errorconfig.types.ts—JourneyClientConfigextended withlog?: LogLevelpackages/davinci-clientclient.store.ts— detects unified JSON, validates, maps, throws on errorconfig.types.ts—DaVinciConfigextended withlog?: LogLevelpackages/sdk-typesauthorize.types.ts—GetAuthorizationUrlOptionsextended withloginHint,nonce,display,uiLocales,acrValues;promptwidenedto include
'select_account'packages/sdk-effects/oidcauthorize.utils.ts—buildAuthorizeParamswiresloginHint,nonce,display,uiLocales,acrValuesinto the authorize URLTests
config.test.tsinsdk-utilities: valid mapping for all threefactories, each required-field-missing error, type-mismatch errors,
unknown-field-ignored,
refreshThreshold→oauthThresholdconversion,scopes join, log field mapping,
cookieNamenon-mappingclient.store.test.tsinoidc-client,journey-client,davinci-client: success path + two throw cases each (validation failure,mapping failure)
authorize.test.tsinsdk-effects/oidc: 6 tests covering each new OIDCparam individually and all together
authorize.request.micros.test.ts,authorize.request.utils.test.ts,logout.request.test.tsinoidc-clientfor new config shape
How to test
1. Unit tests