refactor: remove @forgerock/javascript-sdk dependency from device-client and e2e app - #816
refactor: remove @forgerock/javascript-sdk dependency from device-client and e2e app#816ryanbas21 wants to merge 3 commits into
Conversation
|
Important Draft PR not reviewedDraft PRs are not automatically reviewed by default.
To automatically review draft PRs, update your CodeRabbit configuration: reviews:
auto_review:
drafts: trueThanks 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 e7cf5ef
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
@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: |
|
Deployed faf19eb to https://ForgeRock.github.io/ping-javascript-sdk/pr-816/faf19eb620411d968e0c25495f326d9012ffee8f branch gh-pages in ForgeRock/ping-javascript-sdk |
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (24.35%) 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 #816 +/- ##
==========================================
+ Coverage 18.07% 24.35% +6.28%
==========================================
Files 155 164 +9
Lines 24398 25823 +1425
Branches 1203 1703 +500
==========================================
+ Hits 4410 6290 +1880
+ Misses 19988 19533 -455
🚀 New features to boost your workflow:
|
📦 Bundle Size Analysis📦 Bundle Size Analysis🆕 New Packages🆕 @forgerock/device-client - 0.0 KB (new) 📊 Minor Changes📈 @forgerock/sdk-types - 9.1 KB (+0.0 KB) ➖ No Changes➖ @forgerock/recognize - 4284.4 KB 15 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
e2d67ec to
c4f8ecb
Compare
There was a problem hiding this comment.
Tysm for doing this! Looks good, just a couple questions. Would be great if we could get a ticket on the board to align the Device Client config with our other clients so you could pass around the same config object.
I still need to verify the tests manually but I'll let you know how it goes.
| * of the MIT license. See the LICENSE file for details. | ||
| */ | ||
| import { type ConfigOptions } from '@forgerock/javascript-sdk'; | ||
| import { type ConfigOptions } from './types/index.js'; |
There was a problem hiding this comment.
Is this a circular dependency? Should we import it from sdk-types package instead?
import type { LegacyConfigOptions as ConfigOptions } from '@forgerock/sdk-types';
| export type { ConfigOptions } from '@forgerock/javascript-sdk'; | ||
| import type { LegacyConfigOptions } from '@forgerock/sdk-types'; | ||
|
|
||
| export type ConfigOptions = LegacyConfigOptions; |
There was a problem hiding this comment.
Is this technically a breaking change we need to be concerned about? The tokenStore option now accepts CustomStorageObject instead of TokenStoreObject (legacy). This change makes sense and I know most of the properties in this config object are ignored by device client, including tokenStore, but if someone was previously passing a TokenStoreObject then this breaks for them.
| @@ -1,63 +1,66 @@ | |||
| /* | |||
| * | |||
| * Copyright © 2025 Ping Identity Corporation. All right reserved. | |||
There was a problem hiding this comment.
copyright should be 2025 - 2026
| yield* Effect.tryPromise({ | ||
| try: () => oidcClientOrThrow().user.logout(), | ||
| catch: (err) => new Error(`Logout failed: ${err}`), | ||
| }); |
There was a problem hiding this comment.
Do we need this? If we are starting the app from a fresh state then won't this always fail preventing you from proceeding?
| if (response && 'error' in response) { | ||
| throw new Error(`Failed to get user info: ${response.error}`); | ||
| } | ||
| return response as unknown as Record<string, string>; |
There was a problem hiding this comment.
Nitpick but I don't think we need this cast. We have a better typed user response in the new oidc client. The device app only uses user.sub which I think is guaranteed from the new type.
| const callNext = (client: JourneyClient, step: JourneyResult) => | ||
| Effect.tryPromise({ | ||
| try: () => FRAuth.next(step), | ||
| try: () => client.next(step as Parameters<JourneyClient['next']>[0]), |
There was a problem hiding this comment.
Another nitpick but I don't think we need this cast either if you type the step parameter as JourneyStep
…ypes ConfigOptions now aliases LegacyConfigOptions from @forgerock/sdk-types, removing the runtime dependency on @forgerock/javascript-sdk. The catalog entry remains for tools/interface-mapping-validator.
…lient Replaces @forgerock/javascript-sdk usage with journey-client and oidc-client. Token renewal now uses backgroundRenew and oidc() error results are validated before caching. platformHeader is dropped as it is unsupported by the new clients. Lockfile updates included here.
Addresses mend scan findings for transitive CVEs: - fast-uri ^3.1.6 -> ^3.1.7 - undici ^7.29.0 -> ^7.29.1 vitest (CVE-2026-84373) is intentionally not touched here and will be updated separately.
c4f8ecb to
e7cf5ef
Compare
3 participants
Summary
Removes the legacy
@forgerock/javascript-sdkruntime dependency from two places:packages/device-client—ConfigOptionsnow aliasesLegacyConfigOptionsfrom@forgerock/sdk-types, removing the@forgerock/javascript-sdkdependency entirely.e2e/device-client-app— migrated from@forgerock/javascript-sdkto@forgerock/journey-client+@forgerock/oidc-client.Config mapping (legacy → new clients)
Config/ javascript-sdk)serverConfig.baseUrlserverConfigrealmPathrealmPathon the respective client configclientIdclientIdon OIDC client configtoken.get()(manual renew)token.get({ backgroundRenew: true })platformHeaderBehavior notes
backgroundRenew: true).oidc()error results are validated before caching (previously errors could be cached).platformHeaderparam dropped (not supported by the new clients).@forgerock/javascript-sdkcatalog entry is intentionally kept —tools/interface-mapping-validatorstill uses it.Verification
device-clientbuild / lint / test ✅device-client-appbuild / lint / typecheck / dev-server ✅journey-suitese2e: 15 passed / 0 failed ✅device-client-app— validated via manual tenant harness.