-
Notifications
You must be signed in to change notification settings - Fork 4
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/CK-187] Error-Boundary를 활용한 에러핸들링(초안) #148
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.
크으으.. 진짜 난이도 빡셌겠네요!! 넘 고생하셨고 몇가지 부분만 확인 부탁드려요!
pr 메세지에 남긴 적용해야 할 개선사항들은 다음 스프린트에 해봅시다!
<RuntimeErrorBoundary> | ||
<ServerErrorBoundary> | ||
<NotFoundErrorBoundary> | ||
<Suspense fallback={<Spinner />}>{children}</Suspense> |
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.
이렇게 되면, Suspense 가 전체 라우터를 감싸는 것 같은디 어느 하나 로딩 상태가 걸리면 전체 화면에 항상 스피너가 돌지는 않았나여? 😀
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.
맞아요오..!!! 이 AsyncBoundary를 필요한 곳에 부분적으로 감싸줘야하는데 아직 완성이 되지 않아서 일단 app 전체를 감싸줬어요!
|
||
const moveMainPage = () => { | ||
navigate('/'); | ||
window.location.reload(); |
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.
😅 ㅋㅋㅋㅋㅋㅋㅋㅋ
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 moveMainPage = () => { | ||
navigate('/'); | ||
window.location.reload(); | ||
}; |
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.
중복되는 것 같은디 훅으로 빼봐도 좋을 듯 싶네요!
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.
앗 어차피 저 reload부분이 리셋로직으로 대체될꺼라서 임시방편으로 그냥 뒀는데요..!! 훅이 필요할까요!?
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.
앗 그걸 몰랐네요! 일단 다음에 바로 수정하죠!
<S.Container> | ||
<S.ElephantImage src={elephantImage} alt='crying-elephant' /> | ||
<S.RuntimeTitle>Error</S.RuntimeTitle> | ||
<S.RuntimeText> |
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.
마크업 계층구조도 반복되는 느낌이 드는데, HOC 와 같은 방식을 추후에 적용해보든건 어떨까여??
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.
호호~ 좋은 생각이에요 고려해보겠습니다!!
|
||
if (hasError && error) { | ||
return <ErrorBoundaryFallback errorMessage={error.message} />; | ||
// eslint-disable-next-line react/no-unused-class-component-methods |
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.
eslint config 파일에서 처리하는건 어떨까요??
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.
네엡 없앨께욧!
return <ErrorBoundaryFallback errorMessage={error.message} />; | ||
// eslint-disable-next-line react/no-unused-class-component-methods | ||
resetError = () => { | ||
// eslint-disable-next-line react/destructuring-assignment |
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.
eslint config 파일에서 처리하는건 어떨까요??
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.
넵!!
|
||
class ServerErrorBoundary extends ErrorBoundary { | ||
componentDidCatch(error: any, _errorInfo: ErrorInfo): void { | ||
const status = /5\d{2}/; |
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.
regex 모아둔곳으로 빼는건 어떨까요??
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.
아핫 넵!!
onError: (e) => { | ||
const error = e as any; | ||
if (error.response.status === 400) { | ||
alert(error.response.data.message); |
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.
alert 말고 다른 방식은 없을까요??!
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.
토스트로 바꿔볼께요~!!
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.
엉 이거 토스트로 변경하려구 했는데, 애초에 우리가 만들어둔 토스트 자체가 ToastProvider 안에서만 쓸 수 있는데 이 queryClient는 그 밖에서 선언되고있어서 이 계층에서 토스트 사용은 불가능하더라구요..!! 일단 alert로 두고 토스트를 사용할 방법을 고민해볼께요
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.
오오.. 맞네요 진짜! 고민해보죠!!
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.
네이브 어려운 부분이었는데 정말 고생하셨어요!
부엉이가 이미 잘 달아주셔서 제 리뷰는 코맨트만 확인해주시면 될 것 같아요🤩
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.
넘 좋네요!
부엉이 말대로 중복만 조금 제거해주면 더 좋을 것 같아요🤩
import ErrorBoundary from './ErrorBoundary'; | ||
|
||
class CriticalErrorBoundary extends ErrorBoundary { | ||
componentDidCatch(_error: any, _errorInfo: ErrorInfo): void {} |
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.
매개변수명이 _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.
사용하지 않는 매개변수때문에 린트가 징징대서 _를 넣어줬어요..!! 추후에 에러 처리 로직이 들어가야하는 부분이라서요!!
import ErrorBoundary from './ErrorBoundary'; | ||
|
||
class RuntimeErrorBoundary extends ErrorBoundary { | ||
componentDidCatch(_error: any, _errorInfo: ErrorInfo): void {} |
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.
없어도 무방한 라인이라는 생각이 들어요🤩
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.
저도그래요! 지워보겠습니다~!
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.
고생하셨으요~~
📌 작업 이슈 번호
CK-187
✨ 작업 내용
에러 핸들링을 위한 ErrorBoundary를 만들었습니다. 선언적으로 에러를 처리하기 위해 에러의 종류별로 에러바운더리를 따로 만들어줬어요.
위의 모든 ErrorBoundary는 AsyncBoundary에 Suspense와 함께 묶여있어요. 해당 에러가 아니면 상위 컴포넌트로 error를 계속 throw해줍니다.
2023-09-13.3.55.56.mov
2023-09-13.3.56.29.mov
💬 리뷰어에게 남길 멘트
에러 핸들링을 위한 초석을 다진 코드라서 많이 부족해요..!! 일단 사용자 피드백을 위해 서비스가 돌아갈 정도로 코드를 짜놓았는데, 담주 내로 고도화 시켜보겠습니다앗
😭부족한부분😭
이밖에도 부족해보이는 부분 말씀해주시면 반영해보겠습니다앙
🚀 요구사항 분석