[오픈소스] shadcn-ui 의 버그를 해결하는 PR을 만들다

park·2024년 9월 20일

목록 보기

4/4

들어가면서

인제님이 9월 달에 모집한 오픈소스 멘토링에 또 참가 신청을 하게되었다. 바쁘다보니 그 몇 달 간 오픈소스 기여에 힘쓰지 못했고 감을 완전히 잃어버렸다. 감을 되찾기 위해 신청했고 신청한 보람이 있었다. 이 멘토링으로 추진력을 받아 그대로 es-toolkit에 기여를 하고 Merge까지 됐기 때문이다! ~~(인제님 감사합니다...)~~

아무튼 각설하고, 어떤 이슈를 어떻게 해결했는지에 대해 작성해 볼 것이다.

이슈 선정

제목을 훑어보고 버그 수정과 관련된 이슈만 살펴보았다. 그 중에 이 이슈가 해결하고자 하는 사람이 없어보였고, 그때 기준 최근에 올라온 것이었고, 문제애 대한 묘사가 명확했다. 따라서 도전하기로 마음 먹었다.

이슈에 대하여

다이얼로그 안에 스크롤 될 만큼 요소를 많이 포함하고 있는 콤보박스를 넣으면 콤보박스가 스크롤이 되지 않는 버그다. 이 콤보박스를 다이얼로그 바깥으로 빼면 스크롤이 잘 된다.

이건 동영상으로 보면 더 잘 이해가 돼서, 문제를 정확히 보여주는 동영상을 보려면 이 이슈를 보면 된다. ~~(velog는 아직 동영상 첨부를 지원하지 않는구나..)~~

문제의 원인

문제의 원인을 파악하는데 꽤 오랜 시간이 걸렸다. 혹자는 문제를 해결하려면 문제를 고치는 것에 집중하지 말고 문제의 원인에 집중을 하라고 하는데, 이게 습관이 아직 안되어있어서 그런지 조금 지체되었던 것 같다.

html 구조를 이리저리 파보기도하고, 소스 코드를 까서 shadcn-ui 뿐 아니라 shadcn-ui의 기반인 radix-ui까지 내려갔다 왔다. 그러다가 shadcn-ui의 dialog는 내부적으로 radix라는 라이브러리의 react-dialog에 의존하고 있으며, radix/react-dialog는 dialog가 켜질 때 document.body에 pointer-events 를 none으로 설정해둔다는 것을 발견했다.

이것이 스크롤 이벤트를 막는 게 아닐까? 하는 생각에 여러 실험을 거쳤다. 그러나 document.body의 pointer-events 를 none으로 설정하는 것은 맞지만, 다이얼로그와 그 자식 컴포넌트 요소들은 영향을 받지 않게끔 해놨다.

pointer-events 뿐 아니라 스크롤을 막는다는 것을 의미하는 사용자 정의 속성도 발견했지만, 이 속성을 조정하는 것만으로는 버그를 해결할 수 없었다. 왜냐? 이 역시도 pointer-events 처럼 다이얼로그와 그 자식 컴포넌트 요소들은 영향을 받지 않게끔 해놨기 때문이다. 좀 더 근본적인 원인을 발견해야 했다.

그러다가 하나의 포인트를 발견할 수 있었다. html 구조를 살펴보니, 리액트에서는 다이얼로그의 자식인 콤보박스가 실제로는 다이얼로그와 형제 관계로 렌더링 된 것을 알 수 있었다. 이것은 React Portal 때문이었다. 콤보박스는 기본적으로 React Portal에 의해 렌더링되게 되어있는데, 그 컨테이너가 body로 고정되어있다.

그러니까 보통 우리가 다이얼로그와 같은 컴포넌트를 만들때는 다이얼로그와 그 내부 자식들에는 스크롤을 할 수 있게 하지만 body 태그에서는 하지 못하도록 body 태그에 여러가지 조정을 하게 되는데, 실제로는 body 태그의 자식 - 다이얼로그의 형제로 렌더링 되어버린 콤보박스가 이 '조정'에 영향을 받아버린 것이다.

따라서 이 문제를 해결하려면 콤보박스가 container를 선택적으로 받을 수 있게 해야한다. 특히나 다이얼로그 안에 콤보박스를 넣는 상황의 경우, 콤보박스의 container를 다이얼로그로 설정할 수 있게해서 다이얼로그 - 콤보박스의 실제 html에서의 관계가 형제가 아닌 부모 - 자식 관계가 되도록 해야한다.

문제의 해결

const PopoverContent = React.forwardRef<
  React.ElementRef<typeof PopoverPrimitive.Content>,
  React.ComponentPropsWithoutRef<typeof PopoverPrimitive.Content> & {
    container?: PopoverPrimitive.PortalProps["container"] // 이 부분!
  }
>(
  (
    { className, align = "center", sideOffset = 4, container, ...props },
    ref
  ) => (
    <PopoverPrimitive.Portal container={container}>
      <PopoverPrimitive.Content
        ref={ref}
        align={align}
        sideOffset={sideOffset}
        className={cn(
          "...",
          className
        )}
        {...props}
      />
    </PopoverPrimitive.Portal>
  )
)

위처럼 contaienr를 선택적으로 받을 수 있게해서 Portal의 container를 원한다면 커스텀할 수 있도록 했다. 이렇게 해서 콤보박스의 container를 다이얼로그로 설정해주니, 스크롤이 되지 않던 문제가 해결됐다.

before

Dialog 와 ComboBox(data-radix-popper-content-wrapper)가 형제다.

after

Dialog 와 ComboBox(data-radix-popper-content-wrapper)가 형제가 아닌 부모 - 자식 관계다.

PR

논의사항

다만, PR에도 명시 했듯이

코드 스타일이 메인테이너가 원하는 스타일이 아닐 것 같아 수정이 필요하다면 말해달라고 했다.
문서에 변경이 필요한데 어디를 변경해야하는지 잘 모르겠어서 알려달라고 했다.
Props를 변경하는 건 어찌보면 해당 컴포넌트의 인터페이스를 변경하는거나 마찬가지라 이게 마음에 안든다면 다른 더 좋은 방법에 대해 논의하자고 했다.
일단은 보수적으로 container를 무조건 옵션으로 받도록 했으나, 정상적인 동작은 콤보 박스가 어떤 컴포넌트 안에 있으면 해당 컴포넌트를 container로 갖게 하도록 자동으로 만들고 그렇지 않다면 body 태그를 container로 갖게 하도록 하는 게 좋을 것 이다. 이런 기능이 필요하다면 고려해보고 말씀해달라고 했다.

교훈

오픈소스를 파헤치는 건 정말 재밌는 것 같다. 이렇게 많은 사람들이 사랑하는 라이브러리에도 이런 허점이 있구나, 하며 문제의 원인을 파악하는 게 참 묘미다. 어쩌면 그렇기에 오픈소스에 대해 너무 무겁게 생각할 필요가 없는 것 같다. 소프트웨어 자체에도 너무 무겁게 생각할 필요가 없는 것 같다. 처음부터 모든 문제를 파악하고 해결할 수는 없다. 내가 생각지도 못한 문제는 분명 어딘가에 있으며, 그런 문제는 발생했을 때, 그때 고치면 된다.