-
Notifications
You must be signed in to change notification settings - Fork 14
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
[wook] Step19 리뷰 부탁드립니다. #34
Conversation
- isLoading도 state이므로 명시적으로 todos로 변경하였다. 이에따라 todosReducer도 state객체안의 todos 키의 배열이 아닌 todos배열로 변경하여 return을 객체가 아닌 배열로 변경하였음.
- 결과는 todoCnt , doneCnt로 state로 관리할필요가없음 어짜피 출력만 하는것이기때문
1. props를 {...todo}로 넘기도록 변경 2. propTypes에 뎁스가 있는경우 shape를 사용할 수 있음.
궁금증 - state는 타입체크 안하나?
하나의 Provider에서 value객체를 받아서 관리하는것이 아닌, 각각 Provider별 value를 분리하고 커스텀 훅을 만들어서 InputBar , ResultBar 리랜더링을 막음. 이렇게 구현하면 Home , ResultBar에서 useMemo를 사용할 필요가 없음(본질적인 해결책이 아님)
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.
먼저 여러가지 적용을 위한 시도가 좋네요.성능개선을 위한 고민도 보이고요.
라우터는 아직 복잡한 애플리케이션이 아니라서 그럴수 있어요.
인증등이 포함되면 redirect도 하고 그러면서 좀더 다양한 기능을 쓸 수 있겠죠.
성능개선도 역시복잡도가 올라갈수록 그 의미가 크죠. 어떤 부분을 왜 개선해야할지 고민하고 그 효과를 가급적 확인하는 게 좋아요. 개발자도구의 component / profiler 패널을 좀더 들여다보시죠.
그리고 기능을 버그가 좀 있어요.
- 처음 접속하면 아무것도 노출되지 않는 문제
- 완료된 일로 표시하고, HOME에 갔다가 다시 할일목록으로 오면 완료표시가 안되는 문제.
요건 버그라서 수정을 하고 넘어가면 좋겠어요.
src/Home.jsx
Outdated
p { | ||
margin: 0; | ||
span { | ||
color: hotpink; |
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/StateHelper/TodoState.js
Outdated
<StateContext.Provider value={{ state, dispatch, isLoading }}> | ||
{children} | ||
</StateContext.Provider> | ||
<TodosStateContext.Provider value={todos}> |
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.
context를 여러새 중첩된 코드로 만든 이유가???
불필요하게 랩핑된 객체가 많아진 결과라서 디버깅도 어렵고 장점이 없을거 같아요.
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를 {state, dispatch, isLoading} 객체로 만들경우 state와 isLoading이 바뀔때마다 state와 isLoading을 받는 InputBar( 입력에 관한 컴포넌트 ) , ResultBar ( 결과창을 출력하는 컴포넌트) 가 리랜더링이 일어납니다.
그러나 context를 위와같은 구조로 만들경우 isLoading이 바뀌어도 InputBar는 리랜더링이 일어나지 않아서.. 이렇게 구현해보았습니다.
https://github.com/facebook/react/issues/15156의 첫번째 답변인 Option 1 (Preferred): Split contexts that don't change together 에 맞게 구현했다고 생각하였는데.. 확실히 가독성이 떨어지기는 하는것같습니다. 하지만.. 랜더링횟수는 확실히 줄어서요 ㅠㅠ 저는 이 방법이 최적화 하는 방법이라고 생각하구... 여러개 중첩하지 않고 isLoading이 바뀔때 InputBar가 리랜더링이 되지 않게 하는방법이 있는지는 잘 모르겠습니다 ㅠ
src/Todo/Header/ResultBar.js
Outdated
`; | ||
|
||
export default function ResultBar() { | ||
const todos = useTodosStateValue(); |
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.
useContext를 여기서 직접 부르는 것과 달리, 이코드가 어떤 장점이 있는지 전 잘 모르겠어요.
괜한 함수하나를 더 둔거 같기도같고요. 장점이 뭘까요?
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 React, { useContext } from 'react'
import { TodosStateContext } from "../../StateHelper/TodoState";
로 모듈에서 두 함수를 불러오는것보다 차라리 TodoState에서 useTodosStateValue라는 함수를 만들어서 결과값만 리턴해주는게 더 깔끔하다고 생각했기 때문입니다.
새로운 함수를 만드들고 export 하는것 vs ResultBar.js 에서 두 함수를 Import 하는것의 trade-off라고 생각했습니다. (그런데 처음에 useContext를 공부할땐.. TodoState에서 새 함수를 만들고 export하는게 더 이해가 쉬워서... 이렇게 구현했고 지금도 이렇게 쓰고있는것같습니다 ㅎ....)
src/Todo/Header/ResultBar.js
Outdated
export default function ResultBar() { | ||
const todos = useTodosStateValue(); | ||
|
||
const countStatus = status => { |
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.
큰 차이는 아니겠지만, 이런코드를 useMemo로 결과를 캐시하면 어때요?
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.
리랜더링이 이미 최소한으로 일어나... useMemo를 사용해도 캐싱이 일어나지 않고, 성능개선을 기대할 수 없다고 생각했습니다.
useMemo와 useCallback으로 캐시하는것보다, 리랜더링을 최소한으로 만드는것이 더 중요하다고 생각하였습니다!
src/Todo/TodoList.js
Outdated
</Folder> | ||
|
||
<TodoWrapper isOpened={isOpened}> | ||
{isLoading && <li>로딩중...</li>} | ||
{!isLoading && makeTodoList(state.todos)} | ||
{isLoading === true && <li>로딩중...</li>} |
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.
true와 비교는 필요가 없겠죠.
{isLoading &&
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.
isLoading === true
이렇게 작성하는게 저는 더 명시적으로 쉽게 보이는것같아서 ㅎ.ㅎ 요렇게 작성하였습니다
src/TodoApp.js
Outdated
<InputBar /> | ||
<TodoList /> | ||
</StateProvider> | ||
<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기능은 별거 없어요.
SPAs가 더 복잡한 상태가 된다면 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.
넵 ㅎㅎ!
src/TodoApp.js
Outdated
<Router> | ||
<StateProvider> | ||
<Navbar /> | ||
<Route exact path="/home" component={Home} /> |
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.
참고로,
Switch 컴포넌트로 라우팅을 하는것과 차이도 알아보세요.
404처리용 라우터도 알아보면 좋고요.
how to redirect path "/" to "/home" react-router 루트를 /가 아니라 /home으로 만들수는 없나? 그렇게 하는게 안좋은가? 라는 고민이 있음. - redirect로 /로 들어온걸 /home으로 보내는 방법은 있지 않을까?
IsClicked 변수와 setIsClicked 대신 props에서 status를 받아서 status가 "todo" 인지 "done" 인지에 따라 다른 style 적용
서버와 분리하기 위해 폴더구조 변경
하드코딩된 포트번호를 port 변수이용하여 변경
- send와 json 차이는 뭐지? - router 변수의 역할은 뭐지? - const 왜 안쓰지? -- json파일 어떻게 읽어오지?
1. fs가 아닌 require로 읽도록함 2. 굳이 router에서 /list라는 하위 url을 두어 구성 ( 시험하고싶어서 해봄) - fs로 읽는것의 2가지 장점. require은 동기적으로 읽어옴, 이렇게 지정한 파일은 Json밖에 못읽음 , 근데 귀찮음 - https://stackabuse.com/reading-and-writing-json-files-with-node-js/
1. webpack.config.js 파일 enviroment 변수 수정 2. DefingPlugin에 ACCESS_PATH 추가 ( json.stringfy로 감싸주어야함) 3. TodoApp 컴포넌트에서 Route, Link 태그 수정 path={ACCESS_PATH+ "about"} path={`${ACCESS_PATH}`+"about"} 굳이 한번더 감싸지 않아도됨
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.
버그해결하면 커밋로그에 보통 (fix) 라고 표현해요.
|
||
### 2.3 아쉬운 점 | ||
- Profiler 탭에서 무엇에 의해 render 되는지 알면 더 좋을듯.. 몰라서 console찍어서 확인함 | ||
- 과제가 useCallback과 useMemo를 사용하기에 적절한 사례가 아니었던것같음. |
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.
넵 생각해보겠습니다!
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.
버그도 수정하고, 잘 동작되네요.
성능개선을 공부한김에 블로그(?)를 써보는 것을 추천!
server/todoApp.js
Outdated
// http://localhost:3001/heart/about | ||
router.get('/list',function(req,res){ | ||
// send와 json 차이는 뭐지? | ||
res.send({ |
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.
json응답이면 json을 사용하는 것이 좀더 좋은거 같네요.
응답헤더에 content-type을 application/json등으로 설정해주는 등의 적절한 조치를 해줄 듯.
@@ -0,0 +1,30 @@ | |||
{ |
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.
mongo db 연동도 해보면 좋겠죠.
NoSQL이 연동하기 쉽더라고요.
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.
step 21 인증 하면서 기회가 되면 db도 구현해보겠습니다!
블로그글도 써보겠습니다! |
배포링크 : https://dxvinci.github.io/react-todo/dist/
1. 진행사항
1. Proptypes 반영
2. react-router 를 활용하여 메뉴탭 구성
3. useMemo , useCallback , + 각종 성능개선
수 있는 부분이 없었습니다.
https://ideveloper2.dev/blog/2019-06-14--when-to-use-memo-and-use-callback/
https://kentcdodds.com/blog/how-to-optimize-your-context-value
2. 아쉬운 점
3. 느낀점
Readme에 Profiler로 컴포넌트를 분석하고 최적화를 어디서부터 할지 설계하고 진행한과정을 적어보았습니다.
감사합니다! 🙇♂️