[PR 코드리뷰] 우아한테크코스 프론트엔드 레벨2 - 리액트 페이먼츠 미션

 

우아한테크코스 프론트엔드 과정 레벨2 <리액트 페이먼츠 미션>을 수행하며 쿠팡의 @wow9144 님께 리뷰를 받을 수 있었다. 리뷰어님께서 지난 유튜브 강의실 미션 때 코드 때도 도움을 받았었다. 스스로 많이 부족하다는 것을 알기에 한편으로 리뷰어님이 보시기에 그 때보다 나아진 게 없어 보일까 걱정도 되었는데, 리뷰어님께서 피드백과 함께 "잘하고 있다. 지금처럼 계속 좋은 모습 보여달라"🥕라는 격려의 말씀을 전해주셔서 바쁜 우테코 생활에서 달달한 당근과 같은 리뷰시간이었다. 😆

 

미션 1단계 피드백

👉 PR 바로가기

작성한 컴포넌트(11): Button, Card, Circle, Container, CreditCard, Form, Input, Label, Modal, Text, Title

1. 컴포넌트 체크리스트 - 범용성  ✅

컴포넌트는 어디에서나 컴포넌트의 이름 그대로 사용 가능하도록 만들어야 한다. 재사용 가능하다는 것은 '범용적'으로 사용가능하다는 말이다. 

재사용 가능한 컴포넌트는 여러 곳에서 재사용되기 때문에 많은 곳에서 해당 컴포넌트에 의존하게 된다. 이는 컴포넌트에 작은 수정이 발생해도 사용 중인 모든 곳에 영향을 미친다는 뜻이다. 컴포넌트 하나만 수정해도 전체를 수정할 수 있는 장점과 더불어, 한 번의 수정으로 전체가 망가지는 위험성도 내포하고 있음에 유의해야 한다.

컴포넌트 내에 존재하는 클래스명(스타일)들은 부트스트랩과 같은 CSS 프레임워크처럼 이 프로젝트와 무관하게 완전히 분리되어도 사용할 수 있는 범용적인 형태이어야 한다. 따라서 컴포넌트의 것이 아닌 구체적이고 특정적인 CSS 스타일 정의는 컴포넌트가 가지고 있으면 안된다. 현재 코드상에서 components/**/style.css 내에 정의된 구체적인 스타일은 모두 pages/**/style.css로 옮겨야 한다. 만약 범용적으로 사용가능한 컴포넌트가 아니라면 'Button'보다 더 구체적인 이름을 갖는 것이 맞다.

// 📦 컴포넌트 내 포장이사 대상
// Button--card-company, CardPasswordInput, CardCompanySelectModal--bottom, CardInfoForm__Input__Filler—filled 외 다수...

2. 컴포넌트 체크리스트 - 편리성  ✅

컴포넌트는 해당 컴포넌트를 사용하는 것이 사용하지 않는 것보다 더 편리하도록 만들어야 한다. 그저 많은 곳에서 사용을 하게 하는게 아니라, 많은 곳에서 '편하게' 사용할 수 있는게 하는 것이 재사용 가능한 컴포넌트의 핵심이다. 

많은 곳에서 사용은 하고 있지만, 컴포넌트를 사용하는 것이(컴포넌트의 인터페이스를 따르는 것이) 오히려 더 불편하고 복잡성을 늘릴 수도 있다. 이럴 경우 컴포넌트를 굳이 사용할 의미가 없다.

컴포넌트 props의 기본값(defaultProps)을 잘못 지정하면 컴포넌트 사용할 때의 피로도가 매우 높아진다. 따라서 기본값은 신중히 정해야 한다. 현재 <Text>의 텍스트 정렬 속성의 기본값이 "center"로 되어있어, 좌측정렬을 위해서는 매번 <Text textAlign="left" .....>.... 라고 명시해야 하는 불편함이 생긴다.

구체적으로 <Text> 컴포넌트에서 다음의 사항을 개선해서 사용하는 측에서 더 편리하게 만들 수 있다. 

defalutProps - textAlign	기본값이 "center"로 지정되어 있어 사용상의 불편함을 야기한다. 👉 기본값을 더 "left"로 지정한다.
props - width	width를 지정하면 빈 공간이 늘어난다는 건지, display가 inline-block이 되는건지 혼란스럽다. 👉 Text 기본 스타일에 지정된 display: inline-block을 제거하고 width 속성을 props로 받지 않는다.
props - fontSize 외 다수	Text 컴포넌트가 fontSize를 받을 수 있도록 구현되어 있다는 이유만으로 props로 따로 주고 있다. 👉 props로 전달하지 않고 className을 받고, 구체적인 스타일을 inline에 명시하지 않는다.

// 개선 전
// index.js
<Text className="AddCompletePage__Text" fontSize="1.5rem">


// 개선 후
// index.js
<Text className="AddCompletePage__Text">
// style.css
.AddCompletePage__Text {
  font-size: 1.5rem;
}

3. 컴포넌트 체크리스트 - 낮은 결합성  ✅

컴포넌트는 컴포넌트를 사용하는 측에서 컴포넌트의 내부 구현을 모른 채로 사용할 수 있도록 만들어야 한다. 즉, 컴포넌트 선언부와 컴포넌트 사용부는 서로 결합도(coupling)가 낮아야 한다.

유념해야할 것은, 컴포넌트를 선언하는 시점과 사용하는 시점이 다르다는 것이다. 지금은 본인이 작성한 후 바로 사용해서 같다고 생각할 수 있지만, 실무에서는 본인이 작성한 컴포넌트를 열흘 뒤, 한달 뒤 팀원이 사용하는 상황이 생긴다.

현재 코드에서 <Button> 컴포넌트는 theme 이라는 prop을 통해 스타일을 정의하고 있다. 만약 열흘 뒤에 팀원이 이 컴포넌트를 사용하고 싶다면 props로 보낼 theme 을 직접 정의해야 한다. 컴포넌트 사용부에서 컴포넌트 선언부에서 theme을 어떻게 적용하고 있는지 모르고서는 <Button> 컴포넌트를 사용할 수 없다. 어떤 theme을 보낼지 사용부에서 작성하고, 해당 스타일도 정의하고, propTypes에도 추가해주어야 한다. 이것은 더 이상 재사용이 아니게 된다.

// 개선 전
// 컴포넌트 선언부
export const Button = ({ type, theme, backgroundColor, children, ...props }) => {
  return (
    <button type={type} className={['Button', `Button--${theme}`].join(' ')} style={{ backgroundColor }} {...props}>
      {children}
    </button>
  );
};
// 컴포넌트 사용부
<Button theme="question-mark" type="button"> ? </Button>


// 개선 후
// 컴포넌트 사용부
<Button className="SecurityCodeInput__Guide_Button" type="button"> ? </Button>

4. 불필요한 모듈 분리는 관리비용만 ++

컴포넌트로부터 분리해서 재사용 가능한 로직이라면 분리해서 사용하는 것이 좋겠다. 하지만 각 컴포넌트의 핸들러(handler.js)의 경우 별도의 모듈로 분리해서 얻는 장점보다 단점이 크다.

우선 각 컴포넌트의 handler.js는 추상화 되지 않은 파라미터와 함수 내의 로직은 재사용 가능하지 않다. 따라서 모듈 분리에 따른 재사용이라는 장점은 얻을 수 없다. 또한, 분리를 하는 컴포넌트 내에서 직접 의존했으면 받지 않아도 되었을 파라미터를 일일이 전달해주고 있다. 이는 불필요하게 복잡도를 증가시키고 관리도 어렵게 한다. handler.js를 분리하지 않는 것이 더 간결하고 관리도 수월하다.

// 개선 전
// handler.js
export const handleNicknameSubmit = ({ e, setRoute, cardInfo, addCardInfoToList }) => {
  e.preventDefault();
  addCardInfoToList(cardInfo);
  setRoute(PAGE.CARD_LIST);
};
// index.js
<Button onClick={(e) => handleNicknameSubmit({ e, setRoute, cardInfo, addCardInfoToList })}> 확인 </Button>


// 개선 후
// index.js
const handleNicknameSubmit = ( e ) => { 
  e.preventDefault(); 
  addCardInfoToList(cardInfo); 
  setRoute(PAGE.CARD_LIST); 
};
...
<Button onClick={handleNicknameSubmit}> 확인 </Button>

5. 라우팅을 삼항연산자로 한다면

지금처럼 삼항연산자로 라우팅 결정을 하면, 새로운 페이지가 하나라도 더 추가되었을 때, 기존의 라우팅 로직을 모두 수정해야 하는 문제가 있다. PAGE.CARD_LIST가 아니면 무조건 AddCardPage 로 라우팅하는 로직도 가만히 생각해보면 어색하다. 라우팅에 조금 더 적절한 로직이 필요하다.

'react-router' 를 도입해보는 것이 좋겠다. (참고: react-router 공식문서)

// 개선 전
{route === PAGE.CARD_LIST ? (
  <CardListPage cardList={cardList} setRoute={setRoute} />
  ) : (
  <AddCardPage addCardInfoToList={addCardInfoToList} route={route} setRoute={setRoute} />
)}

6. 컴포넌트 관심사에 맞는 네이밍

'보안코드' 컴포넌트가 '비밀번호' 컴포넌트 내의 ref인 passwordInputRef를 알고 있는 것이 어색하다. 포커스를 넘겨주기 위해 필요한 것이라면 '보안코드' 커포넌트의 관심사가 아닌 '비밀번호'라는 이름 대신에 '다음에 포커스를 이동시킬 ref'라는 의미를 살려서 추상화된 네이밍을 해주는 것이 좋겠다.

실무에서는 이런 일이 있을 수 있다. 지금은 '보안코드' 다음에 '비밀번호'를 입력하게 하는데 순서가 반대로 바뀌거나, '비밀번호' 컴포넌트가 보안키패드 모달로 변경되거나, '비밀번호' 입력란이 아예 다른 페이지로 이동하면 '보안코드' 컴포넌트는 passwordInputRef가 아닌 다른 ref를 필요로 하게 된다. 이러한 상황에서 지금처럼 '보안코드' 컴포넌트 내부에서 passwordInputRef 와 같은 구체적인 이름을 사용하게 되면 이 이름도 모두 변경해야 하지만, 적절하게 추상화된 이름을 사용하면 '보안코드' 컴포넌트에 props로 전달하는 ref만 바꾸어주면 되기 때문에 변화에 대응하기 쉬워진다.

// 개선 전
const { securityCode, setSecurityCode, passwordInputRef } = props;

// 개선 후
const { securityCode, setSecurityCode, refToBeFocusedNext } = props;

 

 

미션 2단계 피드백

👉 PR 바로가기

1. 리덕스를 선택하는 이유

리덕스는 Action을 Dispatch하면 Reducer를 통해 Store를 갱신하고, 이를 구독하고 있는 곳에 반영된다는 명확한 프로세스를 가지고 있다. 아주 간단한 기능이더라도 이 명확함을 위해 작성해야할 코드량이 많다. 이러한 명확성은 프로젝트 규모가 커서 여러 개발자가 참여하더라도, 코드를 일관성 있게 전개할 수 있도록 도와 유지보수에 유리하다. 이것이 바로 규모가 클 때 리덕스를 사용하면서 얻을 수 있는 이점이다.

한편, 규모가 크다고 하면 무의식적으로 성능이 중요하다는 쪽이로 사고가 전개될 수 있다. 그러나 규모가 큰 것과 성능이 중요한 것은 다른 이야기이다. 리덕스를 규모가 클 때 선택한다는 결론은 맞지만, 성능이 중요할 때 선택한다는 결론을 내면 안된다. 같은 이유에서 규모가 클 때 리덕스가 좋다는 말은 인터넷 상에도 많지만, 성능이 중요할 때 리덕스라는 말은 찾아보기 어려울 것이다.

 

2. 전역 Context 는 필요할 경우에만!

CardListContextProvider와 같이 국소적으로 사용되는 Context를 전역 컨텍스트로 사용하는 것은 적절하지 않다. 해당 Context가 App 어디서든 변경될 수 있어 예상하지 못한 사이드 이펙트가 발생할 가능성이 커진다. 또한, (비용이 실제로 크지는 않지만) 엔지니어링 관점에서는 App 전체가 리렌더링되는 것도 문제가 된다.

 

3. props 네이밍과 관심사 분리

App <---> Button 에서 보면 부모 컴포넌트 쪽으로 올라갈수록 구체적이고, 재사용성이 떨어진다. 반대로 자식 컴포넌트 쪽으로 내려갈수록 추상적이고, 재사용성이 높아진다.

이 때, 컴포넌트의 props는 받는 쪽 기준으로 네이밍 해주어야 한다. 즉, props 네이밍은 상위에서 결정된 구체적인 관심사의 이름 그대로를 더 추상적인 하위 컴포넌트로 전파하면 안된다. 자식 컴포넌트 입장에서는 일을 잘 하기 위해 내려받는 것일 뿐, 그 함수가 구체적인 도메인은 관심사가 아니다. 

<CardCompanyItem> 은 <li> 와 <button>으로 이루어져 있을 때, 이 <button>은 눌리면 외부에서 주입받은 함수 onClickButton 함수를 그저 실행할 뿐이다. 그 함수가 company를 set 하든지 말든지는 CardCompanyItem 의 관심사가 아니다. setCompany 처럼 구체적인 일을 하는 네이밍으로 하위 컴포넌트에게 전달한다면, 상위에서 변경사항이 생기면 하위 코드까지 모두 변경해야하는 문제가 생긴다.

 

4. try...catch 구문 활용 방법

try...catch 구문을 활용하는 방법은 양측에서 try catch로 처리하는 방법과, httpClient에서 리스폰스 객체를 만들어 리턴하는 방법이 있다. 서버 측에서 어떻게 응답을 주는지에 따라 더 편한 방식이 있고, 취향의 영역이기도 해서 둘 중 하나가 더 낫다고 말하기는 어렵다. 두 번째 방식과 같이 error인 경우에도 리스폰스 객체를 만들어 리턴하는 방식은 사용부에서 다시 try...catch 감쌀 필요가 없다는 장점이 있다.

/* fetch 에러일 경우 throw Error */

export const httpClient = async ({ url, method = HTTP_METHOD.GET, body }) => {
  const options = getOptions(method, body);

  try {
    const response = await fetch(url, options);

    if (!response.ok) {
      throw new Error('HTTP 요청 실패');
    }
    const body = await response.json();

    return body;
  } catch (error) {
    throw new Error(error);
  }
};

/* 유틸함수 사용부에서 catch */

const addCard = async (card) => {
    const newCard = deepCopy(card);

    try {
      const body = await httpClient({
        url,
        method: HTTP_METHOD.POST,
        body: JSON.stringify(newCard),
      });

      setCardList((prevList) => [...prevList, body]);
    } catch (error) {
      window.alert(MESSAGE.CARD_ADD_FAILED);
      console.error(error);
    }
  };

 

/* fetch 성공일 경우 response 객체 리턴 */

{
  success: true,
  data: response.json(),
  message: null
}

/* fetch 에러일 경우 response 객체 리턴 */

{
  success: false,
  data: null,
  message: error
}