chore(calm-hub-ui): migrate from fetch to axios#2137
chore(calm-hub-ui): migrate from fetch to axios#2137selwyntheo wants to merge 6 commits intofinos:mainfrom
Conversation
Replace native fetch calls with axios in calm-service and authService for better testability via dependency injection. Convert calm-service from standalone functions to a CalmService class following the existing AdrService pattern, and add comprehensive tests for both services.
selwyntheo
force-pushed
the
chore/issue-979-migrate-fetch-to-axios
branch
from
a23c7a3 to
71323fd
Compare
|
@selwyntheo thanks for the contribution! We'll take a look over this soon as we have some other PRs pending or coming up for calm-hub-ui. FYI @aamanrebello |
|
@markscott-ms - Thanks much!. Let me know how I can help on other issues. |
|
@selwyntheo if able, come along to an Office Hours meeting, every Thursday 1530 UK / 1030 EST. See https://github.com/finos/architecture-as-code?tab=readme-ov-file#getting-involved for details and #2136 for this week's - where we intend to agree what's next from #2075 as a focus area. If unable, you can always drop a comment on an office hours issue letting us know that you might pick up any issue from the backlog. It's definitely worth commenting before picking up anything just in case someone else is working on it or about to introduce a conflicting change. We will usually get back to any comments promptly. |
|
Thanks @markscott-ms . Will definitely try to attend office hours. Appreciate it |
…te-fetch-to-axios # Conflicts: # calm-hub-ui/src/service/calm-service.tsx
aamanrebello
left a comment
aamanrebello
left a comment
There was a problem hiding this comment.
The only change I am actually requesting is to add public access modifiers in the CalmService class. Apart from this I had a few questions. I am sorry again for taking so long to review this PR.
There was a problem hiding this comment.
Hi Selwyn, sorry for taking so long to review this. Is it possible to fix the linting error in this file (around ReachHook having a missing dependency)? I'd rather we did not let this stuff creep in. If not possible, please explain why before I approve.
There was a problem hiding this comment.
@aamanrebello Thanks for reviewing this. I will add the lint error and push the changes requested here.
| } | ||
|
|
||
| export async function getAuthHeaders(): Promise<HeadersInit> { | ||
| export async function getAuthHeaders(): Promise<Record<string, string>> { |
There was a problem hiding this comment.
Can you explain why the type has been changed here? The new type is less restrictive, offering less type safety.
There was a problem hiding this comment.
Record<string, string> is more type-safe, not less is my view. It's a specific subset of HeadersInit that precisely describes what the function returns (an object with string keys/values), rather than a union of three possible types. This provides better type safety for consumers. But let me know your thoughts.
There was a problem hiding this comment.
If this is the case, then could you create a type for the auth header response and for now set it to be Record<string, string e.g. export type AuthHeaders = Record<string, string>
| const resourceId = '1'; | ||
| const version = '1.0.0'; | ||
|
|
||
| const errorStatusCodes = [400, 401, 403, 404, 500]; |
There was a problem hiding this comment.
Why specifically these codes alone?
There was a problem hiding this comment.
These cover the most common HTTP error scenarios: But we can generalize
There was a problem hiding this comment.
If we want to cover different HTTP error scenarios, we should either cover all of them or cover just one and say that it can be extended to any other error code. Using only the "most common" scenarios feels a little arbitrary.
Could you possibly enumerate all the error codes. Or just use one error code.
There was a problem hiding this comment.
Thank you for creating these tests, is there any reason why you have only tested checkAuthorityService and not the other functions?
- Fix linting error in TreeNavigation by memoizing service instances with useMemo - Add public access modifiers to all CalmService methods for consistency - Expand authService tests to cover getToken, getAuthHeaders, and isAuthServiceEnabled - Add JSDoc comment documenting future type safety improvements for API responses Resolves PR review comments regarding: 1. React Hook useEffect missing dependency warning 2. Type safety explanation for HeadersInit -> Record<string, string> 3. Error status codes selection rationale 4. Limited test coverage in authService 5. Missing public modifiers on CalmService methods 6. Future type safety improvements for API responses
|
Second Aaman's comments. Awaiting resolution |
5 participants
Summary
calm-service.tsxfrom standalonefetch-based functions to aCalmServiceclass with injectableAxiosInstance, following the existingAdrServicepatternfetchcall inauthService.tsx(checkAuthorityService) withaxios.head()TreeNavigation.tsxand its tests to consume the new class-based APICalmService(20 tests) andcheckAuthorityService(2 tests)Closes #979
Test plan
npm testincalm-hub-ui/)npm run buildincalm-hub-ui/)fetch(calls incalm-hub-ui/src/service/orcalm-hub-ui/src/authService.tsx