-
Notifications
You must be signed in to change notification settings - Fork 3
refactor(oidc-client): move authorize requests into rtk query #378
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: 4d0ee71 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 4d0ee71
☁️ Nx Cloud last updated this comment at |
f4eec22
to
f127143
Compare
Micro.promise(async () => { | ||
const deleteResponse = await storageClient.remove(); | ||
const deleteRes = await storageClient.remove(); | ||
const deleteResponse = typeof deleteRes === 'undefined' ? null : deleteRes; |
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 wonder if this logic should live in the storage client and we should standardize the return type. Currently, if the customer is using typescript it's not possible for them to use a custom remove function that resolves to some value because remove
is typed a Promise<void>
. I propose we make the following change in the storage client:
Currently this is the type:
export interface StorageClient<Value> {
get: () => Promise<Value | GenericError | null>;
set: (value: Value) => Promise<void | {
code: string;
message: string;
type: string;
}>;
remove: () => Promise<void>;
}
Which can be changed to:
export interface StorageClient<Value, T = null> {
get: () => Promise<Value | GenericError | null>;
set: (value: Value) => Promise<GenericError | null>;
remove: () => Promise<T | null>;
}
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 change makes sense, but we'd have to plan it for 2.0.
edit: Actually I suppose this isn't actually breaking
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, I agree. Let's write up a quick story to handle that in the near future. I'd do it now, but I'm starting to get behind in my work :(
response.error = { | ||
status: responseError.status, | ||
statusText: 'AUTHORIZE_ERROR', | ||
data: { | ||
error: details.code, | ||
error_description: details.message, | ||
type: 'auth_error', | ||
}, | ||
} as FetchBaseQueryError; |
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 technically isn't a FetchBaseQueryError because statusText
is a an added property. For it to be a FetchBaseQueryError it would have to be something like:
response.error = {
status: responseError.status,
data: {
error: details.code,
error_description: details.message,
type: 'auth_error',
},
error: details.message,
};
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.
You are correct. I'm intentionally doing this for reasons that are not easily articulated in a comment. Let's discuss this when we discuss errors on our upcoming call.
response.error = { | ||
status: responseError.status, | ||
statusText: 'CONFIGURATION_ERROR', | ||
data: { | ||
error: 'FETCH_ERROR', | ||
error_description: | ||
'Configuration error. Please check your OAuth configuration, like clientId or allowed redirect URLs.', | ||
type: 'network_error', | ||
}, | ||
} as FetchBaseQueryError; |
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.
Same comment here. This isn't quite a FetchBaseQueryError. We might want to consider using status: 'CUSTOM_ERROR'
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.
Let's talk about this.
).pipe( | ||
Micro.flatMap( | ||
(response): Micro.Micro<AuthorizationSuccess, AuthorizationError, never> => { | ||
if ('error' in response) { |
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 handles FetchBaseQueryError but not SerializedError. I have an example of handling both in the device client as recommended from the RTK docs.
https://github.com/ForgeRock/ping-javascript-sdk/blob/main/packages/device-client/src/lib/device.store.utils.ts
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.
Okay, I didn't do exactly what you suggested, but I added some handling for it.
9cf39dc
to
bbfa76a
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #378 +/- ##
=======================================
Coverage 55.47% 55.47%
=======================================
Files 32 32
Lines 2044 2044
Branches 340 340
=======================================
Hits 1134 1134
Misses 910 910 🚀 New features to boost your workflow:
|
Deployed 193d2b0 to https://ForgeRock.github.io/ping-javascript-sdk/pr-378/193d2b0e55962c103c3cf3fa15bff8d0502cc660 branch gh-pages in ForgeRock/ping-javascript-sdk |
📦 Bundle Size Analysis📦 Bundle Size Analysis📊 Minor Changes📈 @forgerock/oidc-client - 22.3 KB (+1.0 KB) ➖ No Changes➖ @forgerock/davinci-client - 34.1 KB 11 packages analyzed • Baseline from latest Legend🆕 New package ℹ️ How bundle sizes are calculated
🔄 Updated automatically on each push to this PR |
bbfa76a
to
4d0ee71
Compare
JIRA Ticket
SDKS-4312
Description
Migrated the
fectch
and iframe requests to/authorize
to the OIDC API that uses RTK Query. This provides better logging and allows for middleware usage.