-
Notifications
You must be signed in to change notification settings - Fork 77
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
[4팀 윤영서] 프레임워크 없이 SPA 만들기 #10
base: main
Are you sure you want to change the base?
[4팀 윤영서] 프레임워크 없이 SPA 만들기 #10
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.
안녕하세요 영서님!
감사하게 코드 리뷰를 해주셔서 은혜갚으러 왔습니다!😁
영서님이 전에 타입스크립트로 과제 진행해도 되는지 질문하시는 거 보고 속으로 '나도 해볼까?'라고 생각했지만 지레 겁먹고 하지 못했습니다.. ㅎㅎ
저도 영서님처럼 타입스크립트를 능숙하게 다루고 싶은데 회피하게 되네용 ㅎㅎ
코드를 보면서 여러 인사이트를 드리고 싶지만 제가 경험과 지식이 부족해서 리뷰를 많이 작성하진 않았지만 제가 남긴 건 코린이의 관점이라는 점 감안하고 봐주셨으면 합니다... ㅎㅎ
PROFILE: isHashRouter ? "#/profile" : "/profile", | ||
NOT_FOUND: isHashRouter ? "#/404" : "404", | ||
} as const; | ||
}; |
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.
우와...👍
경로 기반 라우터인지 해시 기반인지 전역으로 관리하니까 getPathnames
에서 isHashRouter
만 체크하고 맞춰서 경로를 반환하게 하셨네요!
저는 경로 기반 path
, 해시 기반 path
따로따로 넣어줘서 페이지를 두 번씩 넣어줘야 하는 번거로움이 있었는데 영서님은 멋지게 해결하셨군요!
isProtectedRoute
를 추가해서 로그인 검사?를 해야 하는 페이지인지 하지 않아도 되는 페이지인지 구분하는 방법도 너무 좋은 것 같아요!
저는 모든 라우팅에 대해서 로그인 유무를 검사하게 코드가 되어 있어서 지금은 페이지가 별로 없지만, 페이지가 많아진다면 하지 않아도 되는 검사를 계속 수행하게 될텐데.. 사실 영서님 코드 보기 전까지는 그런 생각도 못했습니다..👍
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 { render, navigate, init }; | ||
}; |
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.
라우터가 로그인 검사, 경로 확인, 페이지 컴포넌트 렌더링, 이벤트처리까지 담당하고 있는 것 같아요!
개인적인 의견이지만 라우터에 역할이 몰려있다는 생각이 들어서 document
에 이벤트 리스너를 추가하고 있느니 로직을 라우터와 분리해도 괜찮을 것 같아요..!
추가로 궁금한게 있는데 render
함수에서 checkAuth
를 호출하고 checkAuth
에서 로그인 검사에 실패하면 navigate
를 호출하잖아요?
근데 navigate
에서 render
를 다시 호출해서 실행하면 pathname
이 변경되긴 하겠지만 checkAuth
에서 navigate
를 호출하는 부분을 빼고 작성해도 정상 동작할 것 같아서요..!
만약에 제가 생각하는 흐름대로 로직이 동작하는게 맞다면 navigate
호출 다음 return
문이 동작하지 않을 것 같은데 navigate
호출부를 아예 제거해도 될 것 같아요!
const checkAuth = (pathname: string) => {
const route = ROUTES[pathname];
if (route?.isProtectedRoute && !localStorage.getItem("user")) {
return PATHNAMES.LOGIN;
}
if (pathname === PATHNAMES.LOGIN && !!localStorage.getItem("user")) {
return PATHNAMES.HOME;
}
return pathname;
};
이렇게 수정하시면 pathname
을 리턴하고 render
에서도 checkPath
변수에 담아서 페이지를 불러오는 걸 보면 위 동작을 의도하신 것 같은데 제가 생각한 게 맞을까요??
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.
라우터가 로그인 검사, 경로 확인, 페이지 컴포넌트 렌더링, 이벤트처리까지 담당하고 있는 것 같아요!
네 맞습니다 ㅠㅠ 사실 라우터가 너무 많은 역할을 담당하고 있어서 잘게 쪼개봐야겠다! 싶었는데 시간이 부족해서 쪼개보진 못했던 것같아요.. ㅎㅎ (반성)
이렇게 수정하시면 pathname을 리턴하고 render에서도 checkPath 변수에 담아서 페이지를 불러오는 걸 보면 위 동작을 의도하신 것 같은데 제가 생각한 게 맞을까요??
저도 원정님 말씀보니 원정님 말씀대로 navigate
를 제거해도 동작해야하지 않을까 생각했는데요! 제 코드에서는 navigate
내부에서 updatePath
와 render
를 모두 동작하고 있어서, 리턴전의 navigate
를 제거하니 의도한대로 동작하지 않는 상황이 발생하고 있습니다. 아마도 제가 역할을 제대로 분리하지 못해서 계속 순환적으로 동작하고 있는것 같아요!
정리하자면 제 코드의 문제는
checkAuth
는 경로 검증만 담당해야 하는데navigate
호출을 통해 라우팅까지 처리하고 있고navigate
는 다시render
를 호출하면서render
는 다시checkAuth
를 호출하는 순환 의존성
요 코드는 정리를 해봐야겠네요... 감사합니다 원정님! 덕분에 문제점을 깨달았습니다..ㅎㅎ
"skipLibCheck": true | ||
}, | ||
"include": ["src", "**/*.ts", "**/*.d.ts"] | ||
} |
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.
타입스크립트는 그냥 언어일 뿐이라...원정님 코드 작성하시는것을 보니 원정님은 타입스크립트도 빠르게 배우실것 같습니다..!!
저도 원정님처럼 많이 생각하고 코드 작성하는 법을 배우고 싶어요ㅎㅎ!!
추가 구현 및 작성 예정
과제 체크포인트
기본과제
1) 라우팅 구현:
2) 사용자 관리 기능:
3) 프로필 페이지 구현:
4) 컴포넌트 기반 구조 설계:
5) 상태 관리 초기 구현:
6) 이벤트 처리 및 DOM 조작:
7) 라우팅 예외 처리:
심화과제
1) 해시 라우터 구현
2) 라우트 가드 구현
3) 이벤트 위임
과제 셀프회고
기술적 성장
hash router
해시 라우터와 브라우저 라우터의 차이에 대해 처음 접하게 되었습니다.
해시는 URL에 포함될 수 있는 해시로 시작하는 부분을 이야기하는데, 해시 변경이 일어났을 때는 브라우저는 서버에 요청하지 않고 페이지가 새로고침됩니다. 새로고침이나 리다이렉션 되었을때도 해시(#) 이전의 도메인 주소로 요청을 보내므로 오류를 발생시키지 않습니다.
history api의 pushState와 popState 메서드의 지원으로 해시를 이용한 라우팅을 하지 않아도 되었지만, 모든 브라우저에서 지원하는 (history api의 이전 지원 버전인 경우) 라우트를 만들기 위해서는 해시를 이용해 라우팅을 지원해야합니다.
해시의 변경은
를 통해 알 수 있습니다.
구글링을 하다가 제가 자주 사용하던 react router dom에도 hash router가 있음을 알게 되었습니다.(BrowserRouter, HashRouter)
옵저버 패턴
항상 알고는 있었지만 직접 써본적은 없었는데, 이번에 리팩토링을 시도하면서 옵저버 패턴의 개념에 대해 이해하게 된 것 같습니다.
옵저버 패턴은 객체의 상태 변화에 따라 다른 객체들이 자동으로 그 변화를 알림을 받는 패턴으로, 주체가 상태를 변경하면, 옵저버들이 이를 감지하고 자신의 상태를 업데이트하는 방식입니다.
방금 md를 쓰기 위해서 구글링을 하다 알게된건데, 준일님의 블로그에서도 옵저버 패턴을 통해 상태관리를 하는 글이 있다는 것을 알게되었습니다.
하위에서 옵저버 패턴을 이용한 방법에 대한 내용이 언급됩니다.
코드 품질
사실 만족스러운 구현은 딱히 없고...전반적으로 리팩토링이 필요하다고 생각했습니다.
어떻게 하면 파일 구조를 잘 가져갈 수 있을지, 어떻게 구조화를 할 것인지를 생각했던 것 같습니다.
특히 hash 라우터와 브라우저 라우터를 어떻게 구분하게 할 수 있을까? 에 대한 고민이 있었습니다.
window.location.hash
의 경우에만 hash 라우터를 타도록 동작하게 하려고 했습니다만 이렇게 하니 브라우저 라우터와 구분할 수 없는 상황이 발생하였습니다. (ex. index.hash.html으로 들어간 경우 window.location.hash가 빈값이 되는 경우가 생기게 됨)window.ROUTER_MODE
를 통해 전역적으로 라우팅 모드를 관리하도록 변경하여 해결하였습니다.학습 효과 분석
다른 분들의 코드를 보고 좀 더 dx적으로 만족스러운 코드를 짜야겠다고 생각했습니다.
과제 피드백
리뷰 받고 싶은 내용
createRouter
개인적으로 마음에 들지 않은 부분이 하위의 부분인데요.
이번 과제의 요구사항이 이벤트 위임이라서 data-link가 달린 요소들의 이벤트를 상위에 위임하도록 작성하였는데요.
제가 이벤트 위임을 잘 이해하고 작성한것이 맞는지 고민입니다. 또 다른 이벤트 위임이 있어야 했을지...?
Store
현재 코드는 사실 단순히 로컬 스토리지에서 값을 가져와서, 전역 객체인 Store에 값을 넣어주는 행위를 반복하고 있습니다.
그런데 제가 생각하기에는 이정도라면 Store와 로컬스토리지와의 차이가 없는 것 같아서,
아래와 같이 옵저버 패턴을 사용하여 상태 옵저빙을 하도록 했는데요.
DOMContentLoaded 이벤트시에 setHeader를 호출하고
이때 userStore을 구독하도록 했는데, 제대로 구독이 되지 않고 상태 변화를 감지하지 못하고 있는 것을 발견했습니다.
프로필 업데이트 시에 Header에 있는 username이 변경되면 좋겠다고 생각했는데, 제가 구현한 대로는 그게 불가능해서...어떻게 하면 옵저빙을 구현할 수 있을지 궁금합니다.