-
Notifications
You must be signed in to change notification settings - Fork 39
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
hook scaffolding #14340
hook scaffolding #14340
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
...t/src/hooks/api/organizations/UseDeleteOrganizationReceiver/UseDeleteOrganizationReceiver.ts
Outdated
Show resolved
Hide resolved
options, | ||
endpointConfig, | ||
}: StaticAuthorizedFetchProps) { | ||
if (options.segments && !endpointConfig) |
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.
Do we want to log any of these potential errors to Azure?
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.
At the moment i'm expecting our custom error boundary to handle emitting these to telemetry.
@@ -5,6 +5,7 @@ import { | |||
UserClaims, | |||
} from "@okta/okta-auth-js"; | |||
import { useOktaAuth } from "@okta/okta-react"; | |||
import axios, { AxiosError } from "axios"; |
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 understand this might be out of the scope of this PR, but we do use both fetch
and axios
in our project. Is it time that we hard cut to one or the other?
For example in: AdminOrgEdit.tsx AdminLastMileFailuresTable.tsx EditSenderSettings.tsx
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.
Yes. Axios is currently the pattern (for context, established even before me), which is why it is continuing to be used. Getting everything in the project moved to using these new hooks will naturally align the project in one direction.
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.
lgtm
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.
Small comment about commenting, but 👍
Quality Gate passedIssues Measures |
Fixes: #14341,#8366,#8379,#8378,#8375,#8373,#8371,#8368
This PR scaffolds the (hopefully) remaining hooks to migrate to react-query.
authorizedFetch
was folded into the Session Context and now allows manual urls in addition to the endpointconfig object.