SDKS-5067: Standardize SDK Configuration#684
Conversation
🦋 Changeset detectedLatest commit: a45f2b1 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 a45f2b1
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughIntroduces unified cross-platform SDK configuration support. New ChangesUnified SDK Config Adoption
Sequence Diagram(s)sequenceDiagram
participant Consumer
participant makeOidcConfig
participant validateUnifiedSdkConfig
participant validateUnifiedOidcConfig
participant unifiedToOidcConfig
participant oidcClientStore
Consumer->>makeOidcConfig: makeOidcConfig(json)
makeOidcConfig->>validateUnifiedSdkConfig: validate(json, strict=true)
validateUnifiedSdkConfig->>validateUnifiedOidcConfig: validate(json.oidc, strict=true)
validateUnifiedOidcConfig-->>validateUnifiedSdkConfig: Either right UnifiedOidcConfig
validateUnifiedSdkConfig-->>makeOidcConfig: Either right UnifiedSdkConfig
makeOidcConfig->>unifiedToOidcConfig: map(unifiedSdkConfig)
unifiedToOidcConfig-->>makeOidcConfig: Either right OidcConfig
makeOidcConfig-->>Consumer: OidcConfig (throws on failure)
Consumer->>oidcClientStore: oidc({ config: OidcConfig })
oidcClientStore-->>Consumer: { authorize, logout, ... }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 ❌ Your project status has failed because the head coverage (22.03%) 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 #684 +/- ##
==========================================
+ Coverage 18.07% 22.03% +3.95%
==========================================
Files 155 157 +2
Lines 24398 25105 +707
Branches 1203 1522 +319
==========================================
+ Hits 4410 5531 +1121
+ Misses 19988 19574 -414
🚀 New features to boost your workflow:
|
|
Deployed b8189e5 to https://ForgeRock.github.io/ping-javascript-sdk/pr-684/b8189e5704884623eb1eb73a2d94f4c68761ac49 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🆕 New Packages🆕 @forgerock/oidc-client - 30.6 KB (new) 14 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
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
There was a problem hiding this comment.
Updated it. Thanks!
| 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
There was a problem hiding this comment.
Updated it to use Either instead. Thanks for the suggestion!
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.
There was a problem hiding this comment.
Goal was to accept both legacy and unified configs, but for sure this was not the ideal way to achieve this. An utility function was added that validates and returns the config type each client expects, so the client interfaces stay the same.
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
There was a problem hiding this comment.
Right, good catch. Moved type declaration to authorize.request.utils.ts and config.types.ts imports from there.
| * 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.
There was a problem hiding this comment.
Moved LogLevel to sdk-types (only layer sdk-utilities can import from). sdk-effects/logger re-exports it from there. LogLevelString alias removed.
| */ | ||
| 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.
There was a problem hiding this comment.
Added a utility function for each client that vaidates and creates the right config type from the unified config.
Thanks!
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/oidc-client/api-report/oidc-client.api.md (1)
241-266:⚠️ Potential issue | 🔴 CriticalRemove duplicate
OauthTokensinterface declaration.The
OauthTokensinterface is declared twice inpackages/oidc-client/src/lib/config.types.ts(lines 9-15 and 17-23) with identical content. In TypeScript, the second declaration overrides the first, making the duplicate redundant. Remove one of the declarations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/oidc-client/api-report/oidc-client.api.md` around lines 241 - 266, The OauthTokens interface is declared twice identically in the source file, causing the duplicate to appear in the API report. Locate the two identical OauthTokens interface declarations (one without the `@public` decorator and one with it) and remove the second duplicate declaration, keeping only the first one. This will eliminate the redundant interface from the API report file.
🧹 Nitpick comments (5)
packages/sdk-utilities/src/lib/config/config.utils.ts (4)
49-51: ⚡ Quick winImprove error message for array inputs.
The check
typeof input !== 'object' || input === nulldoesn't explicitly reject arrays. While arrays will fail validation later, the error message "Expected object, got object" is confusing when the input is an array.📝 Suggested improvement
- if (typeof input !== 'object' || input === null) { - return Either.left([{ field: prefix, message: `Expected object, got ${typeof input}` }]); + if (typeof input !== 'object' || input === null || Array.isArray(input)) { + return Either.left([{ field: prefix, message: `Expected object, got ${Array.isArray(input) ? 'array' : typeof input}` }]); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk-utilities/src/lib/config/config.utils.ts` around lines 49 - 51, The type check in the condition at this location does not explicitly reject arrays, which causes confusing error messages since typeof array returns 'object'. Modify the condition to additionally check if the input is an array using Array.isArray(), and update the error message to properly distinguish between array and non-object types by checking for arrays first and returning a message like "Expected object, got array" when an array is detected, otherwise use the existing type-based message.
259-266: ⚡ Quick winType guard doesn't explicitly exclude arrays.
The checks for
oidcandjourneyverify they're objects and not null, buttypeof [] === 'object'is true in JavaScript. While validation will catch array inputs later, the type guard should be more precise to ensure correct branching logic.🔍 Suggested improvement
const oidcIsObject = - 'oidc' in raw && typeof raw['oidc'] === 'object' && raw['oidc'] !== null; + 'oidc' in raw && typeof raw['oidc'] === 'object' && raw['oidc'] !== null && !Array.isArray(raw['oidc']); const journeyIsObject = - 'journey' in raw && typeof raw['journey'] === 'object' && raw['journey'] !== null; + 'journey' in raw && typeof raw['journey'] === 'object' && raw['journey'] !== null && !Array.isArray(raw['journey']);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk-utilities/src/lib/config/config.utils.ts` around lines 259 - 266, The type guard function isUnifiedSdkConfig does not properly exclude arrays when checking if oidc and journey properties are objects. Since typeof [] === 'object' is true in JavaScript, arrays will pass the current object type checks. Add an explicit Array.isArray() check to exclude arrays for both the oidcIsObject and journeyIsObject conditions, ensuring that only plain objects (not arrays) satisfy the type guard requirements.
146-148: ⚡ Quick winImprove error message clarity for arrays.
The array check is present, but the error message doesn't distinguish between arrays and other non-object types. Use the same pattern as line 108 for consistency.
📝 Suggested improvement
if (typeof raw['journey'] !== 'object' || Array.isArray(raw['journey'])) { - return [{ field: 'journey', message: `Expected object, got ${typeof raw['journey']}` }]; + return [{ field: 'journey', message: `Expected object, got ${Array.isArray(raw['journey']) ? 'array' : typeof raw['journey']}` }]; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk-utilities/src/lib/config/config.utils.ts` around lines 146 - 148, The error message for the journey field validation does not distinguish between array and non-object types in the error message. Update the validation logic to provide a more specific error message that explicitly states "Expected object, got array" when the value is an array, following the same pattern used at line 108 for consistency. This requires checking Array.isArray(raw['journey']) separately and providing the appropriate error message for each case rather than using a generic typeof check in the error message.
130-132: ⚡ Quick winImprove error message for array inputs.
Same issue as in
validateUnifiedOidcConfig(line 49): array inputs should be explicitly rejected with a clear error message.📝 Suggested improvement
- if (typeof input !== 'object' || input === null) { - return Either.left([{ field: 'config', message: `Expected object, got ${typeof input}` }]); + if (typeof input !== 'object' || input === null || Array.isArray(input)) { + return Either.left([{ field: 'config', message: `Expected object, got ${Array.isArray(input) ? 'array' : typeof input}` }]); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/sdk-utilities/src/lib/config/config.utils.ts` around lines 130 - 132, The validation check at lines 130-132 does not explicitly reject array inputs, even though arrays should be invalid. Since typeof array returns 'object' in JavaScript, arrays would currently pass this validation. Add an explicit check using Array.isArray() to detect and reject array inputs with a clear error message that distinguishes arrays from other invalid types, similar to how the issue is already handled in validateUnifiedOidcConfig at line 49. Update the condition to catch arrays early and return an appropriate error.packages/oidc-client/src/lib/config.types.ts (1)
9-15: ⚡ Quick winRemove the duplicate
OauthTokensdeclaration.
OauthTokensis declared twice in this file. Keeping a single declaration avoids unnecessary declaration merging and reduces maintenance risk.Suggested cleanup
-export interface OauthTokens { - accessToken: string; - idToken: string; - refreshToken?: string; - expiresAt?: number; - expiryTimestamp?: number; -}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/oidc-client/src/lib/config.types.ts` around lines 9 - 15, The OauthTokens interface is declared twice in the file, which causes unnecessary declaration merging. Locate both declarations of the OauthTokens interface and remove the duplicate declaration, keeping only one instance that contains all the necessary properties (accessToken, idToken, refreshToken, expiresAt, and expiryTimestamp). If the two declarations have different properties, merge them into a single definition before removing the duplicate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/sdks-5067-unified-config.md:
- Line 23: The changeset documentation on line 23 references incorrect type
names `OidcDisplayValue` and `OidcPromptValue` as being exported from
sdk-utilities, but the actual canonical type names exported from sdk-types are
`AuthDisplayValue` and `AuthPromptValue`. Either remove these type references
from the sdk-utilities bullet point on line 23, or move them to the sdk-types
section (lines 33-35) with the correct type names `AuthDisplayValue` and
`AuthPromptValue`, clarifying that sdk-types is the canonical source for these
types.
---
Outside diff comments:
In `@packages/oidc-client/api-report/oidc-client.api.md`:
- Around line 241-266: The OauthTokens interface is declared twice identically
in the source file, causing the duplicate to appear in the API report. Locate
the two identical OauthTokens interface declarations (one without the `@public`
decorator and one with it) and remove the second duplicate declaration, keeping
only the first one. This will eliminate the redundant interface from the API
report file.
---
Nitpick comments:
In `@packages/oidc-client/src/lib/config.types.ts`:
- Around line 9-15: The OauthTokens interface is declared twice in the file,
which causes unnecessary declaration merging. Locate both declarations of the
OauthTokens interface and remove the duplicate declaration, keeping only one
instance that contains all the necessary properties (accessToken, idToken,
refreshToken, expiresAt, and expiryTimestamp). If the two declarations have
different properties, merge them into a single definition before removing the
duplicate.
In `@packages/sdk-utilities/src/lib/config/config.utils.ts`:
- Around line 49-51: The type check in the condition at this location does not
explicitly reject arrays, which causes confusing error messages since typeof
array returns 'object'. Modify the condition to additionally check if the input
is an array using Array.isArray(), and update the error message to properly
distinguish between array and non-object types by checking for arrays first and
returning a message like "Expected object, got array" when an array is detected,
otherwise use the existing type-based message.
- Around line 259-266: The type guard function isUnifiedSdkConfig does not
properly exclude arrays when checking if oidc and journey properties are
objects. Since typeof [] === 'object' is true in JavaScript, arrays will pass
the current object type checks. Add an explicit Array.isArray() check to exclude
arrays for both the oidcIsObject and journeyIsObject conditions, ensuring that
only plain objects (not arrays) satisfy the type guard requirements.
- Around line 146-148: The error message for the journey field validation does
not distinguish between array and non-object types in the error message. Update
the validation logic to provide a more specific error message that explicitly
states "Expected object, got array" when the value is an array, following the
same pattern used at line 108 for consistency. This requires checking
Array.isArray(raw['journey']) separately and providing the appropriate error
message for each case rather than using a generic typeof check in the error
message.
- Around line 130-132: The validation check at lines 130-132 does not explicitly
reject array inputs, even though arrays should be invalid. Since typeof array
returns 'object' in JavaScript, arrays would currently pass this validation. Add
an explicit check using Array.isArray() to detect and reject array inputs with a
clear error message that distinguishes arrays from other invalid types, similar
to how the issue is already handled in validateUnifiedOidcConfig at line 49.
Update the condition to catch arrays early and return an appropriate error.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 408272e8-09af-432e-a266-9bc32654662e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (37)
.changeset/sdks-5067-unified-config.mdpackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/client.store.test.tspackages/davinci-client/src/lib/client.store.tspackages/davinci-client/src/lib/config.types.tspackages/journey-client/api-report/journey-client.api.mdpackages/journey-client/api-report/journey-client.types.api.mdpackages/journey-client/package.jsonpackages/journey-client/src/lib/client.store.test.tspackages/journey-client/src/lib/client.store.tspackages/journey-client/src/lib/config.types.tspackages/oidc-client/api-report/oidc-client.api.mdpackages/oidc-client/api-report/oidc-client.types.api.mdpackages/oidc-client/src/lib/authorize.request.micros.test.tspackages/oidc-client/src/lib/authorize.request.micros.tspackages/oidc-client/src/lib/authorize.request.utils.test.tspackages/oidc-client/src/lib/authorize.request.utils.tspackages/oidc-client/src/lib/client.store.test.tspackages/oidc-client/src/lib/client.store.tspackages/oidc-client/src/lib/config.types.tspackages/oidc-client/src/lib/logout.request.test.tspackages/oidc-client/src/lib/logout.request.tspackages/oidc-client/src/lib/oidc.api.tspackages/sdk-effects/logger/package.jsonpackages/sdk-effects/logger/src/lib/logger.types.tspackages/sdk-effects/logger/tsconfig.lib.jsonpackages/sdk-effects/oidc/src/lib/authorize.test.tspackages/sdk-effects/oidc/src/lib/authorize.utils.tspackages/sdk-types/src/lib/authorize.types.tspackages/sdk-types/src/lib/config.types.tspackages/sdk-utilities/package.jsonpackages/sdk-utilities/src/index.tspackages/sdk-utilities/src/lib/config/config.test.tspackages/sdk-utilities/src/lib/config/config.types.tspackages/sdk-utilities/src/lib/config/config.utils.tspackages/sdk-utilities/src/lib/config/index.ts
SteinGabriel
force-pushed
the
SDKS-5067
branch
3 times, most recently
from
ac08161 to
dd74f99
Compare
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 fix(sdk): tighten config param types to XConfig | UnifiedSdkConfig test(oidc-client): migrate authorize.request.micros tests to @effect/vitest test(oidc-client): migrate authorize.request.utils tests to @effect/vitest refactor(sdk-utilities): use as const satisfies for required-field constants refactor(sdk-utilities): replace mutable error array with effect Either in config validators refactor(sdk): utility-first config — move json→config transform to sdk-utilities refactor(sdk-utilities): remove LogLevel duplicates — use LogLevel from sdk-types docs(changeset): update sdks-5067 changeset to reflect utility-first config API refactor(sdk-utilities): rename *ConfigFromJson to make*Config refactor(sdk-types): move client config types to sdk-types, delete Mapped* mirrors fix(oidc-client): remove duplicate OauthTokens interface declaration fix(oidc-client): remove config.state from OidcConfig — PKCE state must not be overridden
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.changeset/sdks-5067-unified-config.md:
- Line 24: The changeset bullet for `AuthDisplayValue` and `AuthPromptValue`
types contradicts other parts of the PR documentation by claiming
`sdk-utilities` is the canonical source when `sdk-types` is actually documented
as owning these types. Either reword this bullet point to clarify that
`sdk-utilities` re-exports or consumes these types (removing the "canonical
source" claim), or move the canonical source ownership note into the `sdk-types`
section of the changeset where it accurately reflects the actual location of
these type definitions. Choose whichever approach is consistent with the PR
summary and other documentation sections.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fded97f-445e-46e9-a700-ccf208e5d75c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (39)
.changeset/sdks-5067-unified-config.mde2e/journey-suites/src/login.test.tspackages/davinci-client/api-report/davinci-client.api.mdpackages/davinci-client/api-report/davinci-client.types.api.mdpackages/davinci-client/src/lib/client.store.test.tspackages/davinci-client/src/lib/client.store.tspackages/davinci-client/src/lib/config.types.tspackages/journey-client/api-report/journey-client.api.mdpackages/journey-client/api-report/journey-client.types.api.mdpackages/journey-client/src/lib/client.store.test.tspackages/journey-client/src/lib/client.store.tspackages/journey-client/src/lib/config.types.tspackages/journey-client/src/lib/journey.utils.tspackages/journey-client/src/types.tspackages/oidc-client/api-report/oidc-client.api.mdpackages/oidc-client/api-report/oidc-client.types.api.mdpackages/oidc-client/src/lib/authorize.request.micros.test.tspackages/oidc-client/src/lib/authorize.request.micros.tspackages/oidc-client/src/lib/authorize.request.utils.test.tspackages/oidc-client/src/lib/authorize.request.utils.tspackages/oidc-client/src/lib/client.store.test.tspackages/oidc-client/src/lib/client.store.tspackages/oidc-client/src/lib/config.types.tspackages/oidc-client/src/lib/logout.request.test.tspackages/oidc-client/src/lib/logout.request.tspackages/oidc-client/src/lib/oidc.api.tspackages/sdk-effects/logger/package.jsonpackages/sdk-effects/logger/src/lib/logger.types.tspackages/sdk-effects/logger/tsconfig.lib.jsonpackages/sdk-effects/oidc/src/lib/authorize.test.tspackages/sdk-effects/oidc/src/lib/authorize.utils.tspackages/sdk-types/src/lib/authorize.types.tspackages/sdk-types/src/lib/config.types.tspackages/sdk-utilities/package.jsonpackages/sdk-utilities/src/index.tspackages/sdk-utilities/src/lib/config/config.test.tspackages/sdk-utilities/src/lib/config/config.types.tspackages/sdk-utilities/src/lib/config/config.utils.tspackages/sdk-utilities/src/lib/config/index.ts
✅ Files skipped from review due to trivial changes (6)
- packages/journey-client/src/lib/journey.utils.ts
- packages/journey-client/src/types.ts
- e2e/journey-suites/src/login.test.ts
- packages/sdk-effects/logger/tsconfig.lib.json
- packages/oidc-client/src/lib/authorize.request.micros.test.ts
- packages/journey-client/api-report/journey-client.api.md
🚧 Files skipped from review as they are similar to previous changes (30)
- packages/oidc-client/src/lib/client.store.ts
- packages/sdk-utilities/package.json
- packages/journey-client/src/lib/config.types.ts
- packages/oidc-client/src/lib/oidc.api.ts
- packages/sdk-utilities/src/index.ts
- packages/oidc-client/src/lib/logout.request.ts
- packages/oidc-client/src/lib/authorize.request.utils.test.ts
- packages/sdk-effects/oidc/src/lib/authorize.utils.ts
- packages/sdk-effects/logger/package.json
- packages/sdk-effects/logger/src/lib/logger.types.ts
- packages/oidc-client/src/lib/client.store.test.ts
- packages/davinci-client/src/lib/client.store.test.ts
- packages/sdk-utilities/src/lib/config/index.ts
- packages/journey-client/src/lib/client.store.ts
- packages/sdk-types/src/lib/authorize.types.ts
- packages/davinci-client/src/lib/client.store.ts
- packages/journey-client/src/lib/client.store.test.ts
- packages/sdk-types/src/lib/config.types.ts
- packages/oidc-client/api-report/oidc-client.api.md
- packages/davinci-client/src/lib/config.types.ts
- packages/sdk-utilities/src/lib/config/config.types.ts
- packages/oidc-client/src/lib/authorize.request.micros.ts
- packages/davinci-client/api-report/davinci-client.api.md
- packages/oidc-client/src/lib/config.types.ts
- packages/davinci-client/api-report/davinci-client.types.api.md
- packages/journey-client/api-report/journey-client.types.api.md
- packages/sdk-effects/oidc/src/lib/authorize.test.ts
- packages/oidc-client/src/lib/logout.request.test.ts
- packages/sdk-utilities/src/lib/config/config.utils.ts
- packages/sdk-utilities/src/lib/config/config.test.ts
| - `validateUnifiedSdkConfig` / `validateUnifiedOidcConfig` — pure validation returning `Either<T, ConfigValidationError[]>` | ||
| - `unifiedToOidcConfig`, `unifiedToJourneyConfig`, `unifiedToDavinciConfig` — pure mappers returning `Either<T, ConfigValidationError>` | ||
| - `isUnifiedSdkConfig` discriminator | ||
| - `AuthDisplayValue`, `AuthPromptValue` types (canonical source — shared between `OidcConfig` and `GetAuthorizationUrlOptions`) |
There was a problem hiding this comment.
Clarify where AuthDisplayValue / AuthPromptValue are actually owned.
This bullet still says sdk-utilities is the “canonical source,” but the PR summary and the sdk-types section place those types there. Please reword this to say sdk-utilities consumes or re-exports them, or move the ownership note into the sdk-types section so the changeset doesn’t describe two sources of truth.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.changeset/sdks-5067-unified-config.md at line 24, The changeset bullet for
`AuthDisplayValue` and `AuthPromptValue` types contradicts other parts of the PR
documentation by claiming `sdk-utilities` is the canonical source when
`sdk-types` is actually documented as owning these types. Either reword this
bullet point to clarify that `sdk-utilities` re-exports or consumes these types
(removing the "canonical source" claim), or move the canonical source ownership
note into the `sdk-types` section of the changeset where it accurately reflects
the actual location of these type definitions. Choose whichever approach is
consistent with the PR summary and other documentation sections.
6 participants
Summary
https://pingidentity.atlassian.net/browse/SDKS-5067
Implements a unified cross-platform JSON configuration schema for the JS SDK. A new
sdk-utilitiesconfigmodule validates and maps the unified JSON shape to each factory's native config type. All three factories
(
oidc,journey,davinci) retain their existing typedconfigparameter — callers convert unified JSON viamakeOidcConfig/makeJourneyConfig/makeDavinciConfigbefore passing it in. Non-breaking: existingnative config objects continue to work unchanged.
Changes
packages/sdk-typesconfig.types.ts—OidcConfig,JourneyClientConfig,JourneyServerConfig,DaVinciConfigmoved here ascanonical definitions; extended with
signOutRedirectUri,loginHint,state,nonce,display,prompt,uiLocales,acrValues,query,logonOidcConfigauthorize.types.ts—GetAuthorizationUrlOptionsextended withloginHint,nonce,display,uiLocales,acrValues;promptwidened to include'select_account';AuthDisplayValueandAuthPromptValuedefined herepackages/sdk-utilitiessrc/lib/config/module:config.types.ts—UnifiedSdkConfig,UnifiedOidcConfig,LogLevelString, re-exportsAuthDisplayValue,AuthPromptValuefromsdk-types; no enums,type/interfaceonlyconfig.utils.ts— pure validation and mapping; useseffectEither.Either<T, ConfigValidationError[]>for results:
validateUnifiedOidcConfig/validateUnifiedSdkConfig— required-field and type checks; strict mode foroidc/davinci, lenient (only
discoveryEndpointrequired) for journeyisUnifiedSdkConfig— discriminator:'oidc' in input || 'journey' in inputunifiedToOidcConfig/unifiedToJourneyConfig/unifiedToDavinciConfig— pure mappers:scopes[]→scopestring,refreshThreshold(sec) →oauthThreshold(ms),log→logLevel,discoveryEndpoint→serverConfig.wellknownmakeOidcConfig(json)/makeJourneyConfig(json)/makeDavinciConfig(json)— public entry points;validate + map + throw on invalid; return the native typed config
config/index.ts— barrel exportsrc/index.ts— exportsconfigmodulepackages/oidc-clientconfig.types.ts— re-export shim; canonical types now insdk-typesoidc.api.ts— appendspost_logout_redirect_urito end-session URL whenconfig.signOutRedirectUriis setauthorize.request.utils.ts/authorize.request.micros.ts— useAuthPromptValuefromsdk-utilities(wasduplicate local
PromptValue)client.store.ts—authorize.url()forwards new OIDC params (loginHint,state,nonce,display,prompt,uiLocales,acrValues,query) from config intocreateAuthorizeUrlpackages/journey-clientconfig.types.ts— re-export shim; canonical types now insdk-typespackages/davinci-clientconfig.types.ts— re-export shim; canonical types now insdk-typespackages/sdk-effects/oidcauthorize.utils.ts—buildAuthorizeParamswiresloginHint,nonce,display,uiLocales,acrValuesinto the authorize URL
Tests
config.test.tsinsdk-utilities: valid mapping for all three factories, eachrequired-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 caseseach (validation failure, mapping failure) via
make*Configauthorize.test.tsinsdk-effects/oidc: 6 tests covering each new OIDC param individually and all togetherauthorize.request.micros.test.ts,authorize.request.utils.test.ts,logout.request.test.tsinoidc-clientfor new config shapeHow to test
1. Unit tests