SDKS-3938: Migrate device client by ancheetah · Pull Request #318 · ForgeRock/ping-javascript-sdk

ancheetah · 2025-06-05T18:32:12Z

JIRA Ticket

https://pingidentity.atlassian.net/browse/SDKS-3938

Description

Copied device client package from legacy sdk to unified sdk
adds manual e2e test scripts

No changeset. Added device client package and e2e app to changeset ignore

changeset-bot · 2025-06-05T18:32:16Z

⚠️ No Changeset found

Latest commit: a3f4d67

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

nx-cloud · 2025-06-05T19:00:09Z

View your CI Pipeline Execution ↗ for commit a3f4d67.

Command	Status	Duration	Result
`nx affected -t build typecheck lint test e2e-ci`	✅ Succeeded	1m 31s	View ↗
`nx-cloud record -- nx format:check`	✅ Succeeded	2s	View ↗

☁️ Nx Cloud last updated this comment at 2025-06-11 19:42:49 UTC

codecov-commenter · 2025-06-05T19:03:01Z

Codecov Report

Attention: Patch coverage is 88.88889% with 39 lines in your changes missing coverage. Please review.

Project coverage is 61.66%. Comparing base (3129a9e) to head (a3f4d67).

Files with missing lines	Patch %	Lines
...s/device-client/src/lib/device.store.test.utils.ts	78.57%	33 Missing ⚠️
packages/device-client/src/lib/device.store.ts	97.94%	3 Missing ⚠️
packages/device-client/src/lib/types/index.ts	0.00%	2 Missing ⚠️
packages/device-client/src/types.ts	50.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #318       +/-   ##
===========================================
+ Coverage   48.16%   61.66%   +13.49%     
===========================================
  Files          29       33        +4     
  Lines        1715     1985      +270     
  Branches      192      288       +96     
===========================================
+ Hits          826     1224      +398     
+ Misses        889      761      -128

Files with missing lines	Coverage Δ
...ckages/device-client/src/lib/device.store.utils.ts	`100.00% <100.00%> (ø)`
packages/device-client/src/lib/services/index.ts	`100.00% <100.00%> (+98.73%)`	⬆️
.../device-client/src/lib/types/bound-device.types.ts	`100.00% <ø> (ø)`
packages/device-client/src/lib/types/oath.types.ts	`100.00% <ø> (ø)`
...evice-client/src/lib/types/profile-device.types.ts	`100.00% <100.00%> (ø)`
...s/device-client/src/lib/types/push-device.types.ts	`100.00% <ø> (ø)`
...ages/device-client/src/lib/types/webauthn.types.ts	`100.00% <ø> (ø)`
packages/device-client/src/types.ts	`50.00% <50.00%> (ø)`
packages/device-client/src/lib/types/index.ts	`14.28% <0.00%> (-2.39%)`	⬇️
packages/device-client/src/lib/device.store.ts	`95.05% <97.94%> (+94.00%)`	⬆️
... and 1 more

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

github-actions · 2025-06-05T19:03:11Z

Deployed ff8be35 to https://ForgeRock.github.io/ping-javascript-sdk/pr-318/ff8be35c780c436dfdfa0f494a68e92a49582e70 branch gh-pages in ForgeRock/ping-javascript-sdk

cerebrl

I think this is really well done. I like the reuse of the autoscript for this particular context. It's super clean. Let's just discuss whether we want to carryover RxJS into this repo or not.

cerebrl · 2025-06-05T23:05:52Z

 export type OathDevice = {
-  id: string;
+  _id: string;
+  deviceManagementStatus: boolean;
  deviceName: string;
  uuid: string;
-  createdDate: Date;
-  lastAccessDate: Date;
+  createdDate: number;
+  lastAccessDate: number;
+  _rev: string;


This reminds me of a MongoDB document. I wonder if they are using a NoSQL data store for storying this info.

cerebrl · 2025-06-06T15:52:22Z

-        return response.data;
+          return response.data.result;
+        } catch (error) {
+          return { error };


@ryanbas21 Should these methods return the GenericError type that we settled on in DaVinci Client? My thought is that we should probably be consistent with errors throughout this SDK suite.

cerebrl · 2025-06-06T16:15:39Z

+} from './mock-data/msw-mock-data.js';
+
+// Create handlers
+export const handlers = [


I feel like this array of handlers should be moved to its own file. That would shorten this file up and make it a bit more readable.

ryanbas21

Honestly, this is excellent work. This looks fantastic. I'm just leaving a comment regarding the rxjs discussion

ancheetah · 2025-06-10T22:55:19Z

updated device client test user to avoid conflicts with Stoyan's automated tests
moved msw handlers to their own file
updated device client delete methods to return null
updated device client integration tests
removed rxjs from e2e tests and replaced with Effect

cerebrl

Initial scan looks good. Just a few small comments.

cerebrl · 2025-06-11T14:31:17Z

-          return undefined;
-        }
+          if (error) {
+            handleError(error, 'Failed to delete device: ');


I feel like we should use handleError consistently throughout these store methods. Is there a reason we only use it in some?

I was just trying not to fix what wasn't broken. The other get/update methods originally didn't handle errors from RTK. They just check for data and if it's missing it'll throw a 'response did not contain data' error. To be more consistent I can update the other methods to check for errors first before returning data.

Fair enough. We can improve this in the future.

cerebrl · 2025-06-11T14:51:04Z

    endpoints: (builder) => ({
      // oath endpoints
-      getOAthDevices: builder.query<OAthResponse, RetrieveOathQuery>({
+      getOAthDevices: builder.query<OathResponse, RetrieveOathQuery>({


Can we change OAth to Oath?

cerebrl

I think this is good for the migration story. I'll make a few improvement stories in Jira to continue iterating on this foundation.

ancheetah force-pushed the SDKS-3938-update-device-client branch from 3ab8247 to 22a1021 Compare June 5, 2025 20:14

cerebrl reviewed Jun 5, 2025

View reviewed changes

cerebrl reviewed Jun 6, 2025

View reviewed changes

ryanbas21 approved these changes Jun 6, 2025

View reviewed changes

Comment thread e2e/device-client-app/src/autoscript.ts Outdated

ancheetah marked this pull request as ready for review June 10, 2025 22:54

ancheetah requested review from cerebrl and ryanbas21 June 10, 2025 22:55

cerebrl requested changes Jun 11, 2025

View reviewed changes

ancheetah and others added 4 commits June 11, 2025 15:27

chore(device-client): update device client

032044a

test(device-client): add e2e tests

b0e387a

chore: refactoring-work

353f1fd

chore(device-client): finish refactor

a3f4d67

ancheetah force-pushed the SDKS-3938-update-device-client branch from e672208 to a3f4d67 Compare June 11, 2025 19:40

ryanbas21 approved these changes Jun 11, 2025

View reviewed changes

cerebrl approved these changes Jun 11, 2025

View reviewed changes

ancheetah merged commit 0b9f15d into main Jun 12, 2025
4 checks passed

ancheetah deleted the SDKS-3938-update-device-client branch June 12, 2025 15:24

Conversation

ancheetah commented Jun 5, 2025

JIRA Ticket

Description

Uh oh!

changeset-bot Bot commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

nx-cloud Bot commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions Bot commented Jun 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cerebrl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cerebrl Jun 5, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cerebrl Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cerebrl Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

ryanbas21 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ancheetah commented Jun 10, 2025

Uh oh!

cerebrl left a comment

Choose a reason for hiding this comment

Uh oh!

cerebrl Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

ancheetah Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

cerebrl Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

cerebrl Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

cerebrl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

changeset-bot Bot commented Jun 5, 2025 •

edited

Loading

nx-cloud Bot commented Jun 5, 2025 •

edited

Loading

codecov-commenter commented Jun 5, 2025 •

edited

Loading

github-actions Bot commented Jun 5, 2025 •

edited

Loading

cerebrl Jun 6, 2025 •

edited

Loading