-
Notifications
You must be signed in to change notification settings - Fork 78
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
[8팀 최준만] Chapter 1-1. 프레임워크 없이 SPA 만들기 #62
base: main
Are you sure you want to change the base?
Conversation
…가 아닌 기본 html string을 제공토록 처리
리뷰 받고싶은 내용 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.
안녕하세요 준만님 ~! 담당 학습메이트 오소현입니다 :)
요번주 과제 하시느라 너무 고생 많으셨어요 ㅎㅎㅎ 주말부터 열심히 공부하고, 최선을 다해 과제해주셔서 너무 감사해요 🍀🍀 다음주 과제도 화이팅입니다~!
준만님 코드를 리뷰하면서 클래스 내부에서 각 메소드들의 역할 분리도 잘 되어있어서 리뷰하기 좋았어요 사용하신 패턴들도 특징에 맞게 잘 사용해 작성해 주신 것 같아서 특히 더 좋았습니다 ㅎㅎ 각 도메인 별로 폴더에 관심사 분리해두려고 노력해두신 것도 어떤 기준으로 나누셨을까 고민하면서 리뷰해보았습니다 !!
제 부족한 개발실력으로,,ㅎㅎ 궁금한점이나 감탄하는 점을 위주로 리뷰를 달아보았어요! 제 부족한 리뷰가 준만님께서 과제를 다시 한번 더 생각해보게 되는 계기가 되셨으면 좋겠어요 🍀
다시 한번 더 과제하시느라 고생많으셨어요 👍
resolve: { | ||
alias: { | ||
"@": path.resolve(__dirname, "./src"), | ||
}, | ||
}, |
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.
제 라스트 리뷰 준만님 ㅎㅎ alias 우리팀에 잘 전파해주셔서 감사합니다 ~! 다들 준만님께서 잘 알려주셨다고 해서 리뷰에서 감동이었네요 ...❤️
준만님에게도 동일한 리뷰를 드립니다 ㅎㅎ 현재는 /src 내부를 기준으로만 하고 있는데 좀더 세분화되어서 단계를 내려가는 것도 좋을 것 같아요!
'@components': path.resolve(__dirname, './src/components'),
'@lib': path.resolve(__dirname, './src/lib'),
'@stores: path.resolve(__dirname, './src/stores'),
다음 주차들 과제에서 폴더들이 많아지게 되면 한 번 시도해보시면 좋을 것 같아용 ㅎㅎㅎ
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.
넵넵 alias 추가해서 2주차는 달려보겠습니다 ㅎㅎ
function log(text) { | ||
console.log(text); | ||
} | ||
|
||
export { log }; |
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.
오~~~ log를 직접 만들어 분리하셨군요
저도 이과제 할때 logger를 직접 만들었었는데 결국에 과제에서는 잘 쓰지 못했고 폐지했습니다 ㅠㅠ 그런데 준만님께서는 잘 사용하셨네요 :) 굿굿 너무 좋습니다 !
준만님은 평소에도 잘 사용하시나요?!
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.
해당 래핑 함수가 깔끔할 수 있다고 생각했는데 저도 생각보다 안쓰게 되더라구요,,, (es6 snippet으로 clg를 써서 쉽게 불러올 수 있다보니)
여담으로 에러로그 수집도 꼭 해봐야하는데 아직 실무도 학습도 해보지 못했네요.
저도 sentry같은거 써서 런타임 로그 수집해 보고싶어요,,,,(할게 많타...)
addObserver(observer) { | ||
this.observers.push(observer); | ||
} | ||
|
||
// 컴포넌트 제거시 옵저버리스트에서 제거해 줘야할 듯. | ||
removeObserver(observer) { | ||
const index = this.observers.indexOf(observer); | ||
if (index > -1) { | ||
this.observers.splice(index, 1); | ||
} | ||
} | ||
|
||
// 컴포넌트 상태 업데이트 및 리렌더 하기 위해 옵저버들에게 데이터 전달 | ||
notifyObservers(data) { | ||
this.observers.forEach((observer) => observer.update(data)); | ||
} |
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.
각각의 옵저버를 add, remove, notifiy로 객체 간의 결합도를 낮추면서 옵저버 패턴으로 잘 작성해주신 것 같아요 !!
Observer 네이밍을 사용해서 그런지 패턴의 예시도 잘 드러나 보여요 ㅎㅎㅎ 너무 좋습니다 bb
class UserStore extends Store { | ||
constructor() { | ||
super({ user: User.getUser() }); | ||
} |
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.
Store 클래스를 상속받아서 UserStore라는 유저에게 특화된 로직을 구현하는 구조로 잘 작성해주셨네요!!
공통되는 Store를 잘 설계해주신 덕분에 UserStore, FeatureStore로 잘 상속받아 사용할 수 있는 구조로 만들어주신 것 같아요 bb 너무 좋습니다 각 클래스의 기능도 잘 드러나고, 로직도 간결하게 bb 된 것 같습니다 !
const MainPage = (dom, query) => { | ||
if (!query) { | ||
dom.innerHTML = render(); | ||
} else { | ||
dom.querySelector(query).innerHTML = render(); | ||
} | ||
addEventListeners(); | ||
}; | ||
|
||
export default MainPage; |
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.
요 로직이 각각 페이지들 마다 구현되어 있는데 이를 공통 함수로 분리해 가져다 쓰는 구조로 가면 좋을 것 같아요 ㅎㅎ
// 해당 파일을 통해 진입하는 경우 무조건 해시 라우터를 사용하도록 설정 | ||
router.toggleHash(true); |
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.
Hash인지 아닌지 toggleHash 라는 네이밍으로 켜고 끄는 것 같은 느낌이 있네요 ㅎㅎㅎ 네이밍 좋은데요?!?
404: { path: "/404", page: ErrorPage }, | ||
}; | ||
|
||
class Router { |
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.
이 Router 클래스가 굉장히 많은 역할을 해주고 있는데, 코드가 잘 읽힌 이유가 준만님께서 클래스 내부 메소드 분리를 넘 잘해주신 것 같아요! 각 메소드의 역할을 잘 드러내고 있는 네이밍, 역할 분리가 잘 되어있는 것 같아요 ㅎㅎ
log를 사용해서 예외처리도 잘 해주신 것 같은데 log을 더 고도화 해보는 건 어떨까요? errorlog 이런 것 같이요!
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.
@junman95 저희 때는 해시 라우터 제시 사항이 없어서 일반 히스토리 라우터만 구현했었는데 만약에 저라면 라우터 추상 클래스를 두고 해시, 히스토리 라우터를 나누어서 구현했을 것 같아요 :) 이유는 말씀해주신 것 처럼 확장성을 고려하기도 했고, 역할 분리를 명확하게 구분 짓는게 좋다고 생각했어요 :)
const TAB_ITEMS = { | ||
main: { | ||
path: "main", | ||
label: "홈", | ||
}, | ||
profile: { | ||
path: "profile", | ||
label: "프로필", | ||
}, | ||
login: { | ||
path: "login", | ||
label: "로그인", | ||
}, | ||
logout: { | ||
path: "logout", | ||
label: "로그아웃", | ||
}, | ||
}; | ||
|
||
const TABS_WITHOUT_USER = [TAB_ITEMS.main, TAB_ITEMS.login]; | ||
const TABS_WITH_USER = [TAB_ITEMS.main, TAB_ITEMS.profile, TAB_ITEMS.logout]; |
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.
상수처리 해두신점 !! 너무 좋습니다 :) 상수가 컴포넌트에 의존적일때는 상단에 작성해두기도 하는데 한번에 관리하려면 모아두는게 맞는 것 같기도 하고 항상 고민입니다,,ㅎㅎ
폴더 구조를 잘 나눠주실려고 노력해주신 것 같아서 한번 tree로 폴더 구조를 한 눈에 보셨으면 좋겠습니다~!
|
이거 어디서 만들어주나요? 항상 궁금했어요 |
@junman95 |
과제 체크포인트
기본과제
1) 라우팅 구현:
2) 사용자 관리 기능:
3) 프로필 페이지 구현:
4) 컴포넌트 기반 구조 설계:
5) 상태 관리 초기 구현:
6) 이벤트 처리 및 DOM 조작:
7) 라우팅 예외 처리:
심화과제
1) 해시 라우터 구현
2) 라우트 가드 구현
3) 이벤트 위임
과제 셀프회고
기술적 성장
코드 품질
학습 효과 분석
과제 피드백
너무 명령형으로 짜여진 코드들이 많아, 정말 필요한 타이밍에 호출되는지 여부를 파악해 봐야 할 것 같습니다.
리뷰 받고 싶은 내용
옵저버 패턴을 이용한 상태관리
junman95@2b0eb8e
(상태 변경을 통해 Header 만 재 렌더링 되는 화면입니다.)
얼핏 학습하기로는 useState도 이러한 형태를 지니는 것으로 알고 있는데 클로저 이외에 이전의 상태를 기억할 방법이 있을까요?
혹은 제가 짠 구조에서 추가되거나 덜어야 될 부분이 있을지 궁금합니다.
해시 라우터 과제
과제가 제가 짠 것처럼 모드를 스위치 하거나, 실행 환경에 따라 모듈화 된 해시 라우터 혹은 히스토리 라우터를 사용하는게 맞을까요? 두개 타입의 라우터가 동시에 작동될 수 있게 구현해야 하는것인지 현재처럼 환경에 맞게만 동작하면 되는 것인지 잘 모르겠습니다.