[손호민] TS Todo Refactor 과제 #1
Conversation
There was a problem hiding this comment.
안녕하세요 호민님! 첫 코드리뷰 잘 부탁드립니다!! ( _ _ ) Pn 룰을 사용해서 리뷰하도록 하겠습니다!
일단 코드 전부 확인했는데 정말 깔끔하네요! 그나저나 TS파일은 직접 변환하신건가요..? vite나 특정한 라이브러리가 안보이길래.. 고생하셨습니당..
그리고 막상 실행해보니 다 JS파일로 연동되어 있더라고요? 그래서 Vite로 따로 프로젝트를 만들어서 TS파일로만 확인하고 호민님이 질문 사항에 남기신 것 코멘트 드립니다!
TS는 파일 확장자명을 안써줘도 됐던거 같은데 그렇게하고나니 .js로 자동추가가 안되더라구요 이거 해결방법 아시는분 공유좀..
요거는 제가 질문 의도를 정확하게 이해하지 못했는데 아마 tsconfig의 문제 아닐까 조심스레 의견 드립니다. .ts확장자는 따로 명시하지 않아도 ts파일로 간주하는데 특정 상황에서 .ts가 명시되지 않은 경우 변환 과정에서 문제가 생기는 것으로 추측됩니다. tsconfig 옵션이나 vsc 옵션 관련 문제일 것 같습니다. 물론 추측입니다..
원래 렌더링을 App의 constructor에서 하긴했었는데 이번에 각 렌더링을 메소드로 옮겨서 하려고하니까 메소드별로 생성되는 클래스의 인스턴스를 갖다쓰는게 안되더라구요..? 이것도 해결방법..아시는분 공유좀 해주시면 감사하겠습니다
요거는 저도 클래스에 대해 잘 모르겠어서... 조금 찾아봤는데 답이 잘 안나오네요ㅠ
메소드들에도 반환값이 없으면:void 있으면 return값에 대한 정의를 최대한 하려고 했는데 적정선에 대한 공유좀 부탁드리겠습니다
저는 개인적으로 return 문이 없는 함수라면 작성하는게 낫다고 생각합니다. 혹여나 실수할 수 있는 부분을 방지해줄 수 있으니까요.
props를 받을 땐 타입을 정의해주지않으면 오류가 나더라구요 근데 props를 내려줄 땐 오히려 붙이면 에러가 나서 이부분 알고계시면 공유좀 부탁드립니다.
오 이거 신기하네요! 요렇게 선언하니까 에러가 없습니다!! 바로 할당하는 것과 그렇지 않은 경우에 내부적인 코드에서 차이가 있나봐요!
new App({
$app: $app as HTMLDivElement,
initialState: todoInitialState as Todo[],
});
// App.ts 내부
$app: this.$app as HTMLDivElement,
todoInitialState: this.state as Todo[],원래 과제를 class로 했어서 전 리팩토링하는데 많은 어려움이 없었습니다..
개인적으로 클래스 잘 쓰는 것도 중요하다고 생각합니다. 저는 클래스를 엄청 못쓰거든요..ㅠㅠ 고생하셨습니다!
의도치않게 상태관리나 데이터 흐름에 대한 리팩토링이 된 것 같아서 많이 성장했다를 느꼈네요
호민님 예전 실력이 어땠는지는 모르겠는데 이번 코드를 보면 정말 깔끔하고 읽기 쉽더라고요!
types파일을 별도로 빼서 모두 모듈시스템으로 관리한 부분이 깔끔하게 잘 나온것 같다는 느낌을 받았네요
인정합니다! bb
src/App.ts
Outdated
| { | ||
| isCompleted: false, | ||
| title: text, | ||
| id: new Date().getTime().toString(), |
There was a problem hiding this comment.
id를 따로 날짜로 주셨군요? 좋은 방법이라고 생각됩니다!
src/components/TodoList.ts
Outdated
| this.$todoList.addEventListener('click', (event) => { | ||
| const target = event.target as HTMLElement; | ||
| const id = target.dataset.id as string | null; | ||
| if (id !== null) { |
There was a problem hiding this comment.
P5; if(id) 정도로 하면 조금 더 간단 명료하게 표현할 수 있을 것 같습니다!
src/main.ts
Outdated
|
|
||
| const todoInitialState = getItem(STORAGE_KEY, []); | ||
|
|
||
| // 여기서 as Todo[]를 안해도 에러가 발생하지 않지만 getItem의 반환값이 Todo[]라는 것을 |
There was a problem hiding this comment.
getItem 은 as Todo[]를 통해 반드시 Todo를 담고있는 배열이 반환되어야 하는데 defaultValue 는 빈배열이라 코드짜다가 그부분에 대해서 그냥 생각나서 주석으로 적어놓고 정리를 안했네요 ㅋㅋㅋㅋㅋ....
src/components/TodoList.ts
Outdated
| this.setState(nextState); | ||
| } | ||
|
|
||
| removeTodo(id: string): void { |
There was a problem hiding this comment.
P1; 호민님 이 부분에서 약간 로직의 문제가 있는 것 같습니다.
일단 배경 상황을 말씀드리면 TodoList목록에서 일부 Todo를 지우고, 그 상태에서 어떠한 동작 없이 다시 새로운 Todo를 추가하면 이전에 삭제했던 내용이 반영이 안되는 현상이 있습니다. 아마 최상단의 App.ts에서 관리하는 this.state를 변경해주는 로직이 없어서 그런 것 같다고 생각이 듭니다. 물론 호민님 코드대로 LocalStorage 상에서는 정상동작 하고 그래서 새로고침하면 정상적으로 동작합니다만 올바른 사용 흐름은 아닌 것 같아서 수정하시면 좋을 것 같습니다!
There was a problem hiding this comment.
타입변환하고 동작시켜보다가 뭔가 이상한데...? 하는생각을 하긴했었는데 ths.state에 대한 변경을 안넣어서 생긴 오류였군요.. 수정해보겠습니다 :)
| @@ -0,0 +1,31 @@ | |||
| export interface Todo { | |||
✅ PR 포인트 & 궁금한 점
:void있으면return값에 대한 정의를 최대한 하려고 했는데 적정선에 대한 공유좀 부탁드리겠습니다✅ 만족스러운 부분
types파일을 별도로 빼서 모두 모듈시스템으로 관리한 부분이 깔끔하게 잘 나온것 같다는 느낌을 받았네요