Refactor(client): use-image-upload 로직 추상화#502
Conversation
|
✅ Storybook이 배포되었습니다. |
jeonghoon11
left a comment
There was a problem hiding this comment.
우와... 추상화 너무 좋은데요?? 깔끔하네요!! 역시... 곽버지..
궁금한점 몇가지 남겼습니다!!
| try { | ||
| return await uploadImageFilesInternal(files); | ||
| } catch (e) { | ||
| setError(true); |
There was a problem hiding this comment.
에러가 발생했을 때 setError 상태가 변경되지만 이후 에러에 대한 대응은 없어보이는것 같아요! 그래서 이미지 업로드 실 패 시 토스트 메시지를 띄우는 방안도 괜찮아 보이네요!
There was a problem hiding this comment.
toast 메시지와 같은 에러 핸들링을 하기 위해서 상태를 설정하고 그걸 return 하고있는거에용 여기선 상태만 선언하고 이 함수에서는 에러 상태만 반환해요
그래야 사용처별로 달라지는 ux ex) 토스트 메시지 등등을 대응할 수 있어요
loading 값도 마찬가지 이유로 반환하는거구용
There was a problem hiding this comment.
오 훅에서 상태만 보고 사용처에서 UX를 결정하는 의도 확인했습니당
다만 기존에도 이 pr에서도 detail-section 같은 호출부에서 에러 대응 로직이 없다 보니, heic 파일 업로드 실패나 서버 에러 시 사용자한테 알림이 안 가고 있더라고요.
이 UI 처리는 이번 PR에서 같이 수정하실 예정인지 아니면 별도 브랜치에서 되어야 하는 부분인지 궁금합니다 !
There was a problem hiding this comment.
toast 메시지와 같은 에러 핸들링을 하기 위해서 상태를 설정하고 그걸 return 하고있는거에용 여기선 상태만 선언하고 이 함수에서는 에러 상태만 반환해요
그래야 사용처별로 달라지는 ux ex) 토스트 메시지 등등을 대응할 수 있어요
이해했습니다. 에러에 따른 UI는 사용처에서 관리하도록 구성하신거군요 👍
There was a problem hiding this comment.
오 훅에서 상태만 보고 사용처에서 UX를 결정하는 의도 확인했습니당
다만 기존에도 이 pr에서도 detail-section 같은 호출부에서 에러 대응 로직이 없다 보니, heic 파일 업로드 실패나 서버 에러 시 사용자한테 알림이 안 가고 있더라고요.
이 UI 처리는 이번 PR에서 같이 수정하실 예정인지 아니면 별도 브랜치에서 되어야 하는 부분인지 궁금합니다 !
@hansoojeongsj 에러처리에 관련된건 별도의 스코프에서 따로 처리하려고해요 ㅎㅎ 센트리 도입부터 시작해서 한번에 진행하려구용
| .cursorignore | ||
|
|
||
| #codex | ||
| AGENTS.md No newline at end of file |
There was a problem hiding this comment.
지욱님의 AGENTS.md 탐나네요.. 공유해주세요~
| }, | ||
| }); | ||
|
|
||
| const { mutate: postImageUploadMutate } = useMutation({ |
There was a problem hiding this comment.
mutation에 대한 코드를 삭제하셨으니 queries.ts에서 MUTATION_QUERY_OPTIONS.POST_IMAGE()도 같이 삭제해도 괜찮을것 같아요!
그런데 여기서 궁금한게 있어요
mutation을 써서 Pending상태일 때 로딩 로띠나 에러처리 등을 할 수 있는데 제거하신 이유가 궁금하네요! JustWonder
There was a problem hiding this comment.
뮤테이션을 제거하지 않은 이유는 인프라 레이어라서 제거하지 않았던건데 옵션 블록은 제거하거나 주석처리하거나 하는게 좋겠네요 일단 제거해둘게요 👍
mutation을 써서 Pending상태일 때 로딩 로띠나 에러처리 등을 할 수 있는데 제거하신 이유가 궁금하네요! JustWonder
useImageUpload 내부에도 로딩 및 에러 처리를 위한 상태 반환 값이 존재해서 우려하는 부분은 대체 될 것 같아요
export const useImageUpload = () => {
const [isUploading, setIsUploading] = useState(false);
const [isError, setError] = useState(false);
const uploadImageFiles = async (files: File[]) => {
setIsUploading(true);
setError(false);
try {
return await uploadImageFilesInternal(files);
} catch (e) {
setError(true);
throw e;
} finally {
setIsUploading(false);
}
};
return { uploadImageFiles, isUploading, isError };
};There was a problem hiding this comment.
아 이해했어요 isUploading이 로딩 상태를 관리하는 state였군요... 설명 감사합니다
| try { | ||
| return await uploadImageFilesInternal(files); | ||
| } catch (e) { | ||
| setError(true); |
There was a problem hiding this comment.
toast 메시지와 같은 에러 핸들링을 하기 위해서 상태를 설정하고 그걸 return 하고있는거에용 여기선 상태만 선언하고 이 함수에서는 에러 상태만 반환해요
그래야 사용처별로 달라지는 ux ex) 토스트 메시지 등등을 대응할 수 있어요
이해했습니다. 에러에 따른 UI는 사용처에서 관리하도록 구성하신거군요 👍
| }, | ||
| }); | ||
|
|
||
| const { mutate: postImageUploadMutate } = useMutation({ |
There was a problem hiding this comment.
아 이해했어요 isUploading이 로딩 상태를 관리하는 state였군요... 설명 감사합니다
📌 Summary
📚 Tasks
👀 To Reviewer
이미지 업로드 중복 로직을 share hook 으로 분리했어요 컴포넌트 레벨에서의 중복 로직을 제거하고, 각 파일에서 필요없는 직접 사용 로직을 제거했어요