-
Notifications
You must be signed in to change notification settings - Fork 3
SDKS-4202: Update Signals SDK to v5.6.0 #362
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🦋 Changeset detectedLatest commit: 4f19711 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 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 18677d9
☁️ Nx Cloud last updated this comment at |
Deployed 3b5d297 to https://ForgeRock.github.io/ping-javascript-sdk/pr-362/3b5d29766977dd2f4d90486bce21b405efa97858 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis🆕 New Packages🆕 @forgerock/protect - 152.3 KB (new) ➖ No Changes➖ @forgerock/oidc-client - 17.9 KB 11 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
3163ccd
to
a7e22ea
Compare
Pending verification of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ancheetah Can we just get a confirmation that we do not intend to release the signals SDK here?
This would be considered a major change i'd think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few questions and suggestions.
packages/protect/README.md
Outdated
# PingOne Protect | ||
|
||
The Ping Protect module is intended to be used along with the ForgeRock JavaScript SDK to provide the Ping Protect feature. | ||
The PingOne Protect module provides an API for interacting with the PingOne Signals (Protect) SDK to perform risk evaluations. It can be used with either a PingOne AIC/PingAM authentication journey with Protect callbacks or with a PingOne DaVinci flow with Protect connectors. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To keep things symmetric, I'd use Nodes and Connectors, or use Callbacks and Collectors. In the above sentence your using terms from different categories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I'll change this to callbacks and collectors here. I do use the term connectors
in the DaVinci section but I think it is more clear there the relationship between collectors and connectors.
Alternatively, you can delay the initialization until you receive the instruction from the server by way of the `ProtectCollector`. To do this, you would call the `start` method when the collector is present in the flow. The Protect collector is returned from the server when it is configured with either a PingOne Forms connector or HTTP connector with Custom HTML Template. |
packages/protect/README.md
Outdated
```js | ||
// PingOneProtectInitializeCallback methods | ||
callback.getConfig(); | ||
callback.setClientError(); | ||
``` | ||
|
||
```js | ||
// PingOneProtectEvaluationCallback methods | ||
callback.setData(); | ||
callback.setClientError(); | ||
callback.getPauseBehavioralData(); | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This may be a bit confusing for readers. This example block of APIs are really about this module but about the callback methods from the legacy SDK. We should probably preface this with a bit of explanation and also distinguish it from DaVinci Collectors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about removing this from the Full API section and moving it into a section at the bottom of the AIC quickstart with the title Protect Callback Methods
. It would have some description like Relevant callback methods for PingOneProtectInitializeCallback and PingOneProtectEvaluationCallback
.
We could even just remove these lines altogether since they're already covered in the quickstart.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we already cover it in a different section, let's go with your second statement.
packages/protect/README.md
Outdated
``` | ||
|
||
The `@pingidentity/protect` module has a `protect()` function that accepts configuration options and returns a set of methods for interacting with Protect. The two main responsibilities of the Ping Protect module are the initialization of the profiling and data collection and the completion and preparation of the collected data for the server. You can find these two methods on the API returned by `protect()`. | ||
The `@pingidentity/protect` module has a `protect()` function that accepts configuration options and returns a set of methods for interacting with Protect. The two main responsibilities of the PingOne Protect module are the initialization of the profiling and data collection and the completion and preparation of the collected data for the server. You can find these two methods on the API returned by `protect()`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the reason for converting Ping
to PingOne
? I also noticed the one above. PingOne specifically refers to the multi-tenant, server product, of which this module doesn't directly relate to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, didn't know this. I can change it back everywhere. I updated it to PingOne because that is how I saw Protect being referred to everywhere in the docs. But I suppose the module isn't necessarily the same thing.
https://docs.pingidentity.com/sdks/latest/sdks/integrations/integrate-with-pingone-protect.html
https://docs.pingidentity.com/connectors/p1_protect_connector.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, in the context of building a DaVinci flow, the Protect feature is configured and comes from the PingOne server and implemented wit that connector. But, from an SDK standpoint, PingOne itself is never directly used, and all interaction is with DaVinci.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed it back to just 'Ping' everywhere where it should be. Let me know if I missed one.
packages/protect/README.md
Outdated
let data; | ||
|
||
if (step.getCallbacksOfType('PingOneProtectEvaluationCallback')) { | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to prevent the wrapping of these methods with a try-catch. I wonder if we should at least have an internal try-catch to prevent this burden on the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add an internal try-catch to each method in our module to ensure they only return an error object instead of throwing. This does simplify the error handling section as well since we would be able to remove the try catches there too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this would be good as it would help align this module more with our other modules where "throwing errors" is completely avoided.
packages/protect/README.md
Outdated
// Asynchronous call | ||
data = await protectAPI.getData(); | ||
data = await protectApi.getData(); | ||
if (typeof data !== 'string' && 'error' in data) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The convention I have in my head for recommended patterns is something like this (which you're very close to):
const result = api.method();
if ('error' in result) {
// handle error and return early
}
// handle success
My intent is for all errors to have an error
property, so this can be relied on across all of our modules. If this is present, they have an error, if not, they have a success.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do this for most of the methods except for getData
because the return type is a string instead of void. If you remove the typeof data !== 'string'
check then you'll get a typescript error:
Type 'string | { error: unknown; }' is not assignable to type 'object'.
Type 'string' is not assignable to type 'object'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, yeah, makes sense. I wonder if it's worth it to have a discussion around functions that return a primitive but also return errors. That would prevent the need to handle two different kinds of data types.
|
||
When calling `protect()`, you have many different options to configure what and how the data is collected. The most important and required of these settings is the `envId`. All other settings are optional. | ||
|
||
The `start` method can be called at application startup, or when you receive the `ProtectCollector` from the server. We recommend you call `start` as soon as you can to collect as much data as possible for higher accuracy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to emphasize this a bit more than this paragraph. Can we use headers for the different approaches and place "(recommended)" in the header text for startup time approach?
474b08a
to
f1b1830
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #362 +/- ##
=======================================
Coverage 57.58% 57.58%
=======================================
Files 32 32
Lines 2044 2044
Branches 320 320
=======================================
Hits 1177 1177
Misses 867 867 🚀 New features to boost your workflow:
|
f1b1830
to
4f19711
Compare
4f19711
to
18677d9
Compare
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-4202
Description
@pingidentity/protect-app
@forgerock
Changeset added