Skip to content
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.

Sign up for GitHub

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

feat(utils): Add support for fetching remote JSON objects. #178

Merged
merged 11 commits into from
Feb 17, 2025
Merged

feat(utils): Add support for fetching remote JSON objects. #178

Changes from 8 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
8b955cb
refactor(http-utils): Refactor download progress handling.
junhaoliao Feb 7, 2025
56c6f94
Add missing empty line.
junhaoliao Feb 7, 2025
8f1ba6c
test(http-utils): Add unit tests for HTTP utilities.
junhaoliao Feb 7, 2025
5f2c317
Add missing changes in package-lock.json.
junhaoliao Feb 7, 2025
bff0feb
feat(http-utils): Add utility to fetch an object from remote JSON.
junhaoliao Feb 7, 2025
34d8496
Merge branch 'main' into http-util-json
junhaoliao Feb 14, 2025
48ca6d2
Remove tests.
junhaoliao Feb 14, 2025
22bfd34
Revert changes in package.json
junhaoliao Feb 14, 2025
7c29dd4
Refactor `getJsonObjectFrom` to use `JsonValue` type.
junhaoliao Feb 14, 2025
cf71755
Update docs.
junhaoliao Feb 14, 2025
ff6c68c
update docs.
junhaoliao Feb 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 27 additions & 2 deletions src/utils/http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,29 @@ const convertAxiosError = (e: AxiosError): Error => {
);
};

/**
* 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay we can leave it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it really parse? or does it just return a massive string?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyways I think your comment is fine, if it returns a js object

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I look at your later PR, it looks like it parses. I think your comment is fine

*
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
/**
* 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.
*

* @param remoteUrl
* @return The parsed JSON object.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* @return The parsed JSON object.
* @return JSON response.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the original docstring is more clear

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it returns a string, it is not an object. While JSON is more like json value

Copy link
Member Author

@junhaoliao junhaoliao Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure. how about:

@ return The parsed response.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@junhaoliao

* @throws {Error} if the download fails.
*/
const getJsonObjectFrom = async <T>(remoteUrl: string)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
    }
};

Copy link
Member Author

@junhaoliao junhaoliao Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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[];

Copy link
Contributor

@davemarco davemarco Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure, that works

: Promise<T> => {
try {
const {data} = await axios.get<T>(remoteUrl, {
responseType: "json",
});

return data;
} catch (e) {
throw (e instanceof AxiosError) ?
convertAxiosError(e) :
e;
}
};

/**
* Downloads (bypassing any caching) a file as a Uint8Array.
*
Expand Down Expand Up @@ -57,5 +80,7 @@ const getUint8ArrayFrom = async (fileUrl: string)
}
};


export {getUint8ArrayFrom};
export {
getJsonObjectFrom,
getUint8ArrayFrom,
};
Loading