-
Notifications
You must be signed in to change notification settings - Fork 8
[react-todo-list step1] 신지훈 미션 제출합니다. #11
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
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.
안녕하세요. 지훈님, App.jsx로 컴포넌트를 옮기시다보니 zustand의 역할이 축소된 느낌입니다.
다음 최적화 작업이후에도 굳이 필요없어보인다면 빼도 괜찮을 것 같네요.
전반적인 피드백
typescript를 일부 적용시키신 점은 너무 좋았으나, 비중이 너무 적어 보이네요. 모든 파일에 적용시켜도 좋을 것 같아요.
text의 길이가 길어질 경우 ...으로 축약되게끔 했습니다.
해당 방식으로 축약한다면 아래와 같이 잘린부분을 확인하지 못하는 문제가 발생할 것 같습니다.
// 입력
그리디 리액트 과제 리뷰 - 원태연
그리디 리액트 과제 리뷰 - 임규영
// 출력
그리디 리액트 과제 ...
그리디 리액트 과제 ...
호버시 풀텍스트를 보여준다거나 글자수를 제한한다거나 다양한 개선방법이 존재할 것 같으니, 고민해보시고 개선해주시길 바랍니다.
코멘트 확인해주세요.
src/App.jsx
Outdated
const todos = useTodosStore((state) => state.todos); | ||
const setTodos = useTodosStore((state) => state.setTodos); |
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.
useTodosStore
를 두번호출할 필요가 있을까요?
const todos = useTodosStore((state) => state.todos); | |
const setTodos = useTodosStore((state) => state.setTodos); | |
const [ todos, setTodos ] = useTodosStore((state) => ([ state.todos, state.setTodos ])); |
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.
배열을 사용하여 셀렉터를 다루는 방식으로 리팩토링을 하니 Maximum update depth exceeded
문제가 발생했습니다. 해당 에러를 분석해보니 zustand는 내부적으로 useSyncExternalStore
를 사용하며, 이 훅은 getSnapshot
함수가 이전과 다른 참조를 반환하면 컴포넌트를 리랜더링 한다고 합니다.
현재 셀렉터는 매 렌더링마다 [state.todos, state.setTodos]
라는 새로운 배열 리터럴을 생성 및 반환합니다. 내부 state.~
의 값이 실제로 변하지 않았더라도 새로운 배열은 이전 배열과 다른 참조를 가지기에 zustand는 컴포넌트를 다시 렌더링하려고 시도합니다. 이 과정이 무한 반복되며 무한루프 문제가 발생한 것 같습니다. 셀렉터를 각각 구현하는 방식을 고수해도 되겠지만 해당 함수를 두 번 호출하는 방식은 다소 비효율적이라고 느껴집니다. 이에 대해 조금 더 탐색을 해보니 shallow
셀렉터라는 키워드가 있었습니다. 얕은? 비교라는 이름에서 유추 가능하듯이 각 요소에 대한 얕은 비교를 하여 무한루프를 방지하는 방법입니다. 구현은 [배열], shallow
로 간단히 될 것 같습니다.
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.
shallow 를 사용해도 여전히 에러가 발생하여 useCallback을 사용해주었습니다.
const [todos, setTodos] = useTodosStore(
useCallback((state) => [state.todos, state.setTodos], []),
shallow
);
이제는 해결이 될 줄 알았는데 여전히 에러가 발생합니다.
Warning: The result of getSnapshot should be cached to avoid an infinite loop Error Component Stack
at App
제가 놓치고 있는 부분이 뭘까요....? 힌트를 조금만 주시면 감사하겠습니다..
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.
버전이 업그레이드 되면서 바뀌었나보군요. useShallow를 사용해보시죠.
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.
해결되었습니다...감사합니다ㅜㅜ
src/App.jsx
Outdated
const updatedTodos = todos.map((todo) => | ||
todo.id === id ? { ...todo, checked: !todo.checked } : todo | ||
); | ||
setTodos(updatedTodos); |
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.
이전 상태에 의존하여 setState를 하는경우 현재의 방식은 문제가 될 여지가 있습니다.
어떤 문제가 발생할 수 있으며 어떻게 처리할 수 있을까요?
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.
updatedTodos에 값을 할당되고 setTodos 메서드가 실행되기 직전 상태가 업데이트 될 때 문제가 생길 것 같습니다.
const handleAddTodo = (text) => {
if (text.trim() === "") {
return alert("할 일을 입력해주세요!");
}
const newTodo = {
id: Date.now(),
text: text,
checked: false,
};
// setTodos([...todos, newTodo]);
setTodos((prevTodos) => [...prevTodos, newTodo]);
};
const handleCheckedTodo = (id) => {
// const updatedTodos = todos.map((todo) =>
// todo.id === id ? { ...todo, checked: !todo.checked } : todo
// );
// setTodos(updatedTodos);
setTodos((prevTodos) =>
prevTodos.map((todo) =>
todo.id === id ? { ...todo, checked: !todo.checked } : todo
)
);
};
const handleDeleteTodo = (id) => {
// const updatedTodos = todos.filter((todo) => todo.id !== id);
// setTodos(updatedTodos);
setTodos((prevTodos) => prevTodos.filter((todo) => todo.id !== id));
};
위 코드처럼 setTodos() 내부에서 기능 동작을 하면 될 것 같습니다만, 현재 useTodosStore 리팩토링과 더불어 에러가 발생하고 있습니다..
src/components/TodoList.jsx
Outdated
function TodoList({ todos, onCheckedTodo, onDeleteTodo }) { | ||
return ( | ||
<TodoListBlock> | ||
{todos.map((todo) => ( | ||
<TodoListItem | ||
key={todo.id} | ||
todo={todo} | ||
onCheckedTodo={onCheckedTodo} | ||
onDeleteTodo={onDeleteTodo} | ||
/> | ||
))} | ||
</TodoListBlock> |
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.
function TodoList({ todos, onCheckedTodo, onDeleteTodo }) { | |
return ( | |
<TodoListBlock> | |
{todos.map((todo) => ( | |
<TodoListItem | |
key={todo.id} | |
todo={todo} | |
onCheckedTodo={onCheckedTodo} | |
onDeleteTodo={onDeleteTodo} | |
/> | |
))} | |
</TodoListBlock> | |
function TodoList(props) { | |
return ( | |
<TodoListBlock> | |
{todos.map((todo) => ( | |
<TodoListItem | |
key={todo.id} | |
{ ...props } | |
/> | |
))} | |
</TodoListBlock> |
도 가능하겠죠?
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.
제안해주신 코드의 7번 라인을 {props.map((todo_ => (
로 수정을 해야할 것 같습니다. 기존 코드처럼 전달 받는 props의 종류가 많은 경우 이들을 props
라는 이름으로 한 번에 전달받고 다루는 방법이 인상적입니다. 하지만 map() 메서드가 배열에 사용이 가능한 메서드로 알고 있는데, 기존의 props였던 todos, onCheckedTodo, onDeleteTodo
들을 -> props
로 한 번에 전달을 받더라도 map() 메서드 적용이 안 되지 않을까? 하는 의문이 듭니다.
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.
넵 말씀주신부분이 맞습니다. todo는 props로 넘겨주셔야합니다. 👍
민석님 안녕하세요! 아마도 step_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.
지훈님 타입스크립 적용까지 하시느라 고생하셨습니다. 👍
Q&A
본론) 중복되는 인터페이스에 대해서 해결 방법이 있을지, 인터페이스를 하나의 파일에서 관리하는게 좋을지 혹은 해당 인터페이스 내에 구현하는게 나은지(관리 측면)
기초적인 질문이라 다소 아쉽군요. 코멘트에 남겨두었습니다.
main.tsx로 변경을 하며 create(document.getElementById("root")에 null 관련 오류가 발생했습니다. 일단은 null 일 경우 Error 를 터뜨리는 코드를 추가하여 null 에 대한 방어를 했습니다. 코드의 변화가 거의 없는데 문제가 없던 create() 구문에 갑자기 null 관련 오류가 뜬 이유가 궁금합니다. 오류 구문에서는 HTMLElement를 언급하는 것을 보니 TodoInsertProps 내 핸들 함수의 이벤트의 타입을 정의할 때 사용한 타입 때문인 것 같긴 합니다..
null 관련 오류
라고 말씀하시면 저도 이해하기가 어렵네요. 캡쳐나 에러메세지라도 첨부 부탁드립니다. 우선은 조금 더 디깅해보시고 해결이 되셨다면 저에게 알려주세요~
PR을 Merge할 시간이 얼마 남지 않아 여기서 마무리하는게 좋을 것 같네요.
코멘트 확인해주시고, 다음 PR에서 답변부탁드려요~
@@ -0,0 +1,61 @@ | |||
import styled from "styled-components"; | |||
|
|||
interface StyleProps { |
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.
StyleProps라는 네이밍이 적절한지 의문입니다.
이정도는 인라인으로 처리하는게 더 가독성에 좋을 것 같네요.
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.
반영했습니다!
checked: boolean; | ||
} | ||
|
||
interface TodoListItemProps { |
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.
TodoListItem 컴포넌트의 props 타입임을 바로 알 수 있도록 잘 네이밍 해주셨습니다. 👍
MdRemoveCircleOutline, | ||
} from "react-icons/md"; | ||
|
||
interface Todo { |
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 에서도 해당 타입을 관리하고 있습니다.
타입도 import해서 쓸 수 있으니 재활용하는건 어떨까요?
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.
맞습니다. 이 부분은 코멘트 남긴 후에 좀 더 고민해보니 너무나 당연한(?) 방법으로 해결이 가능하더군요.. 바로 반영하겠습니다!
} | ||
|
||
function TodoInsert({ onAddTodo }: TodoInsertProps) { | ||
const [value, setValue] = useState(""); |
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.
value의 타입이 어떤 타입인지, useState의 제네릭을 활용하여 처리할 수 있어요.
const [value, setValue] = useState(""); | |
const [value, setValue] = useState<string>(""); |
하지만 현재는 따로 제네릭으로 타입을 지정해두지 않았음에도 string을 추론하고 있습니다.
무슨 이유 때문일까요?
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.
타입스크립트가 초기값에 해당하는 ""
을 자동으로 string이라 추론하기 때문입니다. 좀 더 찾아보니 ""와 같은 리터럴 문자가 초기값으로 사용될 때는 더 넓은 타입인 String으로 확장되어 추론된다고 합니다.
하지만 이번에는 운이 좋게도(?) 제가 생각한 초기값("", 공백)과 실제로 들어가야 하는 값(string)이 맞아떨어졌다고 생각합니다. 또 타입스크립트에서 타입을 검사해주고 확장되어 추론이 되는 것은 좋으나 제 코드를 처음 보는 개발자를 고려하여 <string>
명시하는 것이 좋을 것 같습니다!
PR comment 🥝
김민석 리뷰어님 안녕하세요! 그리디 첫 미션과 마지막 미션을 함께 하게 되다니,, 영광입니다! 이번 미션(step_1)은 기존에 해왔던 다른 미션들에 비해 새롭게 배우고 적용하는 부분이 없었습니다. 그래서 욕심을 내서
Typescript
를 적용시켜봤습니다. 확실히 타입을 정해주는 방식이 여러모로 좋은 것 같습니다만 타입스크립트가 익숙치 않은터라 상태 관리를 하는todoStore
만ts
로 구현했습니다.jsx
파일 또한 전부tsx
로 통일시키는 방식이 좋았을까요..?상태 관리는 저번 미션에서 처음 도입해본
zustand
를 한 번 더 사용했습니다. todo와 관련된 상태는 요구사항과 같이 해당 스토어에서 관리하도록 구현을 했고,TodoInsert
에서 입력값(Input)에 해당하는 value는 전역 상태로 관리하는 것보다 해당 컴포넌트에서 지역 상태로 관리하는 것이 더 적합한 것 같아서useState
로 구현했습니다.1️⃣ Step1 요구사항
App.jsx
내에 다시 구현했습니다.todoStore.ts
내에 인터페이스로 타입 지정을 했습니다..컴포넌트 설명
TodoTemplate
화면을 가운데에 정렬시켜주며, 앱 타이틀(일정관리)를 보여줌. children으로 내부 JSX를 Props로 받아와 렌더링합니다.
-> 그리디 이미지와 "일정관리" 제목을 보여주고 Todo 앱 전체 틀 역할을 합니다.
TodoInsert
새로운 항목을 입력하고 추가할 수 있는 컴포넌트. state를 통해서 인풋의 상태를 관리합니다.
-> "할 일"을 입력 받고
useTodoStore
내에서 정의한addTodo
액션 함수를 통해 상태를 업데이트 합니다. 입력값에 대해서는 지역상태로 TodoInsert 컴포넌트에서만 사용하도록 구현 했습니다.TodoListItem
각 할 일 항목에 대한 정보를 보여주는 컴포넌트. Todo 객체를 props로 받아와 상태에 따라 다른 스타일의 UI를 보여줍니다.
-> 기존 음식점 미션의 RestaurantCard와 같이 틀은 재사용 가능하며 내부 값만 todo에 따라 전달받는 형식으로 구현했습니다.
TodoList
todo배열을 props로 받아 온 후, 여러개의 TodoListItem 컴포넌트로 변환한 후 보여줍니다.
-> 앞선 아이템을 리스트로 형태로 보여줍니다.
구현에 신경 쓴 부분
Date.now()
를 사용하여 중복된 id 값을 갖지 않도록 했습니다.TodoTemplate.styled.js - AppContainer
)TodoListStyled.js - TodoListBlock - overflow-y
)...
으로 축약되게끔 했습니다. (TodoListItem.js - Text - text-overflow
)시현 영상
video2.mov
** 항상 바쁜 시간 내서 리뷰해주셔서 감사합니다! 이번 미션과 더불어 다음 step2 미션도 잘 부탁드립니다!!