feat(utils): Add support for fetching remote JSON objects.#178
feat(utils): Add support for fetching remote JSON objects.#178junhaoliao merged 11 commits intoy-scope:mainfrom
Conversation
WalkthroughThe pull request introduces an asynchronous function named Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant U as getJsonObjectFrom
participant A as Axios
C->>U: Call getJsonObjectFrom(remoteUrl)
U->>A: Execute HTTP GET request
A-->>U: Return response data
alt Response is Object
U->>C: Return parsed JSON object
else Response is not an object
U->>C: Return response as string
end
alt Request Error Occurs
U->>C: Throw error after converting with convertAxiosError
end
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
This PR depends on #177 |
junhaoliao
changed the title
# Conflicts: # src/utils/http.ts
As discussed offline with @davemarco , #177 is skipped for now. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/utils/http.ts (1)
41-42: Consider adding type constraint for better type safety.The generic type T is unconstrained, which could lead to runtime type mismatches if the JSON response doesn't match the expected type.
Consider adding a type constraint:
-const getJsonObjectFrom = async <T>(remoteUrl: string) +const getJsonObjectFrom = async <T extends object>(remoteUrl: string)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/http.ts(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,ts,tsx}`: - Prefer `false ==
**/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/utils/http.ts
**/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/utils/http.ts🔇 Additional comments (2)
src/utils/http.ts (2)
44-46: Consider adding caching control options.Unlike
getUint8ArrayFrom, this function doesn't specify any caching behaviour. Consider whether caching control should be consistent across both functions.Would you like me to propose an implementation that includes caching control options?
83-86: LGTM!The export statement is well-structured and follows TypeScript best practices.
| /** | ||
| * Retrieves an object that is stored as JSON in a remote location. | ||
| * If the response is not an object, the response is returned as a string. | ||
| * |
There was a problem hiding this comment.
Fix inconsistency in JSDoc documentation.
The JSDoc comment states "If the response is not an object, the response is returned as a string", but the function signature Promise<T> doesn't reflect this behaviour. The function will actually return whatever type the JSON response maps to.
Apply this diff to fix the documentation:
/**
* Retrieves an object that is stored as JSON in a remote location.
- * If the response is not an object, the response is returned as a string.
+ * The response is parsed as JSON and cast to type T.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Retrieves an object that is stored as JSON in a remote location. | |
| * If the response is not an object, the response is returned as a string. | |
| * | |
| /** | |
| * Retrieves an object that is stored as JSON in a remote location. | |
| * The response is parsed as JSON and cast to type T. | |
| * |
junhaoliao
changed the title
src/utils/http.ts
Outdated
| * @return The parsed JSON object. | ||
| * @throws {Error} if the download fails. | ||
| */ | ||
| const getJsonObjectFrom = async <T>(remoteUrl: string) |
There was a problem hiding this comment.
Did you try and replace type T, with import {JsonValue} from "../typings/js"
Something like. Did this not work?
const getJsonObjectFrom = async (remoteUrl: string)
: Promise<JsonValue> => {
try {
const {data} = await axios.get<JsonValue>(remoteUrl, {
responseType: "json",
});
return data;
} catch (e) {
throw (e instanceof AxiosError) ?
convertAxiosError(e) :
e;
}
};
There was a problem hiding this comment.
ah, this is to supply type information for the data to be returned. e.g., user can now write
const profileNames = await getJsonObjectFrom<string[]>("/profile-names.json");
instead of
const profileNames = await getJsonObjectFrom("/profile-names.json") as unknown as string[];
There was a problem hiding this comment.
I think it would be better as JsonValue. It restricts the type more appropriately than T. Also you dont really know if string[], until validated. I think you can write
const profileNames = await getJsonObjectFrom("/profile-names.json") as string[];
Or add a type predicate isStringArray() if you really want
src/utils/http.ts
Outdated
| * Retrieves an object that is stored as JSON in a remote location. | ||
| * If the response is not an object, the response is returned as a string. |
There was a problem hiding this comment.
| * Retrieves an object that is stored as JSON in a remote location. | |
| * If the response is not an object, the response is returned as a string. | |
| * Downloads JSON from remote url. |
There was a problem hiding this comment.
I do think it's worth mentioning a string would be returned for any non JSON response body, rather than anything thrown (especially now we don't have test cases to show the behavior). Shall we keep the last sentence?
There was a problem hiding this comment.
How about:
Downloads and parses JSON from remote URL. If the HTTP response body is not JSON, the body is gracefully returned as a string.
There was a problem hiding this comment.
Does it really parse? or does it just return a massive string?
There was a problem hiding this comment.
Anyways I think your comment is fine, if it returns a js object
There was a problem hiding this comment.
I look at your later PR, it looks like it parses. I think your comment is fine
src/utils/http.ts
Outdated
| * If the response is not an object, the response is returned as a string. | ||
| * | ||
| * @param remoteUrl | ||
| * @return The parsed JSON object. |
There was a problem hiding this comment.
| * @return The parsed JSON object. | |
| * @return JSON response. |
There was a problem hiding this comment.
I think the original docstring is more clear
There was a problem hiding this comment.
If it returns a string, it is not an object. While JSON is more like json value
There was a problem hiding this comment.
sure. how about:
@ return The parsed response.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/utils/http.ts (1)
43-56: Consider adding cache control headers for consistency.The function could benefit from the same cache control headers as getUint8ArrayFrom to maintain consistent behaviour across HTTP utilities.
const {data} = await axios.get<JsonValue>(remoteUrl, { responseType: "json", + headers: { + "Cache-Control": "no-cache", + "Pragma": "no-cache", + "Expires": "0", + }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/utils/http.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{cpp,hpp,java,js,jsx,ts,tsx}`: - Prefer `false ==
**/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/utils/http.ts
**/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
src/utils/http.ts🔇 Additional comments (3)
src/utils/http.ts (3)
3-3: LGTM! Import statement follows review discussion.The import of JsonValue type aligns with the previous review discussion about using a more specific type instead of a generic T.
35-42: LGTM! Documentation reflects review discussion consensus.The JSDoc documentation clearly describes the function's behaviour, including the handling of non-JSON responses.
85-88: LGTM! Export statement is well-organized.The export statement properly includes both functions in alphabetical order.
src/utils/http.ts
Outdated
| * Downloads and parses JSON from remote URL. | ||
| * If the HTTP response body is not JSON, the body is gracefully returned as a string. |
There was a problem hiding this comment.
I revisited our coding guidelines and believe this description should instead be placed in the return value description. let me update the PR.
davemarco
left a comment
davemarco
left a comment
There was a problem hiding this comment.
I left a small nit, but can also just fix on next PR
|
title: |
davemarco
left a comment
davemarco
left a comment
There was a problem hiding this comment.
approve see earlier title
junhaoliao
changed the title
2 participants
Description
2. Add corresponding tests.Checklist
breaking change.
Validation performed
1. Rannpm run testand observed all tests succeeded, and there was no coverage violation.Summary by CodeRabbit