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.

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

[ui-core]: feat: add useLoadData hook for API calls #3819

Merged
merged 16 commits into from
Aug 20, 2024

Conversation

ramprasadagarwal
Copy link
Collaborator

@ramprasadagarwal ramprasadagarwal commented Aug 14, 2024

What changes were proposed in this pull request?

  • This PR adds a useLoadData react hook which exposes the data, loading, error, and refetch to the react component.

How was this patch tested?

  • Added the unit test for the hook that tests all the logic of the hook

Please review Hue Contributing Guide before opening a pull request.

@ramprasadagarwal ramprasadagarwal changed the title [ui-core]: feat: add useFetch hook for API calls [ui-core]: feat: add useLoadData hook for API calls Aug 16, 2024
const response = await get<T, U>(fetchUrl, localOptions?.params, fetchOptions);
setData(response);
} catch (error) {
setError(error);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Make sure that we're setting an Error object. As the type set is Error
setError(error instanceof Error ? error : new Error(error);

@ramprasadagarwal ramprasadagarwal enabled auto-merge (squash) August 19, 2024 17:02
Copy link
Collaborator

@bjornalm bjornalm left a comment

Choose a reason for hiding this comment

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

Nice work, clean and well written code! Just a couple of questions.

import { useCallback, useEffect, useMemo, useState } from 'react';
import { ApiFetchOptions, get } from '../../api/utils';

export type IOptions<T, U> = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the purpose of the I-prefix? If it is to indicate an interface, perhaps it would better to use an interface instead of type.

const [loading, setLoading] = useState<boolean>(false);
const [error, setError] = useState<Error | undefined>();

const fetchOptionsDefault: ApiFetchOptions<T> = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: this could be defined outside the component

reloadData: () => void;
};

const useLoadData = <T, U = unknown>(url?: string, options?: IOptions<T, U>): IUseLoadData<T> => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the use case for not passing in a url?

@ramprasadagarwal ramprasadagarwal merged commit 8c7a364 into master Aug 20, 2024
6 checks passed
@ramprasadagarwal ramprasadagarwal deleted the feat/useFetch-hook branch August 20, 2024 15:18
ramprasadagarwal added a commit that referenced this pull request Aug 20, 2024
* [ui-core]: feat: add useFetch hook for API calls

* add the license information

* fix lint

* lint fix

* add library

* pin the library version

* revert extra changes

* revert package-lock json

* fix: rename hook

* fix the function name

* typecaste error object
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants