SDKS-3941: Migrate Protect from Legacy to new Ping SDK #274

ancheetah · 2025-05-07T22:51:41Z

JIRA Ticket

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

Description

Moves Ping Protect module from legacy SDK using functional pattern. Improves JS Doc annotations and unit tests.

No changeset.

changeset-bot · 2025-05-07T22:51:46Z

⚠️ No Changeset found

Latest commit: 119cbb2

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-05-07T22:58:37Z

View your CI Pipeline Execution ↗ for commit 119cbb2.

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

☁️ Nx Cloud last updated this comment at 2025-05-09 16:26:50 UTC

github-actions · 2025-05-07T23:01:03Z

Deployed a146209 to https://ForgeRock.github.io/ping-javascript-sdk/pr-274/a146209e1fd443e5f79f7a70ca475a6d059efd21 branch gh-pages in ForgeRock/ping-javascript-sdk

codecov-commenter · 2025-05-07T23:01:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 49.33%. Comparing base (6bc605b) to head (119cbb2).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #274   +/-   ##
=======================================
  Coverage   49.33%   49.33%           
=======================================
  Files          29       29           
  Lines        1571     1571           
  Branches      173      173           
=======================================
  Hits          775      775           
  Misses        796      796

🚀 New features to boost your workflow:

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

cerebrl

Looks good to me. I'll let @ryanbas21 be the final arbiter as most of this has to do with the usual scaffolding/infrastructure implementation.

ancheetah · 2025-05-08T15:53:53Z

packages/protect/src/lib/protect.ts

+         */
+        await import('./signals-sdk.js' as string);
+      } catch (err) {
+        console.error('error loading ping signals', err); // TODO: should we throw an error here?


@cerebrl @ryanbas21 I copied this from the legacy sdk but left one comment here. Wondering if we should throw an error if the signals sdk fails to load. Currently if it fails then it will continue to try to initialize anyways... but that will also fail because no methods would have been added to window._pingOneSignals

I think throwing an error here makes sense.

@cerebrl I'm curious if maybe it makes more sense to load the signals-sdk in the closure? That way if the signals SDK fails to load, we don't actually return the returning object so there's no confusion on the api "working" at that level.

Obviously this would make the factory async, which is a bit different than what we've typically done. But I think it could be useful to error (if we do) at the creation stage, because without the signals-sdk the api is useless.

Yeah, I think that's a good idea.

ryanbas21 · 2025-05-08T17:02:19Z

packages/protect/package.json

@@ -0,0 +1,29 @@
+{


@ancheetah Can we mark sideEffects in this package.json. I believe we have to mark the signals sdk as a side effect like we do in legacy.

ryanbas21 · 2025-05-08T17:05:28Z

packages/protect/src/lib/protect.ts

+         */
+        await import('./signals-sdk.js' as string);
+      } catch (err) {
+        console.error('error loading ping signals', err); // TODO: should we throw an error here?


I think throwing an error here makes sense.

@cerebrl I'm curious if maybe it makes more sense to load the signals-sdk in the closure? That way if the signals SDK fails to load, we don't actually return the returning object so there's no confusion on the api "working" at that level.

Obviously this would make the factory async, which is a bit different than what we've typically done. But I think it could be useful to error (if we do) at the creation stage, because without the signals-sdk the api is useless.

ryanbas21 · 2025-05-08T17:06:40Z

packages/protect/src/lib/protect.types.ts

+ */
+export interface ProtectConfig {
+  /**
+   * @param {string} envId - the environment id from your PingOne tenant


small note: these technically are not params. I think the JSDoc equivalent is property for these.

Ok I can change it to property. I was confused by this doc on the typescript website, not sure if it even pertains to this, but I didn't see property or interface as supported so I opted not to use them.
https://www.typescriptlang.org/docs/handbook/jsdoc-supported-types.html

the descriptions are formatted a little better with param.

@param consoleLogEnabled — true to enable SDK logs in the developer console. default is false

vs.

@property — {boolean} [consoleLogEnabled] - true to enable SDK logs in the developer console. default is false

could this be because typescript doesnt support property?

These are just JSDoc comments, they shouldn't have any affect on typescript. its just a description of the interface/type.

This is mostly a semantic thing.

If you were using the api here in your editor and your intellisense was triggered, you would see @param instead of @Property as a comment, its just a little bit incorrect because it's a property of the object, not a param of a function.

ryanbas21 · 2025-05-08T17:09:16Z

packages/protect/src/lib/protect.ts

+ * @returns {Protect} - a set of methods
+ */
+export function createProtect(config: ProtectConfig): Protect {
+  const options = config;


Is this assignment necessary? We could rename the above parameter to options if we prefer that?

It's not necessary. I can rename it to options. Think I probably got lazy and didn't want to change the naming in the start method.

ancheetah · 2025-05-08T19:23:04Z

moved loading signals sdk out of protect api. added test for failed signals sdk load.

ryanbas21 · 2025-05-09T14:42:15Z

packages/protect/package.json

+    "url": "git+https://github.com/ForgeRock/ping-javascript-sdk.git",
+    "directory": "packages/protect"
+  },
+  "sideEffects": true,


I may have misled you. What I meant was something like this:

https://github.com/ForgeRock/forgerock-javascript-sdk/blob/develop/packages/ping-protect/package.json#L14

This is my reasoning/thought process so let me know if this tracks with you.

Tree shaking is the process of dead code removal. This is a static process, so bundlers are doing this based on the static code available during the uglification/minify process.

The signals SDK is not a module. It's just like an iiffe that is executed when the script loads. Additionally, it modifies the window with new functions that it uses. These modify the global scope (window) which are side effects.

So we need to tell bundlers that this file is effectful. It can't be removed. We rely on the effects produced by this in other code that.

So here we can use an array to say the files we are asking not be removed.

Another common input in sideEffect arrays would be CSS files (in a glob syntax) because CSS also changes the dom.

Another one would be a polyfill that extends the Array prototype.

Now - truth be told, I'm not sure the code style of factory functions is very tree shakeable to begin with because the bundler can't execute the factory to determine if some of the code is used or not later on. So it must include it.

But I could be wrong, or maybe things advanced and I'm not sure, so its worth including it anyways.

ryanbas21 · 2025-05-09T14:44:29Z

packages/protect/README.md

@@ -0,0 +1,11 @@
+# protect


We should also update this to be a bit more descriptive. We may be able to copy over the existing readme

https://github.com/ForgeRock/forgerock-javascript-sdk/blob/develop/packages/ping-protect/README.md

and slightly modify it.

ancheetah marked this pull request as draft May 7, 2025 22:53

ancheetah force-pushed the SDKS-3941-ping-protect branch from 6af5770 to 579c6ba Compare May 7, 2025 22:57

ancheetah requested review from ryanbas21 and cerebrl May 7, 2025 23:01

ancheetah force-pushed the SDKS-3941-ping-protect branch from 579c6ba to 6e4b133 Compare May 8, 2025 14:43

ancheetah marked this pull request as ready for review May 8, 2025 14:44

ancheetah force-pushed the SDKS-3941-ping-protect branch from 6e4b133 to 24a1fed Compare May 8, 2025 14:52

cerebrl approved these changes May 8, 2025

View reviewed changes

ancheetah commented May 8, 2025

View reviewed changes

ryanbas21 requested changes May 8, 2025

View reviewed changes

feat(protect): add protect package

8623e44

ancheetah force-pushed the SDKS-3941-ping-protect branch from 24a1fed to 8623e44 Compare May 8, 2025 19:19

ancheetah requested review from ryanbas21 and cerebrl May 8, 2025 19:23

ryanbas21 reviewed May 9, 2025

View reviewed changes

chore(protect): try to fix signals build

f3d68ea

ancheetah marked this pull request as draft May 9, 2025 15:25

chore(protect): update JS doc comments and readme

119cbb2

ancheetah marked this pull request as ready for review May 9, 2025 16:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SDKS-3941: Migrate Protect from Legacy to new Ping SDK #274

SDKS-3941: Migrate Protect from Legacy to new Ping SDK #274

ancheetah commented May 7, 2025

changeset-bot bot commented May 7, 2025 •

edited

Loading

nx-cloud bot commented May 7, 2025 •

edited

Loading

github-actions bot commented May 7, 2025 •

edited

Loading

codecov-commenter commented May 7, 2025 •

edited

Loading

cerebrl left a comment

ancheetah May 8, 2025

ryanbas21 May 8, 2025

cerebrl May 8, 2025

ryanbas21 May 8, 2025

ryanbas21 May 8, 2025

ryanbas21 May 8, 2025

ancheetah May 8, 2025

ancheetah May 8, 2025

ryanbas21 May 8, 2025

ryanbas21 May 8, 2025

ancheetah May 8, 2025

ancheetah commented May 8, 2025

ryanbas21 May 9, 2025

ryanbas21 May 9, 2025

SDKS-3941: Migrate Protect from Legacy to new Ping SDK #274

Are you sure you want to change the base?

SDKS-3941: Migrate Protect from Legacy to new Ping SDK #274

Conversation

ancheetah commented May 7, 2025

JIRA Ticket

Description

changeset-bot bot commented May 7, 2025 • edited Loading

⚠️ No Changeset found

nx-cloud bot commented May 7, 2025 • edited Loading

github-actions bot commented May 7, 2025 • edited Loading

codecov-commenter commented May 7, 2025 • edited Loading

Codecov Report

cerebrl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ancheetah commented May 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented May 7, 2025 •

edited

Loading

nx-cloud bot commented May 7, 2025 •

edited

Loading

github-actions bot commented May 7, 2025 •

edited

Loading

codecov-commenter commented May 7, 2025 •

edited

Loading