[2단계 - 웹 기반 로또 게임] 레스 미션 제출합니다.#470
[2단계 - 웹 기반 로또 게임] 레스 미션 제출합니다.#470lee-eojin wants to merge 21 commits intowoowacourse:lee-eojinfrom
Conversation
degurii
left a comment
There was a problem hiding this comment.
요번주도 고생 많으셨습니다. 🚀
PR diff에 도메인 로직 변경이 하나도 없는 걸 보니 기존 도메인 로직 재사용과 뷰 레이어 전환을 성공적으로 수행하신 것 같아, 과제의 핵심 목표를 잘 달성해주셨다고 느껴졌습니다.
특히 컴포넌트마다 mount/unmount를 두고 생성, 해제 시점을 명확히 나눠주신 부분이 인상깊었습니다. 이런 인터페이스를 미리 잡아두면 상위에서 기초적인 라이프사이클 제어를 수행할 수 있고, 이후 렌더링 로직을 고도화하는 시점에서도 확장에 더 유연하게끔 만들어주는 부분이라고 생각해요.
step1 초반에 헤매셨던 모습이랑 달리, 빠르게 발전하시는 모습을 보여주신 것 같습니다.
질문 주신 부분 답변 드리고, 세부 코멘트는 코드레벨에 달아두도록 하겠습니다. 고생 많으셨습니다!
스타일과 컴포넌트의 역할 분리 기준이 적절한가에 대해 피드백을 받고 싶습니다.
- CSS 파일은 "변경 이유가 다른가"를 기준으로 분리했습니다. 하나의 파일에 모든 스타일을 두면, 변경 범위를 파악하기가 어렵고 의도치 않은 부수효과가 생길수도 있다고 판단했습니다. 브라우저 호환성, 디자인 시안, 타이포그래피 정책, 레이아웃은 각각 독립적인 이유로 바뀌기 때문에 파일을 나누면 수정 범위가 명확해지고 영향도를 예측하기 쉬워지지 않을까 라고 생각했습니다.
CSS를 변경 이유가 다른가를 기준으로 나눈 것도 납득 가능했고, 적어도 지금 구조에서는 단순히 파일을 쪼갠 게 아니라 나름의 이유를 가지고 분리한 흔적이 보여서 좋았습니다.
이런 분리는 사실 정답이 있다기보다 기준이 있느냐가 더 중요하다고 생각하는데, 이번 작업에서는 그 기준이 명확한 편으로 보여 괜찮았다고 생각합니다.
이번에 세우신 기준에 대해서는 이후에도 계속 점검해보시면 좋을 것 같아요. 실제로 개발을 진행하다 보면 처음엔 분명 다른 관심사라고 생각했던 것들이 예상보다 자주 같은 이유로 바뀌기도 하고, 반대로 하나로 묶어놨던 것들이 기획 대격변으로 완전히 갈라지는 경우도 잦습니다. 그러니 이번에 잡으신 "변경 이유가 다른가"라는 기준을 이후에도 계속 점검해보시면서, 어디서 잘 맞아 떨어지고 어디서 어긋나는지, 그에따른 불필요한 리소스가 얼마나 되는지 경험으로 확인해보시면 좋을 것 같습니다.
- 컴포넌트는 자신의 DOM을 직접 생성하고 마운트 / 언마운트를 관리하도록 설계해보았습니다. 그리고 컴포넌트 내부에서 인라인 스타일을 사용하지 않고 클래스명만 부여한 이유는, 시각적 표현을 CSS가 전담해야 컴포넌트 코드 수정없이도 디자인을 바꿀수 있다고 판단했습니다.
우선 인라인 스타일 대신 CSS 클래스를 이용한 부분은 좋은 접근이셨다고 생각합니다. 그리고 마운트/언마운트를 도입해보신 부분도 꽤 의미있는 시도라고 생각되어요.
[권장]
다만 개인적으론 마운트/언마운트를 왜 도입했는지에 대해서는 PR만 봤을 때 잘 납득되지 않는다는 생각은 듭니다. 설명에 써주신 것처럼 "컴포넌트가 자신의 DOM 생명주기를 직접 관리"하기 위한 장치인 점은 이해했는데, 그렇게 관리하는 방식이 어떤 이점을 가지기에 도입을 하신건지, 이번 과제에서 그 장점을 잘 살렸다고 생각하시는지, 이를 도입하지 않았을 때 생기는 불편함이 어떤 것인지 등등, 좀 더 설득력 있는 근거로 풀어주셨다면 더 좋았을 것 같아요.
물론 지금은 학습 과정이시고, 모던 프레임워크에서 흔히 보이는 개념들을 직접 시도해보면서 감을 잡아가는 과정임을 잘 알고 있기 때문에, 잘못했다고 말씀 드리는 건 아니에요. 다만 이렇게 시도해보는 과정에서도 단순히 개념을 적용하는데 의의를 두기 보다는, 왜 필요하다고 느꼈는지, 그래서 실제로 무엇이 더 좋아졌는지까지 그 트레이드 오프를 함께 설명할 수 있다면 설계에 대한 정당성은 훨씬 높아질 것 같아요.
- 또한 DOM 생성시 innerHTML을 전혀 사용하지 않고 createElement와 append만으로 구현했습니다. innerHTML은 문자열로 HTML을 삽입하기 때문에 XSS 위험이 존재하고, 기존 자식 노드의 이벤트 리스너가 사라질 수 있다는 점에서 지양했습니다. 대신 초기화가 필요한 경우 replaceChildren()을 사용하였습니다. 다만 creatElement 방식은 가끔 과하게 길어지는 것 같아서, 이 선택이 적절했는지 의견을 듣고 싶습니다.
innerHTML은 말씀해주신 위험성이 있어 대신 createElement를 사용하는 것이 일반적으로 권장되는 표준적인 방식이라, 좋은 선택이었다고 생각합니다.
그리고 이번 과제는 렌더러나 UI 라이브러리 자체를 만드는 과제는 아니다보니, createElement를 쓰면서 코드가 길어지는 것 자체는 문제라고 보지 않습니다. 지금 단계에서 그정도 복잡도는 충분히 감수 가능한 범위라고 생각해요.
[제안]
그치만 이런 부분이 정말 불편하게 느껴졌고, 직접 해결해보고 싶으시다면 여기서 한번 더 추상화를 시도해보는 것도 좋을 것 같습니다. 예를 들어 DOM 생성 과정을 조금 더 선언적으로 쓸 수 있게 헬퍼를 만든다거나, 공통적인 생성 패턴을 묶어보는 식으로 할 수 있을 것 같아요.
저 같은 경우에는 예전에 회사 인턴 과제를 할 때, react의 인터페이스를 참고해서 비슷한 형태의 함수를 직접 만들고, 내부적으로는 직접 구현한 간단한 가상돔과 dom 생성 헬퍼를 붙인 작은 라이브러리를 만들어서 과제에 이용했던 경험이 있어요. 요건 돔 생성 로직 자체를 내부로 감추고, 루트에서부터 렌더링 흐름을 제어할 수 있게 해서 뷰 레이어와 렌더링 사이클을 조금 더 선언적으로 다뤄보려는 의도로 도입했던 거였어요.
물론 지금 과제에서 여기까지 할 필요는 없다고 생각합니다만, 요런 불편함이 신경쓰이셨다면 제가 했던 것처럼 본인만의 방식으로 한 단계 더 추상화해보는 것도 좋은 연습이 될 것 같아요.
| #manager = new LottoManager(); | ||
| #priceForm; | ||
| #lottoList; | ||
| #winningForm; | ||
| #resultDialog; | ||
| #lottoListSection; | ||
| #winningSection; |
There was a problem hiding this comment.
[제안]
필드수가 좀 많아보이기는 한데, 하위 뷰 인스턴스랑 DOM 참조를 위한 변수들이라 크게 문제라고 생각되지는 않아요. WebView라는 중간 레이어를 둘까 하는 고민도 하셨다기에, 아마 이정도는 이 클래스에서 가져가도 괜찮은 크기의 책임이라고 판단하셔서 이렇게 넣어두신 것 같아요.
다만 이후 스펙이 커지면서 컴포넌트들이 많아지고, 컴포넌트간 의존성이 복잡해진다고 가정해봤을 때, 말씀해주신 WebView 레이어를 어떤식으로 구성해낼 수 있을지 구체적인 시나리오를 생각해보면 학습차원에서 좋을 것 같습니다👍👍
There was a problem hiding this comment.
음 스펙이 커지면..
만약 어떤 영역에 하위 컴포너느가 추가되서 하나의 클래스가 너무 많은 것을 알게되는것 같다고 판단이 된다고 한다면, 제 코드상에서는 컴포넌트의 마운트/언마운트와 DOM참조 관리정도는 WebView가 담당하고, WebApp이 도메인호출이랑 비즈니스 흐름제어에만 집중하는 식도 괜찮을수있을것같다고 생각합니다.
직접 구현해보지는 못해서 확실하지는 않지만, WebApp이 하나의 의도만 전달하고 WebView가 어떤 컴포넌트를 어디에 마운트할지 결정하는 느낌으로 가도 괜찮을까요?
| if ([...winningNumbers, bonusNumber].some(isNaN)) { | ||
| this.showError('[ERROR] 숫자를 입력해주세요.'); | ||
| return; | ||
| } |
There was a problem hiding this comment.
[권장]
WinningNumbersInputForm에서는 submit 시점에 isNaN 검사를 해서 에러를 직접 보여주고 있는데, PriceInputForm은 상위에서 에러를 처리하는 식으로 구현되어있더라구요. ( https://github.com/woowacourse/javascript-lotto/pull/470/changes#diff-edd7c61c0f0aeb7d30fd5be7d903dcd8244d6149bc5dcf5e8caeff1075bae9a2R43 )
둘 다 가능한 방식이긴 한데, 지금처럼 입력 폼마다 검증 책임의 위치가 다르게 잡혀있으면 장기적으로 코드가 파편화될 가능성이 있어요. 실무에서는 프로젝트 전체 구조와 맥락을 다 아는 사람이 계속 붙어있는 일이 드물고, 히스토리를 모르는 인원이 중간에 들어와 수정하는 경우도 많다 보니 요런 정책이 명확히 통일되어 있는 편이 유지보수에 좋다고 생각됩니다.
에러 노출 책임에 대한 정책을 통일하고, 좀 더 자연스럽다고 느껴지는 방향으로 수정해주시면 좋을 것 같아요.
There was a problem hiding this comment.
저는 이런 상황에서 너무 선택이 어렵습니다. 제가 한 선택이 맞는지 한번 봐주시면 감사하겠습니다.
우선은 데구리의 피드백으로 기준을 세워봤습니다. 뷰의 역할은 사용자 입력을 수정하고 에러를 표시하는 것이고, 입력값의 유효성을 판단하는 것은 뷰의 책임이 아닌것 같다고 생각했습니다. PriceInputForm은 지금 이미 이 기준대로 값만 상위로 전달하고 있었고, CLI의 App.js도 동일하게 도메인이 던진 에러를 컨트롤러가 캐치해서 출력하는 구조였는데요. WinningNumberInputForm만 내부에서 하는게 좀 부자연스럽다고 저도 생각이 들었습니다.
그치만 이런 상황에서 제가 갑자기 고민이 많아집니다. 제가 어디서부터 설계판단을 다시 내려보는게 좋을까요? 도메인 설계부터일까요? 무엇보다 지금 중요한 이슈인 WinningNumbersInputForm에서 제거한 isNaN 검사가 어디로 가야할까요?
일단 제가 생각해본 방법은 3가지로 수렴된다고 생각합니다. 먼저 도메인에서 잡는방법입니다. 현재 Lotto의 범위 검증이 n < 1 || n > 45 로 되어있는데, NaN과의 비교는 항상 false를 반환하기 때문에 NaN이 검정을 통과해버립니다. 이 부분을 좀 보강하는 느낌으로 (예를들어 !Number.isInterger(n) || n < 1 || n > 45) 한다면 모든 진입점에서 자동으로 보호되고, 검증 로직이 한 곳에만 존재하기때문에 가장 깔끔하긴 할 것 같습니다. 대신 도메인 변경이 수반되기때문에 선뜻 주저하게 되었습니다.
일단 두번쨰는 서비스에서 잡는 방법인데요, 도메인에 값을 넘기기 직전의 경계에서 검사하는 겁니다. 도메인은 건드리지 않으면서 CLI와 웹 모두에 LottoManger를 거치기 때문에 한 곳에서 보호할 수 있습니다. 근데 다만 뭔가 "숫자인지 확인하는 것?" 이게 서비스의 비즈니스 로직이 맞을까 고민을 오래했었습니다. 저는 뭔가 아니라고 생각해서 그런지 어색함이 조금 느껴졌습니다.
그럼 결국 컨트롤러에서 잡는건데 도메인과 서비스를 모두 건드리지 않을 수 있지만, 이러면 같은 검증 로직이 그럼 컨트롤러 두 곳에서 중복됩니다.
그래도 일단 저는 세번째 방법인 컨트롤러에서 잡는 방법으로 선택했습니다. step2 과제의 핵심을 조금 지키고 싶었던 이유가 컸는데요, 음 뭔가 이렇게 하고나서 보니까 이후에 진입점이 추가되면 같은 검사를 또 넣어야하고, 누락될 가능성도 커지게 만들어버리는 설계같다고 느껴졌습니다. 사실 근본적으로는 NaN이 도메인 검증을 통과하는 것 자체가 조금 문제가 있다고도 느껴지긴 했는데요, Lotto의 범위 검사를 Number.isInterger로 보강하면 컨트롤러의 중복 검증 없이도 모든 경로가 보호되고, 에러메세지도 하나로 통일해서 통과 할수있습니다. 이 부분에 대해 의견 주시면 많은 학습이 될 것 같습니다.
저는 이런 부분에서 선택하는것이 너무 재밌으면서도 어렵습니다. 이 부분에 의견을 주시면 많은 도움이 될것같습니다.
|
상세한 피드백 정말 감사합니다. 말씀해주신것처럼 그 반복하는 부분이 불편하게 느껴졌어서, DOM 생성 헬퍼 함수를 만들어 적용해보았습니다. 코드가 너무 절차적인것같아서 (만들고-속성주고-만들고-속성주고-붙이고), 순서를 따라가야 머릿속에서 조립할 수 있었던것같습니다. 이때 DOM 구조가 코드에서 바로 읽히는지를 기준으로 한번 리팩토링을 해보았습니다. 저는 데구리가 말하는 "좀 더 선언적으로 쓸 수 있게 헬퍼를 만드는 방향은 이렇게이지않을까" 라고 이해하고 시도해보았습니다. 시간상 말씀해주신 가상 DOM이나 랜더링 사이클 제어까지는 아직 시도하지 못했지만, 이번에 헬퍼를 직접 만들어보면서 왜 이런 추상화가 필요한지를 체감한것같습니다. 이후 스스로 주말내내 다양하게 추상화를 시도해보고 연습해보면서 공부해보겠습니다. 조언 정말 감사합니다! |
안녕하세요 데구리! step-1 리뷰가 정말정말정말 도움 많이되었습니다 감사합니다.
이번 step-2 리뷰도 잘 부탁드립니다...!
학습 목표
이번 미션을 통해 다음과 같은 학습 경험들을 쌓는 것을 목표로 합니다.
제출 전 체크 리스트
리뷰 요청 & 논의하고 싶은 내용
1) 이번 단계에서 가장 많이 고민했던 문제와 해결 과정에서 배운 점
웹 환경에서 View 레이어를 별도로 둘 필요가 있는지 고민했습니다. 처음에는 WebView라는 중간 레이어를 두려 했지만, 각 컴포넌트 자체가 DOM 생성과 랜더링 책임을 직접 갖는 구조에서는 중간 레이어가 간접층만 늘릴것 같다고 판단해서 제거했습니다.
컴포넌트 설계할때
mount(container)/unmount()패턴을 도입해서, 컴포넌트가 자신의 DOM 생명주기를 직접 관리해보도록 시도해보았습니다. 이벤트 전달은 생성자 콜백({ onSubmit }) 방식으로 처리하여 컴포넌트가 상위 로직을 몰라도 되도록 분리했습니다.또한 모든 DOM을 자바스크립트로 동적 생성하는 CSR 구조를 선택하면서 공통피드백 도중 SEO 측면의 한계를 인지했습니다. 크롤러가 HTML을 처음 받는 시점에는 빈 컨테이너만 존재하기 때문에 콘텐츠를 읽을 수 없습니다. 다만 이 프로젝트는 검색엔진 노출이 필요한 서비스가 아니기 때문에 현재 구조가 적합하다고 생각했습니다. SEO가 중요한 서비스라면 어떤식으로 구성하는게 좋을지, 그렇다면 SSR 방식을 고려해야하 하는지도 고민하게 되는 좋은 경험이였습니다.
추가로, 이번 미션에서 가장 중요한, 웹 전환시 도메인과 서비스 부분을 변경없이 구현하고자 노력했습니다..!
2) 이번 리뷰를 통해 논의하고 싶은 부분
스타일과 컴포넌트의 역할 분리 기준이 적절한가에 대해 피드백을 받고 싶습니다.
CSS 파일은 "변경 이유가 다른가"를 기준으로 분리했습니다. 하나의 파일에 모든 스타일을 두면, 변경 범위를 파악하기가 어렵고 의도치 않은 부수효과가 생길수도 있다고 판단했습니다. 브라우저 호환성, 디자인 시안, 타이포그래피 정책, 레이아웃은 각각 독립적인 이유로 바뀌기 때문에 파일을 나누면 수정 범위가 명확해지고 영향도를 예측하기 쉬워지지 않을까 라고 생각했습니다.
컴포넌트는 자신의 DOM을 직접 생성하고 마운트 / 언마운트를 관리하도록 설계해보았습니다. 그리고 컴포넌트 내부에서 인라인 스타일을 사용하지 않고 클래스명만 부여한 이유는, 시각적 표현을 CSS가 전담해야 컴포넌트 코드 수정없이도 디자인을 바꿀수 있다고 판단했습니다.
또한 DOM 생성시
innerHTML을 전혀 사용하지 않고createElement와append만으로 구현했습니다.innerHTML은 문자열로 HTML을 삽입하기 때문에 XSS 위험이 존재하고, 기존 자식 노드의 이벤트 리스너가 사라질 수 있다는 점에서 지양했습니다. 대신 초기화가 필요한 경우replaceChildren()을 사용하였습니다. 다만creatElement방식은 가끔 과하게 길어지는 것 같아서, 이 선택이 적절했는지 의견을 듣고 싶습니다.✅ 리뷰어 체크 포인트
1단계
2단계