-
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
[8팀 김도운] [Chapter 1-1] 프레임워크 없이 SPA 만들기 #22
base: main
Are you sure you want to change the base?
Conversation
[1주차] 기본과제
오 도운님 평소에 말씀도 차분하게 잘 하시는데, 그게 주석에서도 느껴지네요..👍 |
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.
안녕하세요 도운님~! 담당 학습메이트 오소현입니다 :)
도운님 과제는 제 귀중한 학습 자료였습니다 ㅎㅎ 처음에는 이 과제를 MVVM 패턴으로 어떻게 할 수 있을까 신기해하면서 리뷰를 하기 시작했는데요! 전체적으로 바인딩 / 컴포넌트 / 컨트롤러의 핵심 로직을 /core
폴더로 분리해서 상속해서 사용하기 쉬운 구조로 설계하신 점이 가장 인상 깊었습니다
관심사 분리를 너무 잘해주신게 아닐까 싶은 생각이 드네요... 몇 수 앞을 내다보신 코드인지 박수만 치다 갑니다 👏🏻👏🏻 (믿기지 않으시겠지만 기립박수입니다 ㅎㅎ)
도운님께서 이번과제에 딱 핵심으로 가져가려고 하시는 디자인 패턴을 정말 확실하게 잘 가져가신 것 같아요 bb 고민의 흔적도 많이 느껴지고, 각 주석을 보면서 저도 여러번 보고 이해하는 구조가 되었습니다 ㅎㅎ
제일 박수 친 구간을 뽑자면 Binding에 이전 Controller 객체에 대한 싱글톤 인스턴스 의존성을 주입하여 캐싱 처리를 하고자 하셨던게 bb 진짜 대단하십니다 상당히 과제에 많은 노력이 들어간 것 같아요 너무 고생많으셧습니다 :)
부족한 제 실력이지만 조금이나마 궁금한 점, 제안하고 싶은 점을 위주로 리뷰를 남겨보았습니다 👍
제 리뷰로 인해서 도운님께서 이번 과제를 다시한번 더 생각해 볼 수 있는 좋은 기회가 되시기를 바래봅니다 🍀
과제로도 느껴졌지만 많은 고민을 하셨군요 이번주 과제하시느라 너무 고생하셨습니다 :) 다음주 2주차 과제도 화이팅입니다~! 좋은 코드 리뷰할 수 있어서 좋았습니다 ㅎㅎ
class ComponentBinding { | ||
$target; | ||
|
||
constructor($target) { | ||
this.$target = $target; | ||
this._dependencies(); | ||
} | ||
_dependencies() { | ||
new NavbarController(this.$target); | ||
} | ||
} |
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.
오호 특정 DOM 요소($target)에 대한 바인딩을 수행하는 역할을 컴포넌트와 페이지로 분리하셨군요,,!?!?
혹시 이렇게 구현하게 된 이유가 있으신가요? 요 역할을 따로 분리해주신게 넘 신기해요 👍 새로운 구조를 하나 알아갑니다,,,
@@ -0,0 +1,52 @@ | |||
class UserService { | |||
static USER_KEY = "user"; |
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.
크 적절한 상수 처리 👍 너무 좋습니다 ~! 유지보수하기 쉬운 구조네요 bb
제가 3기 하면서 적절한 상수 처리의 기준이 뭘까 고민했던 적이 있었는데요 😲 도운님 만의 상수로 처리하는 기준이 있으신가요? 저는 보통 이제 변할 것 같은 값들을 위주로 선정하게 되는데, 다른 개발자의 기준도 궁금합니다!
/** | ||
* 인증 정보 존재 여부 반환 | ||
* @returns {string|null} localStorage에 저장된 사용자 정보(JSON 문자열) 또는 null | ||
*/ |
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.
오! 주석 깔끔하게 잘 작성해주셨군요!
저도 이제 회사에서 jsdoc을 사용하는데요! 자세하게 작성된 블로그 글을 하나 공유할게요~!
https://ilimes.github.io/javascript/post29/
현재 도운님처럼 여러 jsdoc의 옵션을 사용해서 잘 구현해주고 계신 것 같지만 다양한 옵션을 써서 더 깔끔한 주석으로 채우시길 응원합니다🍀
아참 제가 3기 때 주석을 너무 남발(?)한적이 있었는데, 너무 많은 주석은 오히려 코드 가독성을 떨어뜨린다는 피드백을 받았었어요! 저도 그 이후로는 핵심 로직을 구분해서 최대한 간결하지만~ 깔끔하게 작성해보려고 노력중입니다
혹시 도운님께서는 주석 작성하시는 기준이 있으신가요?! 궁금합니다!
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.
좋은 레퍼런스 감사합니다 :) 기존에 주석을 기본적으로만 작성할지 jsdoc으로 상세하게 작성할지 고민중이었습니다. 그러다가 소현님이 언급하신것처럼 주석이 너무 많아져서 가독성이 떨어지는 것 같아 핵심 로직에만 일반 주석을 포함했습니다! 다만 위 코드의 경우 미처 수정하지 못한 부분이네요😅
export const ROUTES = { | ||
MAIN: "/", | ||
PROFILE: "/profile", | ||
LOGIN: "/login", | ||
ERROR: "/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.
오 ROUTES를 상수로 따로 정의해두셨군요 bb 좋습니다
그런데 요 파일의 폴더 구조를 /contants라는 상수를 관리하는 폴더를 만들어서 해당 폴더에 넣었으면 조금 더 역할 분리가 되었을 것 같다는 생각이 듭니다! 유지보수하기도 더 편할 것 같아요 :)
하지만 요렇게 상수로 따로 분리해서 관리해두신 점 매우 좋은 것 같습니다 bb
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.
관리되는 상수가 별로 없다보니 routes 파일에 포함했는데 말씀주신대로 constants 폴더를 만들어서 포함하는게 훨씬 깔끔한 것 같습니다!
export function loginGuard() { | ||
return !UserService.getUser(); | ||
} | ||
|
||
export function profileGuard() { | ||
return !!UserService.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.
이 서비스의 핵심이 되는 로그인 / 로그아웃 상태를 Guard로 분리해서 사용하고 계신 점 너무 좋네요 bb
요 guard로 인해 접속한 유저의 로그인 여부를 체크하고 있도록 하는 구조 좋습니다 :)
이 guards를 사용하는 곳의 코드가 기대돼요 ㅎㅎ
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.
해당 가드는 라우트를 추가할 때 가드를 포함할 수 있도록 설계하였습니다 ㅎㅎ
// main.js
router.addRoute(ROUTES.PROFILE, pages.profile, profileGuard, "/login");
router.addRoute(ROUTES.LOGIN, pages.login, loginGuard, "/");
@@ -0,0 +1,137 @@ | |||
import Component from "../core/Component"; | |||
|
|||
class Router extends Component { |
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.
구조를 먼저 일일이 파악하기보다 AI로 시각화하여 참고한다면 훨씬 도움이 되겠네요.. 좋은 정보 알아갑니다!!
} | ||
} | ||
|
||
export default 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 클래스를 열심히 공부해봤는데, 어떤 흐름대로 기능들을 모두 다 제어하기 위해서 잘 작성해주신 것 같아요 👍 부족한 저에게는 ㅠㅠ 조금 어려웠지만 많이 배웠습니다 도운님 ,,, 👍 최고입니다 ㅎㅎ
보다보니까 드는 생각이 클래스 크게 하나가 있고 여러 메소드가 있는 구조인데 기능별로 모듈을 분리하는 것은 어떤가요?
예를들면 제가 요 클래스를 모듈화 해본다면
- [1] 라우트 관리 하는 모듈 : 라우트를 추가하고 제거하거나.. 정규식 변환하는 역할이 들어갈 것 같네요 ?!
- [2] 라우트를 핸들링 하는 모듈 : handleRoute()와 같이 현재 URL을 기준으로 처리되고, 네비게이션 하는 역할이 들어갈 것 같네요
- [3] 이벤트 처리하는 모듈
- [4] 에러 처리하는 모듈?
이렇게 분리해보면 좋을 것 같은데 도운님 생각은 어떠신가요? 이 클래스에 너무 공들이신게 보여요,, 진짜 어떻게 하신것이죠 너무 대단합니다 !!
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.
현재 전체적인 컴포넌트들이 클래스 기반으로 구조화되어있어 별도로 모듈화는 진행하지 않았습니다. 다만 라우터의 경우 컴포넌트를 상속하지만 UI 컴포넌트가 아니기 때문에 소현님 말씀대로 함수형으로 모듈화해도 좋은 방법일 것 같아요!!
<div class="flex items-center mb-2"> | ||
<img src="https://via.placeholder.com/40" alt="프로필" class="rounded-full mr-2"> | ||
<div> | ||
<p class="font-bold">홍길동</p> | ||
<p class="text-sm text-gray-500">5분 전</p> | ||
</div> | ||
</div> | ||
<p>오늘 날씨가 정말 좋네요. 다들 좋은 하루 보내세요!</p> | ||
<div class="mt-2 flex justify-between text-gray-500"> | ||
<button>좋아요</button> | ||
<button>댓글</button> | ||
<button>공유</button>a | ||
</div> | ||
</div> |
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.
도운님 과제 폴더 구조를 보니까 컴포넌트 파일도 따로 생성해서 분리해두셨던데 이 페이지에서 이 부분도 UI를 따로 분리 할 수 있지 않을까!? 싶습니다 ㅎㅎ 공통된 UI를 분리해서 조금 더 간결한 코드로 작성해 보는 것도 좋을 것 같아요!
필요한 데이터는 아래와 같이 배열로 따로 빼도 되고..! 중복 코드를 줄이는 것도 좋을 것 같다는 생각이 들었습니다 ~!
{
name: "홍길동",
time: "5분 전",
message: "오늘 날씨가 정말 좋네요. 다들 좋은 하루 보내세요!",
avatar: "https://via.placeholder.com/40"
},
{
name: "김철수",
time: "15분 전",
message: "새로운 프로젝트를 시작했어요. 열심히 코딩 중입니다!",
avatar: "https://via.placeholder.com/40"
},
{
name: "이영희",
time: "30분 전",
message: "오늘 점심 메뉴 추천 받습니다. 뭐가 좋을까요?",
avatar: "https://via.placeholder.com/40"
}
];
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 Router from "../router/Router"; | ||
import ProfileController from "./ProfileController"; | ||
|
||
class LoginController extends Controller { |
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.
각각의 역할별로 컨트롤러를 분리하시고 core의 컨트롤러를 가지고 확장하는 구조를 가지고 있군요 ... 헙🧐 도대체 얼마나 많은 고민과 노력을 하셨을지 ... 👍 도운님의 코드의 의도를 잘 파악해 보겠습니다 각각의 관심사별로 잘 분리해 사용하고 계신 것 같습니다 bb 최고최고
// 컨트롤러가 있으면 상태 변경 시 자동 리렌더링 | ||
if (this.controller) { | ||
this.controller.setOnStateChange(() => this.render()); | ||
} | ||
|
||
this.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.
저는 도운님의 코드를 보고 이것을 위해 Core로 Components를 개발해두신 게 아닐까?! 생각했습니다
우선 이 구조를 보고 컴포넌트의 렌더링 로직들이 캡슐화되어있어서 관리하기도 쉽고, 애초에 이 컴포넌트를 사용하고자 하는 도메인에 의존한 컴포넌트에서 상속받아서 쉽게 사용할 수 있다는 것, 그리고 이 코드의 장점인 컨트롤러를 통한 상태 관리 구조는 컴포넌트의 상태 변화에 따른 자동 리렌더링을 지원하게 한다는 것👏🏻👏🏻👏🏻 이 가장 두드러지는 장점인것 같아요!
저 혹시 잘 이해한거 맞나요 ..?ㅎㅎ 하하 코드 리뷰가 아니고 도운님의 멋진 코드를 더 고민해보는 리뷰가 되었네요
처음에는 어떤 목적으로 해당 디자인 패턴을 도입해서 이 구조를 짜게 되셨을 까 고민을 했는데 각 목적성에 맞게 잘 분리해주시고, 이 Core 폴더에 있는 바인딩, 컴포넌트, 컨트롤러의 핵심 로직들이 이 서비스의 확장성에 가장 높은 기여를 할 것 같아요 👍 정말 몇 수 앞을 내다 보신 것입니까,,, 👀 저도 이렇게 도운님처럼 확장성을 잘 고려하는 개발자가 되고 싶군요,, core 로직까지 보고나서야 점차 이해가 잘 되었습니다
이 구조를 처음부터 끝까지 설계하시다니,,, 존경스럽습니다 많이 배우고 갑니다! 고생하셨습니다 도운님
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) 이벤트 위임
과제 셀프회고
기술적 성장
React 프레임워크에 대한 간단한 이해로 웹을 개발해왔기에 자바스크립트나 DOM과 같은 중요한 개념에 대한 이해가 부족했습니다. 이번 과제를 계기로 React의 근간을 이루는 SPA의 개념에 대해 깊게 공부해볼 수 있는 좋은 계기가 된 것 같습니다. 주로 학습한 내용은 다음과 같습니다.
코드 품질
이번 과제에서 설계에 관한 부분을 가장 오랫동안 고민하였습니다. 비록 한 주 동안 작성하고 떠나보낼 코드이지만 그럼에도 소소하게 개발해오며 쌓아온 원칙에 어긋나지 않기 위해 노력하였습니다. 제가 중요하게 생각하는 소소한 원칙은 다음과 같습니다.
유지보수성과 확장성을 고려하여 개발하자
다소 뻔한 이야기일 수 있지만 언제나 가장 중요한 원칙 중 하나라고 생각합니다. 저는 주로 유지보수성, 확장성을 고려할 때 무한 확장에 대한 간단한 예시를 들어보는 편입니다. 예를 들어 이번 과제에서는 "페이지가 수십개이고 이벤트 리스너가 무수히 많이 늘어난다면?" 라는 가정을 해보았습니다.
무의미한 혹은 목적성에 맞지 않는 개발은 지양하자
과거 실무 프로젝트에서 리팩토링을 진행한 경험이 있습니다. 당시 MVVM 패턴으로 설계되어 있었는데 이때 API를 호출하는 경우 불필요하게 4개의 레이어를 거쳐야 하는 과도한 추상화 문제가 존재했습니다.(Repository Interface → Repository Implement → DataSource Interface → DataSource Implement) 이는 패턴을 잘못 이해하고 무조건 General한 케이스를 적용하여 했던 문제였습니다.
금주 멘토링에서 로직 분리와 추상화의 적절한 수준에 대해 코치님과 얘기를 나누게 되었습니다. 그 과정에서 무조건적인 로직 분리와 추상화가 반드시 좋은 설계로 이어지지 않는다는 코치님의 의견에 다시금 공감하게 되었습니다. 저 또한 준일 코치님의 클래스 기반 컴포넌트 블로그 글을 참고하면서 코드를 무의식적으로 복사하여 적용하는 경우가 있었습니다. 하지만 다시 고민해보니 내가 개발하고자 하는 패턴과 구조에 맞지 않게 무의미한 개발을 하고 있다는 점을 인지하였습니다.
통일감있게 코드를 작성하자
팀 혹은 개인이 스스로 정한 컨벤션(Convention)을 통해 일관성을 지키자는 원칙입니다. 파일명, 변수명, 주석 등 코드의 전반적인 부분에서 일관된 스타일과 명명 규칙을 적용하기 위해 노력했습니다.
이번 프로젝트에서는 단일책임원칙에 따라 크게 4개의 핵심 요소를 분류하였습니다.
먼저 컴포넌트에 관한 추상화 클래스입니다. 해당 클래스를 확장하여 페이지 컴포넌트 등을 생성할 수 있습니다.
다음은 컨트롤러 추상화 클래스입니다. 컨트롤러는 앞서 설명한 바와 같이 상태 및 이벤트 리스너를 관리할 수 있습니다.
컨트롤러를 통해 상태를 변경했을 때 컴포넌트에서 콜백으로 등록한 렌더 함수를 작동시켜 UI 상에서 업데이트될 수 있도록 하였습니다.
라우터의 경우
main.js
파일에서 페이지 컴포넌트와 함께 등록하여 라우트 동작 시 마다 해당하는 라우트의 페이지 컴포넌트를 렌더링하는 방식으로 구축하였습니다. 이때 라우트가드에 대한 검증 함수와 가드 이벤트가 동작할 때 리다이렉트 URL을 함께 등록할 수 있도록 해주었습니다.Binding의 경우 Controller를 초기 웹 로드 시 사전에 Controller 객체에 대한 싱글턴 인스턴스 의존성을 주입하여 캐싱하여 사용할 수 있도록 하는 전략을 사용하였습니다.
학습 효과 분석
직접적으로 과제와는 연관이 없지만 테스트 기반으로 코드를 검증하는 게 매우 흥미로웠습니다. 그동안 TDD를 개념적으로만 접했을 뿐 실제로 적용해본 경험은 없기 때문입니다. 이번 과제에서 테스트 코드를 함께 검토해보며 단순히 코드의 논리적 검증 뿐만 아니라 UI가 기대한 대로 동작하는지 화면 상에서 확인하는 방법에 대해서도 알게되었습니다. 특히 실무에서 매주 앱 배포를 할 때 CI/CD 파이프라인에 좀 더 공부하여 테스트 기반으로 자동화된 QA를 도입해볼걸 하는 아쉬움도 있었습니다..
리뷰 받고 싶은 내용
상태 관리(특히 localStorage와 연관한 유저 상태 관리)에 있어 옵저버 패턴을 기반으로 리팩토링을 계획하다가 동기가 충분하지 않아 기존 코드를 유지하였습니다. 옵저버 패턴은 주로 특정 상태 변화가 여러 곳으로 전파될 때 유용하다고 이해하고 있는데 이번 프로젝트 구조에서 이를 도입했을 때 어떤 이점이 있을까요?
위에 설명된 내용과 같이 이번 과제를 View - Controller 구조와 의존성 주입 기반 구조로 구성하였습니다. 이러한 아키텍처가 SPA 구현에 있어 적절했는지 혹은 잠재적으로 발생할 수 있는 단점(메모리 관련 이슈 혹은 의존성 관리 복잡도 등..)이 무엇이 있을지 궁금합니다.