-
Notifications
You must be signed in to change notification settings - Fork 47
feat: add-device-client #530
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
adds device client to the SDK package
🦋 Changeset detectedLatest commit: 5d68b9f The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 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 5d68b9f.
☁️ Nx Cloud last updated this comment at |
| "baseBranch": "master", | ||
| "updateInternalDependencies": "patch", | ||
| "ignore": [ | ||
| "@forgerock/device-client", |
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.
package no longer exists.
| "type": "module", | ||
| "dependencies": { | ||
| "@forgerock/javascript-sdk": "workspace:*", | ||
| "@forgerock/device-client": "workspace:*", |
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.
no longer exists
| // @ts-nocheck | ||
| import * as forgerock from '@forgerock/javascript-sdk'; | ||
| import { deviceClient } from '@forgerock/device-client'; | ||
| import { deviceClient } from '@forgerock/javascript-sdk/device-client'; |
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.
added as both an export from . and from /device-client.
packages/javascript-sdk/src/index.ts
Outdated
| import ChoiceCallback from './fr-auth/callbacks/choice-callback'; | ||
| import ConfirmationCallback from './fr-auth/callbacks/confirmation-callback'; | ||
| import DeviceProfileCallback from './fr-auth/callbacks/device-profile-callback'; | ||
| import { deviceClient } from './device-client'; |
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.
added it because if i dont i have to add a new entry point in the vite config, not a big problem if we do not want it exported from here though.
| name: 'javascript-sdk', | ||
| formats: ['es', 'cjs'], | ||
| fileName: (format, fileName) => { | ||
| console.log(fileName); |
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.
will remove.
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.
It looks like this is still here.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #530 +/- ##
===========================================
+ Coverage 57.25% 58.91% +1.66%
===========================================
Files 123 123
Lines 26967 27295 +328
Branches 1717 1776 +59
===========================================
+ Hits 15439 16080 +641
+ Misses 11528 11215 -313 ☔ View full report in Codecov by Sentry. |
fix types, add tests, fix tests, fix manual e2e
cerebrl
left a comment
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.
Mostly some naming fixes needed.
| const un = url.searchParams.get('un') || 'spetrov'; | ||
| const platformHeader = url.searchParams.get('platformHeader') === 'true' ? true : false; | ||
| const pw = url.searchParams.get('pw') || 'Demo1234!'; | ||
| const pw = url.searchParams.get('pw') || '1111'; |
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.
Did we mean to commit this?
| deleteOathDevice: builder.mutation< | ||
| DeletedOAthDevice, | ||
| RetrieveOathQuery & { device: OathDevice } |
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 see this quite often throughout the codebase. Can we make sure all references to "oath" are either OATH, Oath or oath? Currently, I see OAth quite a bit, which I'd like to correct to Oath.
| query: ({ realm = realmPath, userId }) => | ||
| `/json/realms/${realm}/users/${userId}/devices/2fa/binding?_queryFilter=true`, | ||
| }), | ||
| updateBindingDeviceName: builder.mutation<Device, BindingDeviceQuery>({ |
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're also not consistent here. Can we make sure to use bound and not binding? So, it would be getBoundDevices, updateBoundDeviceName and deleteBoundDevice.
| body: device satisfies Device, | ||
| }), | ||
| }), | ||
| getDeviceProfile: builder.query<GeneralResponse<ProfileDevice[]>, GetProfileDevices>({ |
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.
Are we querying for a single profile here, or are we querying for all of the user's profiles. I would think all, so I'd expect the name to be plural: getDeviceProfiles.
| name: 'javascript-sdk', | ||
| formats: ['es', 'cjs'], | ||
| fileName: (format, fileName) => { | ||
| console.log(fileName); |
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.
It looks like this is still here.
| userId: string; | ||
| }; | ||
|
|
||
| export type OAthResponse = { |
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.
Another naming inconsistency.
| realm?: string; | ||
| } & Device; | ||
| }; | ||
| export type BindingDeviceQuery = GetBoundDevicesQuery & { device: Device }; |
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.
"Binding" instead of "Bound".
1fa8030 to
3f2cdfc
Compare
e2bc5c1 to
856b3fb
Compare
renamed all per comments to better fit naming schemes
856b3fb to
5d68b9f
Compare
JIRA Ticket
https://pingidentity.atlassian.net/browse/SDKS-3413
Description
adds device client to the SDK package