Skip to content
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

Merged
merged 24 commits into from
Sep 4, 2019
Merged

Conversation

pocokim
Copy link

@pocokim pocokim commented Sep 1, 2019

배포링크 : https://dxvinci.github.io/react-todo/dist/

1. 진행사항

1. Proptypes 반영

  • Todoitem
  • StateProvider

2. react-router 를 활용하여 메뉴탭 구성

  • About , Home 컴포넌트는 jsx로 만듦( js 벗어나고 싶어서 ㅎ.ㅎ jsx가 코드짤때 더 편한것같습니다. 단축키가 지원되서요..)

3. useMemo , useCallback , + 각종 성능개선

2. 아쉬운 점

  • react-router를 이용하여 메뉴탭을 구현하였지만 Route 컴포넌트의 props인 history,match,location을 사용할 부분이 없어서, 공부하지 않았고 너무 react-router를 대충쓰고 넘어간것같아서 아쉽습니다
  • Profiler 탭에서 무엇에 의해 render 되는지 알면 더 좋을듯한데 어떻게 알아봐야할지 몰라서 ㅎㅎ... console찍어서 확인하였습니다
  • useCallback과 useMemo를 사용하기에 적절한 사례가 아니었던것같습니다. (contextAPI를 사용했기에 props로 값이나 함수를 넘겨주지 않기때문)
  • Profiler 탭에서 reload하고 재실행할때마다 시간이 다르게 측정되어 최적화가 됬는지 확인하기 어려웠습니다

3. 느낀점

  • 라우팅을 적용하고싶은데 많이 파지 못했고 더 공부해보고싶은데 적절한 예시가 무엇이 있을지 모르겠습니다.
  • 최적화는 재미있는것같은데, profiler 로 레코딩을 할때마다 시간이 다르게 나와서.. 어떻게 동일한조건으로 맞춰주고 측정해야할지 모르겠습니다. 그냥 탭에서 색깔보면서 리랜더링 유무를 파악한.. 무식한 방법을 사용한것같습니다.
  • Proptypes 가 이미 정해져서 넘어오는 값이라서 그런지.. 사용했을 때의 큰 이점이 무엇일지 모르겠습니다.

Readme에 Profiler로 컴포넌트를 분석하고 최적화를 어디서부터 할지 설계하고 진행한과정을 적어보았습니다.
감사합니다! 🙇‍♂️

- 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를 사용할 필요가 없음(본질적인 해결책이 아님)
Copy link
Contributor

@crongro crongro left a 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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(핫핑크..)

<StateContext.Provider value={{ state, dispatch, isLoading }}>
{children}
</StateContext.Provider>
<TodosStateContext.Provider value={todos}>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

context를 여러새 중첩된 코드로 만든 이유가???
불필요하게 랩핑된 객체가 많아진 결과라서 디버깅도 어렵고 장점이 없을거 같아요.

Copy link
Author

@pocokim pocokim Sep 2, 2019

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가 리랜더링이 되지 않게 하는방법이 있는지는 잘 모르겠습니다 ㅠ

`;

export default function ResultBar() {
const todos = useTodosStateValue();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useContext를 여기서 직접 부르는 것과 달리, 이코드가 어떤 장점이 있는지 전 잘 모르겠어요.
괜한 함수하나를 더 둔거 같기도같고요. 장점이 뭘까요?

Copy link
Author

@pocokim pocokim Sep 2, 2019

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하는게 더 이해가 쉬워서... 이렇게 구현했고 지금도 이렇게 쓰고있는것같습니다 ㅎ....)

export default function ResultBar() {
const todos = useTodosStateValue();

const countStatus = status => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

큰 차이는 아니겠지만, 이런코드를 useMemo로 결과를 캐시하면 어때요?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

리랜더링이 이미 최소한으로 일어나... useMemo를 사용해도 캐싱이 일어나지 않고, 성능개선을 기대할 수 없다고 생각했습니다.
useMemo와 useCallback으로 캐시하는것보다, 리랜더링을 최소한으로 만드는것이 더 중요하다고 생각하였습니다!

</Folder>

<TodoWrapper isOpened={isOpened}>
{isLoading && <li>로딩중...</li>}
{!isLoading && makeTodoList(state.todos)}
{isLoading === true && <li>로딩중...</li>}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true와 비교는 필요가 없겠죠.

{isLoading &&

  • 로딩중...}

  • Copy link
    Author

    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>
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Router기능은 별거 없어요.
    SPAs가 더 복잡한 상태가 된다면 router기능도 좀더 활용하게 되겠죠.
    예를들어 인증이 붙는다던가 등등

    Copy link
    Author

    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} />
    Copy link
    Contributor

    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"} 굳이 한번더 감싸지 않아도됨
    Copy link
    Contributor

    @crongro crongro left a 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를 사용하기에 적절한 사례가 아니었던것같음.
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    추가할만한 기능하나만 아이디어 좀 주시면 감사..!

    Copy link
    Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    넵 생각해보겠습니다!

    Copy link
    Contributor

    @crongro crongro left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    버그도 수정하고, 잘 동작되네요.

    성능개선을 공부한김에 블로그(?)를 써보는 것을 추천!

    // http://localhost:3001/heart/about
    router.get('/list',function(req,res){
    // send와 json 차이는 뭐지?
    res.send({
    Copy link
    Contributor

    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 @@
    {
    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    분리 잘 했음

    Copy link
    Contributor

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    mongo db 연동도 해보면 좋겠죠.
    NoSQL이 연동하기 쉽더라고요.

    Copy link
    Author

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    step 21 인증 하면서 기회가 되면 db도 구현해보겠습니다!

    @crongro crongro merged commit 5394766 into code-squad:wo0kgod Sep 4, 2019
    @pocokim
    Copy link
    Author

    pocokim commented Sep 4, 2019

    버그도 수정하고, 잘 동작되네요.

    성능개선을 공부한김에 블로그(?)를 써보는 것을 추천!

    블로그글도 써보겠습니다!

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants