toss / overlay-kit

The simplest and most intuitive way to manage overlays in React.
https://overlay-kit.slash.page
MIT License
295 stars 30 forks source link

[Feature Request] useOverlay #27

Closed saengmotmi closed 4 months ago

saengmotmi commented 4 months ago

안녕하세요. 좋은 라이브러리 만들어주셔서 감사합니다 :)

기존 @toss/use-overlay도 잘 사용하고 있었어서 이번에 업데이트 된 overlay-kit도 바로 적용해보았는데요.

마이그레이션을 하다 보니 이런 케이스가 있더라고요.

const overlay = useOverlay()
// 중략...
return {
  openModal: openFooModal,
  closeModal: overlay.exit,
};

위 예시 케이스는 openFooModal 에서 구체적인 비즈니스 로직을 포함한 모달 관리 로직을 구현하고, 그 모달 내부에서 돌려 받은 콜백에서 컨트롤이 끝나는게 아니라 외부로 해당 모달의 id가 담긴 overlay.exit을 노출시켜야 하는 경우였습니다.

하지만 이번 변경으로 인해 overlay가 훅으로 부터 분리되면서 close, unmount 시 id를 맥락으로 가지고 있지 못하게 되었고, 별도로 useRef 등 수단에 id를 담아주는 구현을 추가해야 했습니다.

물론 의도하신 디자인일수도 있으나 기존 라이브러리로부터의 마이그레이션을 돕는 관점에서든 id 값에 대한 캡슐화 관점에서든 useOverlay를 제공해주시면 조금 더 편하게 활용할 수 있는 라이브러리가 되지 않을까 싶어 이슈를 올려봅니다.

아래는 제가 간단히 구현해본 버전입니다

import { useCallback, useRef } from 'react';
import { overlay } from 'overlay-kit';

// OverlayControllerComponent가 내부 타입으로만 선언되어 있어 overlay의 타입을 뽑아왔습니다. 
type OverlayControllerComponent = Parameters<(typeof overlay)['open']>[number];

const useOverlay = () => {
  const id = useRef<string | null>(null);

  const open = useCallback((controller: OverlayControllerComponent) => {
    id.current = overlay.open(controller);
    return overlay.close.bind(null, id.current);
  }, []);

  const close = useCallback(() => {
    if (id.current !== null) {
      overlay.close(id.current);
      id.current = null;
    }
  }, []);

  const unmount = useCallback(() => {
    if (id.current !== null) {
      overlay.unmount(id.current);
      id.current = null;
    }
  }, []);

  return {
    ...overlay,
    open,
    close,
    unmount,
  };
};

export default useOverlay;
jungpaeng commented 4 months ago

안녕하세요, overlay-kit에 관심을 갖고 의견을 주셔서 감사합니다!

useOverlay를 사용할 때, 훅에서 반환된 close, exit를 직접 사용하는 것은 overlay가 여러 개 열려있는 상황(열린 순서에 따라 1, 2, 3 오버레이라고 지칭)에서 2, 3 오버레이를 유지한채로 1 오버레이를 닫는 등의 상황에서 필요할 거라고 생각했습니다. 이러한 케이스는 대부분의 경우에는 필요하지 않을 거라고 생각했기 때문에 말씀주신 것처럼 의도된 디자인이긴 합니다.

다만, use-overlay에서 overlay-kit로 마이그레이션하는 과정에서 불편함이 생길 수 있다는 점은 깊게 고민해보지 못했고 이를 개선하기 위한 작업 역시 필요할 것으로 생각됩니다.

하나의 useOverlay가 하나의 오버레이를 담당해서 한 화면에서 여러 overlay를 띄워야 할 때 useOverlay를 여러 번 호출하는 과정, 그리고 overlay.open의 사용부와 useOverlay가 멀리 떨어져있어서 코드를 읽을 때 불편함을 줄 수 있는 부분 등의 이유로 이를 위해 useOverlay 훅을 다시 살리는 것은 가능하면 피하고 싶습니다.

overley-kit의 overlay.open이 현재는 overlayId만을 반환하고 있는데, 해당 overlayId를 관리하는 close, exit, unmount 함수를 함께 반환하는 방법이 생각나는데 혹시 이렇게 하면 마이그레이션이 쉬워질까요?

아래는 대충 생각한 코드를 작성해보았으며, 작업하면서 API는 변경될 수 있습니다.

// 반환된 close를 사용하면 overlay.open 으로 연 오버레이가 닫힙니다.
const { overlayId, close, unmount } = overlay.open(({ isOpen }) => {
  return (
    <Modal isOpen={isOpen} />
  );
});
jungpaeng commented 4 months ago

해당 부분에 대해 좀 더 고민해보았습니다.

overlay.open(..., { overlayId: 'customId' });

위와 같이 custom id를 받을 수 있도록하면 기존 API에 Breaking Change를 만들지 않고도 해당 문제를 보다 효과적으로 해결할 수 있을 것으로 생각되어요.

위와 같이 작성되었을 때, custom hook 마이그레이션은 다음과 같이 진행할 수 있을 것입니다.

function openFooModal() {
  const overlayId = 'foo-modal'; // 또는 FooModal이 여러 개 띄워질 수 있다면 random한 Id를 지정합니다.

  return {
    open() {
      overlay.open(() => <FooModal {...} />, { overlayId });
    },
    close() {
      overlay.close(overlayId);
    },
    ...
  };
}

이와 같이 하면 overlay.open이 반환하는 값을 ref에 다시 넣어서 close를 위해 전달하는 과정이 제거될 것 같아요.

이는 overlay-kit과 유사한 API를 제공하는 유명 라이브러리 react-toastify에서도 제공하는 방법으로 보여서 이질적인 API는 아니라고 생각됩니다.

혹시 이렇게 진행했을 때 컨선이 있을까요?

saengmotmi commented 4 months ago

@jungpaeng 빠르고 상세한 답변 감사드립니다 🙇🏻‍♂️🙇🏻‍♂️🙇🏻‍♂️

마지막에 제안주신 overlayId를 추가하는 방식이라면 충분히 합리적인 대안이 될 것 같습니다. 결국 내가 오픈한 모달을 특정할 수 있으면 되는 것이기에 꼭 open에서 리턴된 값을 쓰지 않아도 괜찮다고 이해했습니다.

또한 꼭 이 경우가 아니더라도 특수한 경우에 사용할 수 있는 비상구 역할을 할 수 있는 일반적인 솔루션이라 좋게 느껴지네요 ㅎㅎ 설계 철학과 어긋나지 않는다면 적용되면 편하게 사용할 수 있을 것 같습니다!

jungpaeng commented 4 months ago

29 PR에서 해당 기능이 추가되었습니다 🙇

1.1.0 업데이트때 포함될 수 있도록 하겠습니다