feat(davinci-client): add label and validated password fields#94
Conversation
🦋 Changeset detectedLatest commit: 34f4140 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 34f4140.
☁️ Nx Cloud last updated this comment at |
| process.exit(0) | ||
| } | ||
| const husky = (await import('husky')).default | ||
| console.log(husky.install()) |
There was a problem hiding this comment.
I don't think we want to delete this,, right? I believe the fix is just to invoke husky() like that.
I have that fix in some other PR currently open.
There was a problem hiding this comment.
Ah, okay, I can put this back. I think it was just the console.log that caused the lint failure.
| /** | ||
| * @method validate - Method for validating the value against validation rules | ||
| * @param {SingleValueCollector} collector - the collector to validate | ||
| * @returns {function} - an function to call for validating collector value |
| console.error('Argument for `collector` has no ID'); | ||
| return function () { | ||
| return { | ||
| error: { message: 'Argument for `collector` has no ID', type: 'argument_error' }, |
There was a problem hiding this comment.
Note: Because we seem to be following this shape elsewhere, I wonder if we should create a function to make this error so we are consistent about it everywhere?
something like
makeError(message: string, type: union|of|error|types) => ShapeOfError
There was a problem hiding this comment.
Yeah, we can definitely do this. Not sure I want to do it now, but I think reducing redundancy in this project is going to be important.
| interface ValidationRequired { | ||
| type: 'required'; | ||
| message: string; | ||
| rule: boolean; | ||
| } | ||
|
|
||
| interface ValidationRegex { | ||
| type: 'regex'; | ||
| message: string; | ||
| rule: string; | ||
| } | ||
|
|
There was a problem hiding this comment.
Not worth changing at this point, but if this is something that will "expand" as more validation types are built, This looks like a good use case for a discriminated union.
Since it's already used like that here: https://github.com/ForgeRock/ping-javascript-sdk/pull/94/files#diff-c9b63f466f9bc9b03f3122bc3f5d75531a0adb0ba81e42bec6d35ab659df1fbaR60
Again - no change at this point, but maybe worth while if we are back here again to add more validation "types"
There was a problem hiding this comment.
This is a good idea. I do have to say I'm not a huge fan of how much repetition these types have with each other, but that could be a good conversation in the near future.
| it('handles missing authentication URL for social login', () => { | ||
| const result = returnActionCollector(mockField, 1, 'SocialLoginCollector'); | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore |
There was a problem hiding this comment.
Why do we want ts-ignore here?
| const field = {}; | ||
| const idx = 3; | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore |
There was a problem hiding this comment.
Because it's a type error if removed. Since we are essentially checking runtime issues that TS would prevent.
| const field = {}; | ||
| const idx = 3; | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore |
| */ | ||
| export function returnActionCollector<CollectorType extends ActionCollectorTypes>( | ||
| field: StandardFieldValue, | ||
| field: RedirectFieldValue | StandardFieldValue, |
There was a problem hiding this comment.
FWIW - I think this type actually fits the StandardFieldValue type because StandardFieldValue does have optional links.
Worth adding this to the StandardFieldValue union?
There was a problem hiding this comment.
nevermind, I see the change below.
| }; | ||
| } | ||
|
|
||
| return returnValidator(collectorToUpdate as ValidatedTextCollector); |
There was a problem hiding this comment.
This seems like a code smell. We should have better inference at this point, but checking this out locally typescript says we have
(property) type: "PasswordCollector" | "SingleSelectCollector" | "SingleValueCollector" | "TextCollector"
So we must be missing something somewhere since ValidatedTextCollector isnt even in the list of possibilities but we are forcing it here
There was a problem hiding this comment.
Maybe this type needs to be added to the SingleValueCollector type?
There was a problem hiding this comment.
Oh, okay, I'll check that out.
cerebrl
force-pushed
the
feat_label-validated-password-support
branch
5 times, most recently
from
ac66297 to
080554a
Compare
cerebrl
force-pushed
the
feat_label-validated-password-support
branch
from
080554a to
34f4140
Compare
|
Deployed 87f2215 to https://ForgeRock.github.io/ping-javascript-sdk/pr-94/87f2215676c47c7deca776646f4b3ab41929b687 branch gh-pages in ForgeRock/ping-javascript-sdk |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #94 +/- ##
==========================================
+ Coverage 47.12% 54.17% +7.05%
==========================================
Files 29 20 -9
Lines 1129 1102 -27
Branches 137 145 +8
==========================================
+ Hits 532 597 +65
+ Misses 597 505 -92
🚀 New features to boost your workflow:
|
3 participants
JIRA Ticket
Jira ticket
Description
This adds support for the password validate and the label field component.