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

[페이먼츠 미션 Step 3] 루루(송지은) 미션 제출합니다. #286

Merged
merged 20 commits into from
May 8, 2023

Conversation

hafnium1923
Copy link

@hafnium1923 hafnium1923 commented May 5, 2023

안녕하세요 리트👍
좋은 연휴 입니당ㅎㅎ
잘 지내고 계신가요? 저는 가족여행 와서 열심히 코딩 중이에요ㅋㅎㅋㅎ
이번 미션은 페이지 구현이 어렵진 않더라구요!
npm 배포하는게 더더 어려울 만큼..!
그래서 이번에는 최적화 공부를 약간 해보았어요 하지만 감이 잘 잡히진 않네요..
마지막 step3 도 잘부탁드립니다🫧🫧


미션페이지

payments

아이폰 12pro로 볼때 가장 이쁩니다.

Storybook

CardLoadingPage에서 스피너가 좀 깨지네요..^^;;
미션페이지에서는 안그래요!

모달 npm 사이트

모달 repository

열심히 readme 작성했어욧!><😆😆


구현한 기능

기능

  • 모달 NPM 배포
    • 배포된 모달을 사용할 사용자를 위한 readMe 작성
    • 배포한 모달을 실제로 사용
  • 카드 스피너 화면 구현
    • 카드 별칭을 입력하고 확인을 누르면 카드 스피너 화면이 보인다
    • 카드 등록이 완료되면 카드 목록으로 이동한다.
    • 만료일에서 잘못된 월 입력시 폼 제출 막

요구사항

  • 사용하던 모달을 분리해서 npm으로 배포하고, 그 라이브러리를 직접 import해서 사용하기
  • 문서로서 스토리북을 고도화하기 위해 리팩터링
    • 각 스토리에 명확한 이름을 지정하고, 스토리 이름을 통해 컴포넌트의 사용 사례를 쉽게 이해할 수 있도록 한다.
    • 변동 가능한 값에 대해 사용자가 직접 조작해볼 수 있게 하여 컴포넌트를 더욱 쉽게 이해할 수 있도록 한다.
  • '카드를 등록중입니다' 스피너 추가

새로운 도전!

  1. useCallback사용
    useCallback은 특정 함수를 새로 만들지 않고 재사용하고 싶을 때 사용하는 훅입니다.
    컴포넌트가 리렌더링 될 때마다 함수들이 새로 만들어진다고 알고있기 때문에 함수들의 재사용성을
    높이기 위해 몇가지 훅에 useCallback을 적용해보았습니다. 단순 setState를 하는 함수들은 제외하고 조금 복잡한 로직이 있는 함수 중에 return으로 컴포넌트에 전달되는 함수에 적용해보았습니다.

  2. 바텀시트 npm 배포
    바텀시트를 분리해 모듈로서 배포해보았습니다.
    최대한 범용성이 높게 하고 싶어서 노력했습니다!
    관련 코드와 readme는 위에 있는 레포지토리 링크를 클릭하시면 확인 가능합니다.


질문

  1. useCallback 사용
    deps 배열안에 사용하는 상태를 넣어야 한다고 배웠는데, 사실 제가 useCallback으로 감싼 함수들이 안에서 상태를 변경하는 함수기 때문에 useCallback 훅을 이렇게 적용하는 것이 의문이 듭니다. 예를들어 useCardNumber 훅에서 changeCardNumber 메서드만 useCallback으로 감싸고 있는데 이 메서드는 두가지 상태 (cardNumberOrigin, cardNumberHidden) 모두 새로 지정하는 함수이기 때문에 useCallback을 사용하는 의미가 없는것이 아닐까? 하는 의문이 듭니다. 대신 deleteNumber라던가 chageHidden을 감싸는게 맞다는 의문도 들구요..! setState 함수를 포함하는 함수에 useCallback을 사용하는것인지, setState를 사용하지 않고 state를 이용해 새로운 결과물을 만드는 함수에 useCallback을 사용하는 것인지 궁금합니다.

  2. React.Memo 사용
    인풋하나만 바뀌었을 때 아래 인풋들도 리렌더링 되는것이 최적화에 알맞지 않다고 생각해서 input컴포넌트 모두 React.memo를 적용했었습니다. 하지만 react 개발자 도구를 활용해 업데이트가 되지않은 인풋 컴포넌트가 렌더링 되지 않는지 확인해보니 React.memo를 사용했더라도 하나의 인풋이 바뀌면 모두 렌더링 된다고 화면에 표시되더라구요. 이게 context API로 Form전체를 감싸고 있기 때문에 그런건지, 아니면 제가 잘못사용하고 있는지 궁금합니다.

  3. 바텀시트
    저는 바텀 시트에서 3가지 props를 받고있습니다. 이중 세번째 props인 buttons에 관련된 질문입니다. 저는 바텀시트를 분리하면서 최대한 관심사가 분리되고, 범용성이 높길 바랬습니다. 따라서 바텀시트를 열고 닫는 것과 관련된 것은 바텀시트 안에 있습니다. 하지만 미션에서는 카드사 이미지를 선택하면

  • 카드사가 바뀌는 메서드
  • 바텀시트가 닫히는 메서드
    이 두개가 작동해야합니다. 원래는 바텀 시트의 열리고 닫히는 상태도 바텀 시트가 아닌, 위 컴포넌트에서 관리했기 때문에 이미지 버튼의 onClick으로 두가지 메서드를 묶어서 보냈습니다. 하지만 바텀시트를 분리하니 더이상 이미지 버튼에 바텀 시트를 닫는 메서드를 추가할 수 없게 되었습니다. 그래서 차라리 닫기 기능을 여러개 추가할 수 있고, 최대한 재사용성이 높도록 모달을 만들자 라는 생각이 들어
{buttons.map((button) => {
     return (
          button &&
          React.cloneElement(button, {
                onClick: () => {
                   if (button.props.onClick) button.props.onClick();
                   closeBottomSheet();
                },
            })
      );
}

이렇게 buttons 하나하나 map 돌려서 강제로 onClick에 원래 onClick메서드와 닫기 메서드를 추가하게 했습니다.
이런 방식 말고 모달을 사용할 때 모달의 닫기 메서드를 children으로 들어오는 요소나 props로 들어오는 요소에 추가하는 방법이 궁금합니다.


느낀점

이번 스텝은 크게 어렵진 않았던 것 같습니다. 하지만 새롭게 시도해본 npm 배포라던가, 최적화에 대해 생각하는 것이 약간 어려웠던 것 같습니다. useCallback, useMemo를 사용하며 여러 방법으로 리렌더링을 줄이려고 했는데 어떤 방식으로도 개발자 도구의 react 랜더링 표시되는 곳에서 계속 렌더링이 되고 있다고 보이더라구요.. 그래서 어떻게 사용하고, 어디에 사용해야하는지 감이 안잡혔습니다.. 최적화가 많이 아쉬움이 남는 step이었어요! 아 그리구 step2에서 말씀해주신 context쪽 console 지우기, data속성 활용하기 등등은 적용했습니다ㅎㅎ!
리트 남은 연휴도 행복하게 보내세용~~

@lsw1164 lsw1164 self-assigned this May 5, 2023
Copy link

@lsw1164 lsw1164 left a comment

Choose a reason for hiding this comment

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

루루, 가족 여행 중에도 코딩하셨다고요?
그래서 이렇게 빠르게 제출할 수 있었던 건가요 😂

질문에 대한 답변들은 코멘트에 남겼으니 확인해주세요!

console.log(e);
},
// eslint-disable-next-line
changeCardNumber: (e: React.ChangeEvent<HTMLInputElement>) => {},
Copy link

Choose a reason for hiding this comment

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

다른 제안을 드려봅니다.

먼저 해당 이벤트는 필수 이벤트인가요?

  1. 필수 이벤트인 경우: 기본값으로 Not Implement Error를 발생시킨다.
  2. 옵션 이벤트인 경우: 타입을 optional로 정의하고 기본값은 전달하지 않는다.

이렇게 처리하면 어떨까요?

const cvc = toOnlyNumber(e.target.value).slice(LENGTH.ZERO, LENGTH.CVC);
setCardCVC(cvc);
},
[cardCVC]
Copy link

Choose a reason for hiding this comment

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

cardCVC는 필요한 의존성일까요?🤔
useCallback 내부에서 사용하는 부분이 없어요.

Copy link

Choose a reason for hiding this comment

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

혹여나 cardCVC를 내부적으로 사용한다고 하더라도 updater function을 사용하면 의존성이 아예 필요 없죠.

https://react.dev/reference/react/useCallback#updating-state-from-a-memoized-callback

최적화에 대한 시도는 정말 좋습니다 👏
공식 문서를 참고해보면 useCallback의 모범 사례를 확인할 수 있어요😁

한 번 공부해보시고 useCallback을 사용한 부분들 다시 살펴보면 좋겠어요!

Copy link
Author

@hafnium1923 hafnium1923 May 8, 2023

Choose a reason for hiding this comment

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

확실히 사례가 있으니 이해에 도움이 되는 것 같습니다. 알려주신 문서에서 제가 생각하는 가장 중요한 부분은

  1. useCallback과 같은 메모화 함수를 사용할 땐 가능한 적은 의존성을 가지는 것이 바람직함
  2. 커스텀 훅에서 반환 함수 모드 useCallback으로 감싸는 것이 좋음-> 소비자가 필요할 때 최적화를 할 수 있음
  3. 컴포넌트 리렌더링에 관련해서도 useCallback으로 감싼다면 건너뛰기가 가능

인 것 같습니다. 감을 좀 잡은 것 같아서 useCallback을 추가적으로 적용해보았습니다! 또한 지금 써둔 것은 약간 의존성 배열을 남발한 것 같아서..ㅎ 수정도 했습니다!

Copy link

Choose a reason for hiding this comment

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

질문에 고민하신 Context를 사용할 때 사례가 있군요.
읽어보면 좋겠습니다 😁

https://react.dev/reference/react/memo#updating-a-memoized-component-using-a-context

Copy link
Author

@hafnium1923 hafnium1923 May 8, 2023

Choose a reason for hiding this comment

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

오,, 컴포넌트에 memo를 적용하여 리렌더링을 제어하고 싶으면 context값을 상위 컴포넌트에서 읽고 하위컴포넌트로 props를 통해 전달해야하는 거군요! 하지만 여전히 궁금한 부분이 몇가지 있습니다.

  1. 하위 컴포넌트에서 props를 통해 상태를 전달받을 때만 memo가 의미있어지는 것일까요?
  2. 그렇다면 상태를 사용하지 않는 컴포넌트들이 렌더링 되는것을 막고싶다면 Passing JSX as children 의 방식으로 해야하는 걸까요?
  3. 제가 이해한 것을 바탕으로 본다면 현재 코드의 CardDetailView는 상위 컴포넌트에서 상태를 props의 형태로 받아서 사용하고 있기 때문에 memo를 적합하게 사용하는 것으로 보이는데 제 판단이 맞을까요?

Copy link

Choose a reason for hiding this comment

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

React normally re-renders a component whenever its parent re-renders. With memo, you can create a component that React will not re-render when its parent re-renders so long as its new props are the same as the old props.

기본적으로 리액트는 부모 컴포넌트를 렌더링하면 자식 컴포넌트도 렌더링이 발생하는데요.
memo를 적용한 자식 컴포넌트는 부모가 전달한 props가 바뀌지 않으면 리렌더링을 방지합니다.
그럼 부모로 부터 전달 받은 props 자체가 없으면요? 바뀌지 않은 경우에 속하므로 이 역시 리렌더링을 방지합니다.
즉, props가 있든 없든 의미 있습니다😄

https://react.dev/reference/react/memo#skipping-re-rendering-when-props-are-unchanged

src/App.tsx Outdated
@@ -38,6 +39,10 @@ function App() {
<CardNamePage addCreditCard={addCreditCard} lastCard={lastCard} />
}
/>
<Route
path="/addCardLoading"
Copy link

Choose a reason for hiding this comment

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

Route Path는 다른 곳에서도 사용할 수 있죠.
다음 미션부터는 상수로 관리해보면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

현재 제 코드의 NAVIGATE 상수처럼 말씀이실까요? navigate 경로도 상수로 정의했었는데 활용해서 적용해보겠습니당!

Copy link

Choose a reason for hiding this comment

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

);

return <BottomSheet trigger={trigger} buttons={IMGButtons}></BottomSheet>;
Copy link

Choose a reason for hiding this comment

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

외부에서 on/off 상태를 완전히 컨트롤 하기 가장 쉬운 방법을 생각해봤는데요.
닫기 기능 없이 props로 isVisible을 넘겨주면 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

리트의 말을 한번 생각해보았는데요

  1. 열기만 작동하고 닫히는 것은 밖에서 상태로 조종하는 것
  2. 열고 닫기 자체를 밖에서 조종하는 것
    둘 중 하나라고 생각했는데 아니라면 다시한번 말씀해주실 수 있으실까요!

Copy link

Choose a reason for hiding this comment

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

제 설명의 의도는 2번입니다!

@hafnium1923
Copy link
Author

리트~! 좋은 일주일의 시작입니다!
어제 반영해서 보내고 싶었는데 연휴의 마지막이라 그런지 차가 너무 막히더라구요..
12시에 속초에서 출발했는데 서울 도착해서 시간을 보니 7시라.... 진이 빠져서 그만 녹아버렸습니다 흑흑
리트는 주말 어떻게 보내셨나요~?😆😆
리뷰 항상 감사드립니당!!!
ps. 요즘 다른 크루들이 리뷰어들과 커피챗 하던데 혹쉬 나중에라도 시간이 되시면 커피챗 어떠신가요?!

미션 페이지

모두 첫 리뷰요청 pr메시지와 동일합니다. 편의성을 위해 미션페이지만 다시 링크 추가했습니다.

payments

주된 변경

  1. useCallback 사용
    알려주신 문서를 토대로 컴포넌트 렌더링과 관련된 메서드(navigate사용 메서드)와 커스텀 훅의 반환 메서드에 모두 적용해보았습니다. 해당 메서드에서 사용되는 의존성이 없을 경우 두번째 인자로 빈배열을 두었습니다.

  2. React.memo
    컴포넌트에 memo를 적용하여 리렌더링을 제어하고 싶으면 context값을 상위 컴포넌트에서 읽고 하위컴포넌트로 props를 통해 전달해야 한다고 이해했습니다. 때문에 상태를 props로 받고있는 컴포넌트에만 memo적용해보았습니다. cardinput 컴포넌트들은 상태를 직접적으로 사용하지 않고 공용 input 컴포넌트에 props로 상태를 내려주고 있기 때문에 공용 input 컴포넌트에 memo를 적용했습니다.

질문

커맨트 밑에 달아두긴 했는데 다시한번 적어봅니당
<메모관련>

  1. 하위 컴포넌트에서 props를 통해 상태를 전달받을 때만 memo가 의미있어지는 것일까요?
  2. 그렇다면 상태를 사용하지 않는 컴포넌트들이 렌더링 되는것을 막고싶다면 Passing JSX as children 의 방식으로 해야하는 걸까요?
  3. 제가 이해한 것을 바탕으로 본다면 현재 코드의 CardDetailView는 상위 컴포넌트에서 상태를 props의 형태로 받아서 사용하고 있기 때문에 memo를 적합하게 사용하는 것으로 보이는데 제 판단이 맞을까요?

끝날 것 같지 않던 페이먼츠의 끝이 코앞이네요.. 뭔가 후련한 것 같기도 하면서 아쉽기도한?! 저 뭔가 이번 리펙토링에서 useCallback이랑 memo에 대해서 많이 감 잡은 것 같은데 리트가 보기엔 어떠신가요?ㅎㅎ 자신감인지 자만심인지 어쨌든 생긴것같아요ㅋㅋㅎ

Copy link

@lsw1164 lsw1164 left a comment

Choose a reason for hiding this comment

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

루루, 페이먼츠 미션 고생 많으셨어요!

감 잡았다는 표현 좋네요. 생소함에서 오는 어려움을 잘 극복하신 거 같습니다😁

남겨주신 질문들은 코멘트에 다 적은 거 같아요!

저도 늦게 발견한 버그(?)가 남았지만, 일정상 이번 미션은 여기서 마무리 짓고 다음 미션하러 가시져🐱‍🏍

@@ -25,19 +29,23 @@ function App() {
<BrowserRouter basename={process.env.PUBLIC_URL}>
<Routes>
<Route
path="/"
path={NAVIGATE.HOME}
Copy link

Choose a reason for hiding this comment

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

상수화가 아무것도 아닌 거 같아도 이런 사소한 디테일이 모여서 관리하기 좋은 프로젝트가 된다고 생각해요😁

Comment on lines +6 to +12
const changeCardName = useCallback(
(e: React.ChangeEvent<HTMLInputElement>) => {
const cardName = e.target.value;
setCardName(cardName);
},
[]
);
Copy link

Choose a reason for hiding this comment

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

useCallback 잘 적용해주셨습니다👍
이번에 많이 연습하셨으니 다음 미션에서는 최적화가 필요한 포인트를 골라서 부분적으로 적용해보면 좋겠어요!
아무래도 메모이제이션은 어딘가에 기록을 해두는 기능이기 때문에 메모리 사용량에 영향을 줍니다.
모든 함수와 값에 일괄적으로 useCallback/useMemo를 적용하다보면 메모리를 과도하게 사용하는 거죠.

일종의 트레이드 오프 죠.
빈번한 리렌더링이 일어나는 경우를 잘 골라서 적용해야 되는데 주관적이긴 해요.
정답이 없네요😅

cardOwnerName,
cardCVC,
cardPassword,
cardCompany,
Copy link

Choose a reason for hiding this comment

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

다시 확인해보니까 cardCompany를 설정되지 않아도 제출이 되네요 😅

@lsw1164 lsw1164 merged commit 7405074 into woowacourse:hafnium1923 May 8, 2023
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