안녕하세요! overlay 객체 공식 문서를 읽고 overlay.open(...) 콜백 함수의 open, close, unmount 와 같은 프로퍼티들이 어떻게 동작하는지 내부 구현을 보고 싶어서 모듈 파일을 확인해봤는데 컴파일되어 있는 결과물만 있는 것 같은데 어떻게 하면 내부 구현 코드를 확인할 수 있나요??

Question

As I looked at the code, I had the following questions.

Neither overlay.close() nor close will change the current.
When I saw the name current, I thought it was related to the overlay that is currently open, but it's not.
Currently, current only changes in relation to unmount.

Is this the intended behavior?

Test

I ran the following tests.

import { act, render, screen } from '@testing-library/react';
import { useEffect, type PropsWithChildren } from 'react';
import { afterEach, describe, expect, it } from 'vitest';
import { OverlayProvider, useCurrentOverlay } from './context';
import { overlay } from './event';

afterEach(() => {
  overlay.unmountAll();
});

describe('overlay object', () => {
  it('overlay.close current test', async () => {
    const wrapper = ({ children }: PropsWithChildren) => <OverlayProvider>{children}</OverlayProvider>;

    const testContent1 = 'context-modal-test-content-1';
    const testContent2 = 'context-modal-test-content-2';
    const closeCurrent = 'close-current';

    let current: string | null = null;

    const Component = () => {
      current = useCurrentOverlay();

      useEffect(() => {
        overlay.open(
          ({ isOpen }) => {
            return <>{isOpen && <p>{testContent1}</p>}</>;
          },
          { overlayId: 'overlayId1' }
        );

        overlay.open(
          ({ isOpen, close }) => {
            return <>{isOpen && <p onClick={close}>{testContent2}</p>}</>;
          },
          { overlayId: 'overlayId2' }
        );
      }, []);

      return <div onClick={() => overlay.close(current!)}>{closeCurrent}</div>;
    };

    const renderComponent = render(<Component />, { wrapper });

    const testContentElement2 = await renderComponent.findByText(testContent2);
    const closeCurrenteElement = await renderComponent.findByText(closeCurrent);

    act(() => {
      testContentElement2.click(); // I expect `current` to change overlayId1.
      closeCurrenteElement.click(); // I expect `testContent1` to be closed.
    });

    expect(screen.queryByText(testContent1)).not.toBeInTheDocument();
    expect(screen.queryByText(testContent2)).not.toBeInTheDocument();
    expect(current).toBe(null);
  });
});

Result

The context-modal-test-content-1 still exists.

The current returns overlayId2, not null.

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

기존 @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;

Current Documentation Challenges

The current documentation for the overlay-kit is scattered across multiple sources. Users must read through various documents to fully understand how to use the overlay-kit, and there are significant risks of missing important information, such as precautions and best practices.

Proposed Solution

This issue aims to consolidate and enhance the documentation to make it more intuitive and accessible. The goal is to ensure that users can understand what the overlay-kit is designed to achieve without having to look at example code. We need to present a clear, unified view of the tool’s functionality and usage guidelines in one comprehensive document.

Issue Description:

While working with the overlay system, I noticed an unexpected behavior where overlays do not maintain their state upon being reopened after a close action, despite not being removed from the DOM.

This behavior deviates from the expected functionality where overlays should retain their state between close and reopen actions when not explicitly unmounted.

Expected Behavior:

When an overlay is closed (without unmounting) and then reopened, it should retain its previous state and appear as it was before being closed. This expected behavior is demonstrated in the attached video.

Current Behavior:

Currently, overlays reset to their initial state when reopened after being closed, regardless of whether they were removed from the DOM. This issue is evident as the component appears to reinitialize, losing any state it previously held. The problem is illustrated in the attached video.

Steps to Reproduce:

1. Open an overlay using overlay.open().
2. Close the overlay without unmounting it.
3. Reopen the same overlay.
4. Observe that the overlay resets to its initial state instead of retaining the state from before the close.

Proposed Solution:

This issue may be stemming from an inadvertent update of overlayData within the overlayReducer during the reopening of an overlay. It is crucial to ensure that the overlayData remains unchanged if an overlay is closed but not unmounted, to maintain its state across open/close cycles.

Additional Context:

This issue was discovered while implementing a feature intended to reuse overlays efficiently. The expected and current behaviors have been documented through video examples to aid in understanding the issue clearly.

Originally posted by @jungpaeng in #55 (comment)

Summery

It's not a problem, but I cautiously suggest that it would be better if overlay-kit had this function.

Could you please consider adding explicit support for Promise to the overlay kit?

Feature Request

I'm opening an issue because there's a feature I'd like to suggest.

overlay-kit documentation provides some nice examples of using overlay-kit with Promise.

It works well, but as you can see in the documentation examples, creating Promise, creating callbacks, and opening overlays feels like a lot of boilerplate

I think it would be good to have a feature that reduces boilerplate and easily supports Promise use cases.

The current usage is as follows:

const result = await new Promise<boolean>(resolve => {
  overlay.open(({ isOpen, close }) => {
    return (
      <AlertDialog 
        open={isOpen} 
        title="Are you sure?"
        onConfirm={() => {
          resolve(true);
          close();
        }}
        onCancel={() => {
          resolve(false);
          close();
        }} 
      />
    )
  })
});

The rough API I think of is as follows.

const result = await overlay.promise(({ isOpen,close, resolve}) => (
  <AlertDialog 
    open={isOpen} 
    title="Are you sure?" 
    onConfirm={() => {
      resolve(true);
      close();
    }}/>
    onCancel={() => {
      resolve(false)
      close()
    }}
  ))

The nice thing about this implementation is that logic like creating new Promise is abstracted away, reducing boilerplate.

I tested a simple piece of code that implements this use case and found that it worked great.

If this suggestion looks good, I'd like to work on it.

The experience and mental model of using this library are excellent. Thank you for creating a good library.

Please consider my offer. Bye.

When I read docs, overlay.close and overlay.unmount arguments are overlayId.

But in the code, they are id as parameters.

events.ts

I think it would be more consistent if it was overlayId.
what do you think?

          It's not higher. ^18 only cover major version 18. not 19 so In my opinion, this should be updated

Originally posted by @manudeli in #23 (comment)

Problem

After merging this PR #33, I observed that it is possible to happen the memory leak.

The repro on this issue. You can check the memory profiling.
https://github.com/ojj1123/overlay-kit-memory-leak

The points of the repro is below:

1. SPA (using react-router-dom)
2. I didn't call unmount callback and didn't set the custom overlayId.

Every calling overlay.open() w/o the custom overlayId, the new random id is generated by randomId() and then the overlayItem with that id is stored to overlayData. Even if unmounting OverlayProvider, the overlayItems are not GC'd.
I think that the problem is not to call unmount. In other word, the users of overlay-kit need to use unmount callback for preventing memory leak. And as SPA, there are no refresh or reload so the overlay state is not refreshed.

When close is executed, the overlay changes isOpen to false, making it disappear from the screen, but it remains in memory and the DOM. Additionally, I assumed that executing close without unmount means the user is aware of this and intentionally keeps the overlay in memory. As a result, close does not modify current.
#44 (comment)

You said the users intent to remain the overlays in memory, but without the custom id, the same overlay is never re-used since every calling overlay.open, the new overlay is generated. So I'm wondering what's the advantage of storing the overlay states.

Workarounds

I came up with some workaround, but I didn't try that yet.

Clean-up

As useOverlay of slash, it has clean-up function to unmount overlay. Like this, overlay-kit need to clean up the overlay state.

WeakMap

If you want to cache the state, we can use WeakMap. So you change overlayData from object to WeakMap setting the keys to overlayItem:

export type OverlayData = {
  current: OverlayId | null;
  overlayOrderList: OverlayId[];
  overlayData: WeakMap<OverlayItem, any>;
};

If no refer to OverlayItem, that overlayItem will be GC'd. But as the situation below, maybe that are not GC'd:

1. In OverlayProvider, all of overlayItem are already referred.
2. If the users use overlayData using useOverlayData, it can't be GC'd.

Revert #33

As #33, you changed the management of the overlay state from React's internal state(useReducer) to external state(useSyncExternalStore). So even if OverlayProvider is unmounted, the overlay state is not cleared. Therefore, if you manage it as React's internal state, you will be able to initialize the state when OverlayProvider is unmounted.

Docs

You can just let the users know you need to call unmount in the Document. But if the user don't take care of it, the memory leak occurs. So I think that it is good for this library to provider unmounting overlay properly.

Other bug

Making the repro, I found other bug:

If unmounting OverlayProvider and mounting OverlayProvider again, all of isOpen set to true. This is because when mounting OverlayProvider, effect function is called in ContentOverlayController. You can see this bug in my repro.

Hi, I'm learning a lot from your code.
I have a question.

I think when I use unmountAll, the overlays value is to be initial overlays value.
But now overlays.current is state.current.

Test

I proceeded with the test as follows.

store.ts

export function dispatchOverlay(action: OverlayReducerAction) {
  overlays = overlayReducer(overlays, action);
  console.log(overlays); // I add console

  emitChangeListener();
}

temp test code

it('overlay unmount, unmountAll test', async () => {
  const wrapper = ({ children }: PropsWithChildren) => <OverlayProvider>{children}</OverlayProvider>;

  const testContent1 = 'context-modal-test-content-1';

  const Component = () => {
    useEffect(() => {
      const overlayId = overlay.open(() => {
        return <p>{testContent1}</p>;
      });

      overlay.unmountAll(); // I expect { current: null, overlayOrderList: [], overlayData: {} }
      overlay.unmount(overlayId); // I expect { current: null, overlayOrderList: [], overlayData: {} }
    }, []);

    return <div>Empty</div>;
  };

  render(<Component />, { wrapper });
  // How can I check overlays.current here? Is it right to test store.ts 🤔?

I checked the following console. 🧐

Suggestion

In my opinion, the following code should be changed

• state.current => null

reducer.ts

    case 'REMOVE': {
      ...
      return {
        current: remainingOverlays.at(-1) ?? null, // I also think empty strings could be in the `overlayId`.
        overlayOrderList: remainingOverlays,
        overlayData: copiedOverlayData,
      };
    }

    case 'REMOVE_ALL': {
      return { current: null, overlayOrderList: [], overlayData: {} };
    }

Thanks for the great library.

Expected Behavior

If there is no OverlayKitProvider in the parent tree and overlay functions are attempted to be used in child trees, an error message should be displayed. This message should inform users that the OverlayKitProvider is missing and that overlay functions cannot be used without it.

Actual Behavior

Currently, there is no build or runtime error when OverlayKitProvider is missing.

To Resolve the Problem

Implement a runtime error to be triggered when OverlayKitProvider is not present in the parent tree. This will prevent the use of overlay-kit functions in child trees, similar to how Recoil handles such cases.

Summery

I'd like to suggest migrating the current "overlay-kit"'s testing methods to use userEvent. Would you please consider it?

Benefit

• By using userEvent, you will be able to write test code in a form that is more similar to the user's actual use cases.

• The more your tests resemble the way your software is used, the more confidence they can give you.

• All test cases currently used in "overlay-kit" can be migrated to userEvent without any problems.

My Opinion

If this proposal seems like a necessary but tedious task,

I am ready to work on it.

Could you please consider this suggestion?

Thank you always for your dedication.

Add a changeset for versioning.

Problem situation

When specifying custom-id separately on overlay.open, multiple occurrences of that event will result in an error.

Example code

<button
  onClick={() => {
    overlay.open(
      ({ isOpen, close, unmount }) => {
        return (
          <CenterModal isOpen={isOpen} onExit={unmount}>
            <div>
              <p>MODAL CONTENT</p>
              <button onClick={close}>close modal</button>
            </div>
          </CenterModal>
        );
      },
      { overlayId: 'overlayid-1' }
    );
  }}
>
  open center modal
</button>

Demo

Cause of the problem

This appears to be caused by the same overlayId already existing in the overlayOrderList.

Workaround

I thought of adding the following code in the ADD action.
If the overlayId already exists, return the existing state.

reducer.ts

    case 'ADD': {
      if (state.overlayOrderList.includes(action.overlay.id)) {
        return state;
      }

      return {
        current: action.overlay.id,
        overlayOrderList: [...state.overlayOrderList, action.overlay.id],
        overlayData: {
          ...state.overlayData,
          [action.overlay.id]: action.overlay,
        },
      };
    }

Expected behavior

If you think the suggestion is appropriate, would you mind if I add a feature fix and test code? 🧐

Problem

If using useOverlay hook of slash, we couldn't handle this use case:

function Component() {
  const overlay = useOverlay()

  // HOW could I access overlay state such as open/close state?

  return (
    <button onClick={() => overlay.open({ isOpen, close } => <Dialog isOpen={isOpen} close={close} />)}>open overlay</button>
  )
}

overlay-kit does not provide useOverlay hook, but it has the same interface as overlay object returned by useOverlay. That is, overlay state is only known inside Dialog component, and there is no way to know the state outside the callback function of overlay.open function. This can cause the following problem:

function Component() {
  return (
    <button onClick={() => overlay.open({ isOpen, close } => <Dialog isOpen={isOpen} close={close} />)}>open overlay</button>
    {/** UI according to overlay state but.. I couldn't do that */}
  )
}

There are cases where you need to change the UI, not the Dialog, depending on overlay state(open/close). I don't think this use case is uncommon. I've been in this situation too.

Question

Has this use case ever been required in Toss products? If so, can't we use the API provided by overlay-kit?

Issue Description:

This problem occurred in overlay-kit 1.3.0 version.

I ran into the following problem while working with "overlay-kit".

Components inside the "overlay.open" do not react properly when the state exists externally

Expected

export default function Page() {
  const [notWorkingText, setNotWorkingText] = useState("world");
  return (
    <div>
      <button
        onClick={() => {
          overlay.open(({ isOpen }) => {
            return isOpen ? (
              <div>
                <div>{notWorkingText}</div>
                <button
                  onClick={() => {
                    setNotWorkingText("hello");
                  }}
                >
                  text change
                </button>
              </div>
            ) : null;
          });
        }}
      >
        not work example button
      </button>
  </div>
)

When I click the "Change Text" button, I think the text that appears on the screen should change from world to hello.

But this example doesn't actually work

Please let me know if I'm doing something wrong

Case that works

      <button
        onClick={() => {
          overlay.open(({ isOpen, close }) => {
            // eslint-disable-next-line react-hooks/rules-of-hooks
            const [workingText, setWorkingText] = useState("world");
            return (
              <div>
                <div>{workingText}</div>
                <button
                  onClick={() => {
                    setWorkingText("hello");
                  }}
                >
                  text change
                </button>
              </div>
            );
          });
        }}
      >
        work example
      </button>

This example works perfectly. I think it probably responds appropriately to state changes because it holds the state inside the component.

Is this a bug?

If this isn't a bug, I'm curious how this use case can be handled.

Additional Context

I've posted a code snippet that reproduces this issue

Thank you for your efforts

Changing the management of the overlays state to be outside of React.
Currently, the overlays state is managed within React, necessitating a hook to access its value.

Ideally, overlay-kit should allow access to the state without requiring a hook. Thus, moving the state management outside of React.

Consequently, the current event-based state management method will be changed to useSyncExternalStore.

Relate: #19

Summery

The overlay kit provider seems to behave differently than other libraries. Is this intended behavior?

Detail

I realized that the following actions were needed in the test code to test the operation of overlay-kit.

afterEach(() => {
  overlay.unmountAll();
});

The reason this is necessary is because the OverlayProvider is separate from the elements mounted in the actual DOM.

Therefore, in most cases providing different OverlayProviders for individual tests does not make sense from the perspective of making the tests independent.

const wrapper = ({ children }: PropsWithChildren) => <OverlayProvider>{children}</OverlayProvider>;

However, libraries such as jotai can guarantee test independence by providing different providers.

import { Provider as JotaiProvider } from 'jotai'
const wrapper = ({ children }: PropsWithChildren) => <JotaiProvider>{children}</JotaiProvider>;

Personally, I think jotai's approach is closer to the provider role that users expect.

My Opinion

If this is the intended design, it would be good to provide documentation on how to test with overlay-kit.

If this wasn't intended, I think we need to think about how to do it.

I'm curious about your opinion,

Thank you for making a great library thanks! 🙂

We are planning to add support for both React 16.8 and React 17 versions in this project. This upcoming update will ensure compatibility and provide flexibility for developers using different versions of React in their projects.