Repository navigation
Recognize davinci - #869
eugeniobet-ping wants to merge 10 commits into
Conversation
Add a PingOne server config entry used to exercise the Recognize connector in the davinci-app e2e app.
Add @forgerock/recognize to the davinci-app e2e dependencies, with the matching tsconfig project reference and pnpm-lock entry.
Map the PingOne Recognize connector's RECOGNIZE field to a new
RecognizeCollector (ObjectValueAutoCollector) that exposes the web SDK
configuration (action, serviceURL, customerName, username,
transactionData, webSDKOptions) in its output. The application sets
either { transactionJWT } or { error } on the collector via update(),
which is validated in resolveCollectorUpdateValue.
Add a recognize component that initializes the Recognize web SDK from the collector output config and updates the collector with the resulting transactionJWT or error, and wire it into main.ts.
|
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
|
View your CI Pipeline Execution ↗ for commit 7cf9e03
💡 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/recognize
@forgerock/sdk-types
@forgerock/sdk-utilities
@forgerock/iframe-manager
@forgerock/sdk-logger
@forgerock/sdk-oidc
@forgerock/sdk-request-middleware
@forgerock/storage
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #869 +/- ##
===========================================
+ Coverage 18.07% 96.29% +78.22%
===========================================
Files 155 1 -154
Lines 24398 81 -24317
Branches 1203 17 -1186
===========================================
- Hits 4410 78 -4332
+ Misses 19988 3 -19985 🚀 New features to boost your workflow:
|
|
Deployed 879b33d to https://ForgeRock.github.io/ping-javascript-sdk/pr-869/879b33d29197a42cf5e56f2e610e1563e2a6f181 branch gh-pages in ForgeRock/ping-javascript-sdk |
Interface Mapping Out of DateThe Drift reportTo fix, run: pnpm mapping:generateThen commit the updated |
📦 Bundle Size Analysis📦 Bundle Size Analysis🚨 Significant Changes🔻 @forgerock/journey-client - 0.0 KB (-95.4 KB, -100.0%) 📊 Minor Changes📈 @forgerock/recognize - 5606.3 KB (+0.7 KB) ➖ No Changes➖ @forgerock/protect - 149.5 KB 15 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
ancheetah
left a comment
ancheetah
left a comment
There was a problem hiding this comment.
Looking good so far. Some comments about how we can potentially improve the types.
| if (hasError === hasTransactionJWT) { | ||
| return err('Value argument must contain either a transactionJWT or an error property'); | ||
| } |
There was a problem hiding this comment.
Should we make this guard stronger? It's possible hasError is false and hasTransactionJWT is true but an empty string then it might be nice to error then as well. That said, these are unlikely so just a question of if we want the JS SDK to fail fast or let the Recognize SDK/DaVinci handle the error.
Edit: I updated the potential outcomes above, I think this is an edge case.
There was a problem hiding this comment.
I echo this. I think these two values are stand alone, in other words, they don't have to equal each other to have an error state right?
hasError and hasTransactionJWT, if a JWT is required we would probably want to error with that message more specifically unless there's a security implication to it (don't see there being one).
Am I on the right track with this?
There was a problem hiding this comment.
they're meant to be mutually exclusive, you either got an error or a transactionJWT. the equality check catches both cases: both present or neither
There was a problem hiding this comment.
as per discussion with @ryanbas21 and @ancheetah fb7d063
| export type RecognizeCollector = AutoCollector< | ||
| 'ObjectValueAutoCollector', | ||
| 'RecognizeCollector', | ||
| RecognizeInputValue | GenericError, |
There was a problem hiding this comment.
Does the Recognize SDK emit GenericErrors? If not, then I think we can remove GenericError here.
| export type RecognizeInputValue = | ||
| | { transactionJWT: string; error?: never } | ||
| | { error: RecognizeError; transactionJWT?: never }; |
There was a problem hiding this comment.
Instead of typing the input value in a way that includes the error, it might follow our patterns (see FIDO collectors) better if we simplify this to type RecognizeInputValue = { transactionJWT: string }. Then below when we define the collector we have better separation of success value vs error value:
export type RecognizeCollector = AutoCollector<
'ObjectValueAutoCollector',
'RecognizeCollector',
RecognizeInputValue | RecognizeError,
RecognizeOutputValue
>;
| .then((err) => { | ||
| if (err) { | ||
| console.error('[recognize] init error', err); | ||
| updater(err); |
There was a problem hiding this comment.
Did you mean to submit the collector to DaVinci after this update? If not, we can probably remove this updater and leave it up to the application to error. In a real world application either the app would handle the initialization error by rendering something to the user or the error would be sent to DaVinci to branch the flow off to an Error Message connector. Since this is a test app, just logging the error is sufficient too.
@eugeniobet-ping let's make sure we add a changeset for the given changes so we publish & update the changelog correctly. |
No description provided.