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

⚡ Props Drilling 문제 해결 및 케밥 메뉴의 유일성 보장 #76

Merged
merged 5 commits into from
May 22, 2024

Conversation

BangDori
Copy link
Collaborator

@BangDori BangDori commented May 22, 2024

작업 이유


작업 사항

동시에 여러 피드의 KebabMenuList를 펼칠 수 있는 문제 해결하기

  • 페이지 내에서 단 하나의 Kebab 메뉴의 유일성을 보장하기 위해 전역 저장소를 생성하였습니다.
interface FeedKebabStore {
  openedFeedId: number; // 열린 피드의 아이디
  isOpen: boolean; // // kebab 메뉴가 열려있는지 여부
  openKebab: (_: number) => void; // Kebab 메뉴를 여는 메서드
  closeKebab: () => void; // Kebab 메뉴를 닫는 메서드
}

리뷰어가 중점적으로 확인해야 하는 부분

  • Kebab 메뉴의 유일성이 보장되고 있나요?
  • 이슈에 대해 함께 고민해주시면 감사하겠습니다!

발견한 이슈

Props Drilling 문제

Feed -> FeedKebabButton -> KebabMenu -> FeedReportsForm -> useSubmitReports

Feed -> useSubmitReports까지 계속해서 props를 전달해줘야 하는 props drilling 문제가 발생

문제 분석

  1. props drilling이란 상위 컴포넌트에서 하위 컴포넌트까지 props를 전달해주는 과정에서 중간 컴포넌트는 해당 props를 사용하지 않음에도 props를 받고, 전달해줘야 하는 것
    => 이로 인해 컴포넌간의 결합도가 증가하고, 유지보수성이 크게 저하하게 됨.

  2. feedIdonClose 메서드를 props로 계속해서 전달해주는 것은 맞지만, 전역 상태로 관리하기 애매한 부분이 존재하며 실제 전달의 역할을 하는 컴포넌트는 KebabMenu만 해당

현재 구조를 확인해 보면 FeedKebabButton부터 feedIdonClose 메서드가 전달되게 되는데 실제 중간 컴포넌트(전달의 역할을 하는 컴포넌트)라고 생각되는 컴포넌트는 KebabMenu입니다.

export const KebabMenu: React.FC<KebabMenuProps> = ({ feedId, onClose }) => {
  const [isVisibilityReportsForm, toggleVisibilityReportsForm] =
    useToggle(false);

  return (
    <>
      <ul className='kebab-menu-list'>
        <li className='kebab-menu-item'>
          <HideButton feedId={feedId} onClose={onClose} /> // 🚨 또 다른 전역 상태로 props를 관리?
        </li>
        <li className='kebab-menu-item'>
          <button
            className='item-btn b2md'
            onClick={toggleVisibilityReportsForm}
          >
            신고하기
          </button>
        </li>
      </ul>
      {isVisibilityReportsForm && (
        <FeedReportsForm feedId={feedId} onClose={onClose} /> // ✅ Modal 전역 상태로 관리 가능
      )}
    </>
  );
};

KebabMenu 버튼은 현재 HideButton과 FeedReportsForm으로 props를 전달해주고 있습니다. FeedReportsForm의 경우에는 신고 모달창으로 모달 전역 상태로 관리가 가능한 부분이지만, HideButton은 모달 전역 상태라고 보기에는 힘들기에 또 다른 전역 상태를 두어 관리를 해야 할 것 같습니다.

개인적인 의견

만약 두 개의 전역 상태로 이를 관리한다면, 단 하나의 중간 컴포넌트 때문에 2개의 전역 상태를 두는 것은 오버헤드라고 생각합니다. 뿐만 아니라 너무 많은 전역 상태는 가독성을 저하시킬 수 있으며 잠재적인 위험을 불러일으킬 수 있다고 생각합니다.

그래서 제가 생각하기에는 해당 부분에서는 컴포넌트의 단위를 기능별로 쪼개서 발생한 문제일 뿐, Props Drilling이 크게 문제가 되는 상황은 아니라는 생각이 들어 기존 방법으로 가도 큰 문제가 되지 않을 것 같다고 생각합니다. 혹시 어떻게 생각하시나요?

@BangDori BangDori requested a review from Legitgoons May 22, 2024 05:11
@BangDori BangDori self-assigned this May 22, 2024
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-76.d37mn03xh3qyyz.amplifyapp.com

@Legitgoons
Copy link
Member

Legitgoons commented May 22, 2024

단순히 props를 이동시키기 위한 drilling이 아니고, 거치는 컴포넌트의 대부분이 사용한다면 굳이 전역으로 상태를 관리할 필요가 없다고 생각합니다.

의견 제시 과정에서 props drilling이 일어나는 것인지를 우선 확인했어야 했는데, 이 부분을 놓쳤네용... 앞으론 좀 더 꼼꼼히 확인해보겠습니당

Copy link
Member

@Legitgoons Legitgoons left a comment

Choose a reason for hiding this comment

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

Kebab 메뉴의 경우에는 유일성이 잘 보장되고 있습니다. 병합하셔도 좋습니다!

@BangDori
Copy link
Collaborator Author

@Legitgoons

아닙니다! 제가 꼼꼼히 확인하고 이슈 등록했어야 했는데, 꼼꼼히 확인하지 않고 이슈로 등록했었던 것 같습니다.

같이 의논해주셔서 감사합니다!

@BangDori BangDori merged commit b28debd into main May 22, 2024
2 checks passed
@BangDori BangDori deleted the refactor/issue-67-props-drilling branch May 22, 2024 08:09
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