[Feature] 토스트 컴포넌트 구현 및 에러핸들링 로직 수정 #108
Conversation
hamo-o
requested review from
ghdtjgus76,
eugene028 and
SeieunYoo
as code owners
| export const Toast = forwardRef( | ||
| ({ text, subText, type, ...rest }: ToastProps) => { | ||
| return ( | ||
| <Flex | ||
| className={toastContainerStyle} | ||
| direction="column" | ||
| justifyContent="center" | ||
| {...rest} | ||
| > | ||
| <Text color="textWhite" typo="body1"> | ||
| {text} | ||
| </Text> | ||
| <Text color="outline" typo="body2"> | ||
| {subText} | ||
| </Text> | ||
| </Flex> | ||
| ); | ||
| } | ||
| ); |
There was a problem hiding this comment.
p3;
type이 에러인 경우도 고려해야 할 거 같아요!
디자인 상에서는 error인 경우를 따로 정의해두신 거 같아요
There was a problem hiding this comment.
좌측은 아이콘을 직접 넣을 수 있는 형태로 되어있고, type이 state가 아니라 토스트 우측에 들어가는 아이콘을 말하는 거더라구요.
요 PR 리뷰 반영하고 머지되려면 조금 걸릴 것 같아서 현재 작업하고 있는 와디 토스트 완성 후 반영하려고 합니다!
| export { default as Text } from "./Text"; | ||
| export * from "./Toast"; |
There was a problem hiding this comment.
p4;
다른 컴포넌트들처럼 export { default as Toast } from "./Toast" 방식으로 통일하면 좋을 거 같습니다!
| const handlePublishAnnouncement = async (studyId: string) => { | ||
| const { success } = await studyApi.publishStudyAnnouncement( | ||
| parseInt(studyId, 10), | ||
| studyAnnouncement | ||
| ); | ||
| if (success) { | ||
| try { | ||
| await studyApi.publishStudyAnnouncement( | ||
| parseInt(studyId, 10), | ||
| studyAnnouncement | ||
| ); | ||
| toast({ type: "success", text: "공지 생성 성공" }); | ||
| revalidateTagByName(tags.announcements); | ||
| setStudyAnnouncement({ | ||
| title: "", | ||
| link: "", | ||
| }); | ||
| } else { | ||
| console.log("공지 생성 실패"); | ||
| } catch (error) { | ||
| if (error instanceof Error) { | ||
| toast({ type: "error", text: error.message }); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
p3;
해당 api는 fetcher 클래스를 사용하는 것 같은데 response interceptor를 사용해보면 공통으로 에러 메시지 같은 부분을 관리할 수 있을 거 같아요!
불가피하게 fetch를 쓰는 경우에만 위 경우처럼 처리해주면 될 거 같은데 fetcher를 안 쓰고 fetch를 쓰는 경우가 있나요?
| export const toastAtom = atom( | ||
| null, | ||
| (get, set) => | ||
| (props: Omit<ToastProps, "id"> & Partial<Pick<ToastProps, "id">>) => { | ||
| const prevAtom = get(toastsAtom); | ||
| const newToast = { | ||
| ...props, | ||
| id: props.id || Date.now().toString(), | ||
| }; | ||
|
|
||
| set(toastsAtom, [...prevAtom, newToast]); | ||
| } | ||
| ); |
There was a problem hiding this comment.
p3;
toastAtom보다는 addToastAtom이라는 네이밍이 적합해보입니다
There was a problem hiding this comment.
p3;
지금 ToastProvider에서는 map 함수를 통해 Toast 컴포넌트를 내려주는 일들만 하고, Toast 컴포넌트에서는 각각 timer를 통해서 각 Toast 컴포넌트만 관리하기 때문에 개별적으로 상태를 관리하고 있어요
이 부분에서 타이머 처리하는 부분이 효율적이지 않을 수 있을 거 같은데, ToastProvider에서 전체적인 타이머 상태를 관리하는게 어떨까요?
There was a problem hiding this comment.
그리고 아래와 같이 토스트를 전부 띄워주는게 아니라 아이디 또는 태그에 맞는 토스트만 찾아서 띄워주는 방식이 더 좋을 거 같다는 생각이 들어요
동시에 하나만 보여주면 되니...
그래서 아이디 (숫자 값 또는 date 값)를 사용하는 방식이 아니라 문자열 태그를 만들어보는 건 어떠신가요?
{toasts?.map((toast: ToastProps) => (
<Toast key={toast.id} {...toast} />
))}
| setIsSuccess(false); | ||
| window.alert(error.message); |
There was a problem hiding this comment.
p3;
이 부분도 window.alert를 toast로 변경해야 할 거 같네요
어드민에 기존에 사용되던 window.alert 부분을 변경해두면 좋을 거 같습니다!
There was a problem hiding this comment.
흠 생각해보니, try catch문을 각각의 비동기 호출문에 작성하기보다는 Errorboundary Provider를 만들어서 해당 프로바이더에서 componentDidCatch에서 에러를 잡으면 에러 토스트를 띄우는 형식으로 변경해보면 어떨까요?!
살펴보니 https://www.slash.page/ko/ <- 요기서 에러바운더리 컴포넌트 직접 구현한 부분이 있던데 괜찮은 코드가 있어서 참고하여 첨부합니다!
componentDidCatch(error: Error, info: ErrorInfo) {
const { onError, ignoreError } = this.props;
if (ignoreError?.(error)) {
throw error;
}
onError?.(error, info);
}
fetcher에서 에러 객체를 뱉고, error.tsx 컴포넌트보다 커스텀한 에러바운더리 컴포너트를 하위에 배치하면 될 것 같다는 생각이...
비동기 로직에서 실행되는 에러들은 에러 바운더리에서 못 잡는 걸로 알고 있습니다. 그래서 방법은 두 가지 정도 같은데,
이렇게 해야할 거 같습니다.. 그런데 2번 방식으로 하게 되면 오히려 더 복잡해질 거 같아서 1번 방식이 조금 더 나을 거 같습니다 |
response interceptor에서 공통 토스트 에러 처리 로직 추가하는것이 가장 간단하겠지만, toast는 커스텀 훅을 바탕으로 움직이게 되는데 fetcher를 SSR 부분에서 사용하고 있는 부분들이 존재하여서 예기치 못한 문제들이 발생할까 확인해보아야 할 것 같아용 위 링크 확인해보니 비동기 에러 결과물을 |
|
🎉 변경 사항
fetcher의 에러핸들링 로직을 수정합니다.🚩 관련 이슈
🙏 여기는 꼭 봐주세요!
fetch는 에러를 던지지 않습니다. (fetcher유틸함수를 쓰지 않는 순정!) 그래서response.ok로 꼭 반환결과를 확인하고, 그에 따른 에러 처리가 필요했습니다.fetcher의 에러핸들링 부분에서 에러를 던져주면서 이 부분이 보완되었습니다.Axios처럼요..Error객체로 확인할 수 없었습니다. 그래서 이 부분을 수정했어요!fetch관련 코드를 모두try~catch구문으로 예외처리해주시는 부분이 꼬옥 들어가야 될 것 같아요!