-
Notifications
You must be signed in to change notification settings - Fork 2
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: implement custom fetch function #3
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
코멘트로 일부 구조에 대한 저의 생각을 남겼습니다. 확인 부탁드려요!
@@ -0,0 +1,12 @@ | |||
export class ApiError extends Error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
서버로부터 api 에러 발생시 errorCode(status code 말고)와 errorMessage가 response body에 담겨 온다고 전달받은 것 같은데, 이 에러 코드와 에러 메시지를 ApiError에 담는건 어떻게 생각하시나요?
과거 프로젝트에서 동일한 형식으로 서버로부터 에러 데이터를 받았는데, 이를 다루는 방식에 대한 글을 작성했었습니다. 한 번 확인해주시면 감사하겠습니다.
해당 글의 에러를 던지는 방법을, (에러 코드와 메시지가 있는)서버 의도 에러는 ApiError로 throw 했고, 서버에서 의도하지 않은 에러(500번대 에러)는 UnknwonApiError라는 커스텀 에러를 throw 했습니다. 이러한 방식으로 에러 처리를 하기 꽤 수월했는데 이 방식은 어떤지 제안해보고 싶습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋습니다!
추후 서버 쪽과 맞춰 수정해도 좋겠네요👍
) | ||
} | ||
|
||
const data = (await res.json()) as T |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
response에는 body 데이터 외에도 redirect url, header의 cookie 등 클라이언트에서 사용할 수 있는 다양한 데이터가 있는데, 이러한 데이터가 필요한 상황에는 fetchApi로 대응하기 힘들 것 같다고 생각이 들어요.
response를 통째로 반환을 해버리거나, 커스텀 Response 타입을 만들어서 반환하는 방식을 제안드리고 싶습니다. 혹시 이 부분은 제가 과거 fetch 추상화 경험이 있는데, 제가 추후에 맡아서 해봐도 괜찮을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
먼저 픽토스에 저정도의 공유해주신 정도의 기능들이 필요한지 생각해보면 좋을 것 같아요!
fetchAPI 외부에서 json이외의 반환 타입이 필요한가
개인적으로 픽토스에서는 필요성이 느껴지지 않습니다! 엣지케이스들에 대해 예를 들어주시면 훨씬 이해하기 좋을 것 같아요!
intercepter 등의 기능이 필요한가 등등
https://github.com/ruslan4432013/fsd-react-query-example/blob/main/src/shared/api/base.ts
현재 방식에서 handleResponse 함수를 통해 intercepter를 대신하거나, 위의 방식에서 next fetch의 request option을 추가하는 방향도 나쁘지 않을 것 같습니다
어떤 방향이든 추후에 정균님이 맡아서 진행해주는 건 너무나 좋습니다👍
다만 fetchAPI가 빠르게 완성 되어야 추후 작업을 진행할 수 있기 때문에, 확장성 및 유연성을 위한 것이라면 �개인적인 생각으로는 우선 순위가 높진 않을 것 같습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
넵 확인했습니다!
다만, 저는 인터셉터 등 다른 기능을 추가하고 싶은 것이 아닌, 반환 데이터에 대해서만 의견을 드리고 싶었습니다!
물론 현재 픽토스의 api에서는 body 데이터만으로도 충분히 처리할 수 있다고 생각합니다. 그러나 앞으로 여러 기능이 추가되면서 body 데이터 외의 다른 response 데이터가 필요한 경우가 생기면 큰 수정 작업이 생기지 않을까라는 생각에 코멘트를 남겼었습니다.
이전 프로젝트들에서 response가 필요했던 상황으로는, api의 특정 상황에 따라 페이지를 리다이렉트 해야하는 경우가 있을 때, response의 redirect 값을 활용하기도 했었고, header의 쿠키 값으로 넘어오는 엑세스 토큰을 사용해 특정 로그인 로직을 수행하는 경우도 있었어요.
하지만 이러한 부분들은 백엔드 개발자와 잘 협의하여 충분히 body 데이터로도 넘길 수 있긴할 것 같아요. 픽토스 개발하면서 이러한 부분에서 충돌나지 않도록 백엔드 개발자와 API 명세를 잘 협의해나가면 지금의 json 반환 방식으로도 잘 해나갈수 있을 것 같습니다.
아무튼 결론: response 객체 데이터를 사용하지 못하는 것이 추후 걸림돌이 되지 않을까 걱정되긴 하지만, 백엔드 개발자와 API 명세 협의 잘하면 크게 문제되지는 않을 것 같다. 그냥 이대로 사용해도 괜찮을 듯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
공유해주신 clientHttp를 보니 거의 axios를 재현해두셨던데.. 개인적으로 탐나긴합니다 ㅋㅋ
next fetch option들을 추가해서 앞으로도 쭉 사용하고 싶을 정도였으니 리펙토링 때 픽토스에 적용해봐도 너무 좋을 것 같습니다🤩
src/apis/fetch-api.ts
Outdated
|
||
const DEFAULT_REVALIDATE = 0 | ||
|
||
export const fetchAPI = async <T extends Response>(params: FetchAPIParams): Promise<T> => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
반환하는 데이터는 response body 타입(JSON 타입)인데, 제네릭 T는 Response 타입을 상속하고 있어서 서로 타입 형식이 안맞지 않나요?!
…o feat-custom-fetch
개요
custom fetch 함수 구현
세부 내용
관련 링크