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

Feat/#95 킬링파트 등록 & 노래 상세 정보 API 로직을 구현 #99

Merged
merged 15 commits into from
Jul 20, 2023

Conversation

Creative-Lee
Copy link
Collaborator

@Creative-Lee Creative-Lee commented Jul 20, 2023

📝작업 내용

  • fetch 추상화 유틸 구현
  • api 로직 구현
    • 킬링파트 등록 post
    • 노래상세정보 get
  • fetch 추상화 custom hook 구현
    • useFetch(get)
    • useMutation(post, patch, delete등 )
  • 컴포넌트에서 사용할 수 있는 개별 custom hook 구현
    • usePostKillingPart
    • useGetSongDetail

💬리뷰 참고사항

[fetch 유틸] --> [각 fetch함수] --> [useFetch / useMutaion hook] --> [use{METHOD}{DOMAIN} 형식의 hook] 

위 순서대로 단계별로 추상화 되어진 api flow입니다.

fetch 유틸 : 각 fetch함수에서 사용하는 fetch API에서 공통되는 부분을 추상화한 유틸입니다.
useFetch / useMutaion : 사용처(컴포넌트)에서 필요한 통신 관련 공통 state(data, loading, error)를 제공하는 hook입니다.

앞으로 추가될 수많은 api 로직을 생각하여 각 로직을 비슷한 레벨로 관리할 수 있도록 신경써봤습니다.
프로젝트 내, 첫 api 로직이라 기준을 잘세우려고 노력했어요.
피드백 많이 부탁드립니다!
다들 화이팅입니다!

#️⃣연관된 이슈

close #95

@Creative-Lee Creative-Lee self-assigned this Jul 20, 2023
@Creative-Lee Creative-Lee added [ 🌞 FE ] 프론트엔드 크루들의 빛나는 개발 이야기 하나둘셋 호! ✨ Feat 꼼꼼한 기능 구현 중요하죠 labels Jul 20, 2023

const useFetch = <T>(fetcher: () => Promise<T>) => {
const [data, setData] = useState<T | null>(null);
const [isLoading, setIsLoading] = useState(false);
Copy link
Collaborator

@ukkodeveloper ukkodeveloper Jul 24, 2023

Choose a reason for hiding this comment

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

처음 isLoading 상태가 false인 이유가 있을까요?

Copy link
Collaborator

@cruelladevil cruelladevil left a comment

Choose a reason for hiding this comment

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

고생많으셨습니다👍👍


const BASE_URL = `http://43.202.41.118`;

const fetcher = async <T>(url: string, method: string, body?: unknown): Promise<T> => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 method가 string보다는 union이 들어가면 더 좋을 것 같은데 어떻게 생각해요? ("GET" | "POST" | ...)

Comment on lines +8 to +17
const options: RequestInit = {
method,
headers: {
'Content-type': 'application/json',
},
};

if (body) {
options.body = JSON.stringify(body);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 타입스크립트에서는 동적으로 속성을 추가하기 보다 한 번에 객체를 선언하는 것이 타입 추론에 좋습니다. example

options에 RequestInit이라고 타입을 명시해주어서 추론과는 크게 상관 없지만
동적으로 추가하기 보다 객체를 한 번에 선언해주면 보다 선언적으로 코드를 관리할 수 있을 것 같아요.

Request body에는 ReadableStream이나 null이 들어갈 수 있습니다.

const options: RequestInit = {
  method,
  headers: {
    'Content-type': 'application/json',
  },
  body: body !== undefined ? body : null,
};

Comment on lines +14 to +21
type KillingPart = {
exist: true;
rank: 1;
start: 5;
end: 15;
partVideoUrl: PartVideoUrl;
voteCount: number;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

KillingPart 타입은 rank가 1, start가 5, end가 15로 고정인가요?!

import useFetch from './@common/useFetch';
import type { SongDetail } from '@/types/song';

export const useGetSongDetail = (songId: number) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 useSongDetail 또는 getSongDetail이 더 이해하기 쉬울 것 같습니다.

isLoading,
error,
fetchData,
} = useFetch<SongDetail>(() => getSongDetail(songId));
Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 useSongDetail 또는 getSongDetail이 더 이해하기 쉬울 것 같습니다.

💬 위 코멘트에 이어서,
getSongDetail이라는 함수가 굳이 나뉘었어야 되나 싶은 느낌입니다.

const {
  data: songDetail,
  isLoading,
  error,
  fetchData,
} = useFetch<SongDetail>(async () => await fetcher<SongDetail>(`/songs/${songId}`, 'GET'));

Comment on lines +10 to +11
setError(null);
setIsLoading(true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

동일한 의견입니다.
useEffect 훅으로 인해 useFetch가 호출될 때 fetchData 함수가 실행됩니다.
fetchData 함수를 실행하면 isLoading을 true로 바꿉니다.
굳이 시작 상태를 false로 두는 이유가 있나요?

💬 위 코멘트에 이어서,
error의 시작 상태는 null인데 null로 set해주는 이유가 있을까요?

setError(error as ErrorResponse);
} finally {
setIsLoading(false);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

👍 finally

} finally {
setIsLoading(false);
}
}, []);
Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 fetchData의 동일 참조를 유지하기 위한 목적으로 useCallback을 사용하였다면
dependency로 fetcher가 들어가지 않을까요?

💬 그와 별개로 동일 참조를 유지해준 이유가 궁금합니다.


const BASE_URL = `http://43.202.41.118`;

const fetcher = async <T>(url: string, method: string, body?: unknown): Promise<T> => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

body에 줄 수 있는 타입이 있습니다.

image

더불어 fetch의 interface를 따라가면서 RequestInit을 활용하는 것도 좋은 것 같습니다.

const fetcher = async <T>(url: string, { method, body }: RequestInit): Promise<T> => { ... };

Copy link
Collaborator

Choose a reason for hiding this comment

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

💬 여기서 더해서 목적이 기존 fetch에서 공통 option을 추가해주는 것이라면
fetcher도 option을 method와 body로 한정짓지 않고 열어주는 것이 좋을 것 같습니다.

const fetcher = async <T>(url: string, init: RequestInit): Promise<T> => { 
  const options = {
    ...init,
    headers: {
      'Content-type': 'application/json',
    },
    body: init?.body !== undefined ? JSON.stringify(init?.body) : null,
  };
  ...
};

message: string;
}

const BASE_URL = `http://43.202.41.118`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

환경변수로 빠져야 될 것 같아요

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[ 🌞 FE ] 프론트엔드 크루들의 빛나는 개발 이야기 하나둘셋 호! ✨ Feat 꼼꼼한 기능 구현 중요하죠
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants