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
ancheetah
left a comment
ancheetah
left a comment
There was a problem hiding this comment.
Couple comments after a quick initial scan
| logger, | ||
| }: { | ||
| config: DaVinciConfig; | ||
| config: DaVinciConfig | Record<string, unknown>; |
There was a problem hiding this comment.
In Andy's design doc, he suggests initializing the client through a new property called json which contains the json config. Is there a reason why you deviated from this? I think it may be a good idea to distinguish between the standard config and json config. The typing could also be a little more strict than Record<string, unknown>. If we have agreed on a standard JSON config as a team then we should be able to create an interface for it. This also removes the need for a isUnifiedSdkConfig utility.
| const messages = validation.errors.map((e) => `${e.field}: ${e.message}`).join(', '); | ||
| throw new Error(`Invalid unified SDK config: ${messages}`); | ||
| } | ||
| const mapped = unifiedToDavinciConfig(validation.data); |
There was a problem hiding this comment.
I'm not sure if "unified" is referring the "Unified SDK" here or the "Unified JSON Config". Can we rename it jsonToDavinciConfig or something? I believe we are trying to move away from calling it the "Unified SDK" and this may even change in the future.
vatsalparikh
left a comment
vatsalparikh
left a comment
There was a problem hiding this comment.
First review, mostly around type definition, usage, and duplications.
Side note: I do want to third (after AJ and Ryan) that we should think about narrowing the acceptable types for config instead of Record<string, unknown>
|
|
||
| export type OidcDisplayValue = 'page' | 'popup' | 'touch' | 'wap'; | ||
|
|
||
| export type OidcPromptValue = 'none' | 'login' | 'consent' | 'select_account'; |
There was a problem hiding this comment.
Why do we have this type defined when we have an identical type in PromptValue under packages/oidc-client/src/lib/authorize.request.utils.ts file
| * of the MIT license. See the LICENSE file for details. | ||
| */ | ||
|
|
||
| export type LogLevelString = 'DEBUG' | 'INFO' | 'WARN' | 'ERROR' | 'NONE'; |
There was a problem hiding this comment.
We already have a LogLevel type. Is there a reason for this duplicated LogLevelString? Maybe we can do something like Uppercase to stay consistent with the unified schema.
| // (undocumented) | ||
| clientId: string; | ||
| // (undocumented) | ||
| display?: 'page' | 'popup' | 'touch' | 'wap'; |
There was a problem hiding this comment.
Why are we inling when we already have OidcDisplayValue type defined. This is common across the PR. Even below for prompt? We are inling it when we already have a type defined.
| oidc?: UnifiedOidcConfig; | ||
| } | ||
|
|
||
| export type MappedLogLevel = 'error' | 'warn' | 'info' | 'debug' | 'none'; |
There was a problem hiding this comment.
This is another duplicate of LogLevelString and LogLevel
| */ | ||
| export async function davinci<ActionType extends ActionTypes = ActionTypes>({ | ||
| config, | ||
| config: rawConfig, |
There was a problem hiding this comment.
Rather than handling this new config structure in each client like we are doing here, I'd rather see a utility function that transforms this new config to our existing config. Something like this:
const davinciClient = davinci(configUtil(unifiedConfig));
The configUtil validates the unified config object and returns the strongly typed config structure that is already in use.
6 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