Suggestion: Prevent ADD action if the custom-id already exists.

[jgjgill]

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

2024-07-11.8.34.23.mov

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

2024-07-11.8.42.15.mov

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

[ojj1123]

This issue occurs even if not calling overlay.open quickly as general.

2024-07-11.22.50.36.mov

As @jgjgill said, that is because every calling overlay.open, ADD action is triggered and then the same overlay key is added to overlayState.overlayOrderList.(not added to overlayData) And overlayOrderList is used to render the overlay components.

overlay-kit/packages/src/context/reducer.ts

Line 16 in 090e68f

overlayOrderList: [...state.overlayOrderList, action.overlay.id],

[jungpaeng]

I understand why this issue is occurring.

Since overlayId functions as a key in React, it should only exist once on a screen.
Therefore, some action is needed when the same custom ID is opened multiple times with overlay.open.

While returning the state as-is when overlayId already exists, as shown in your code, can prevent errors, it might appear as a bug to the user. From the user’s perspective, it seems like the overlay isn’t opening even though they attempted to open it.

So, I think it would be better to throw an error to make it clear that this is not the correct usage. Additionally, we should update the documentation to inform users not to open the same custom ID multiple times.

What do you think?

[ojj1123]

From the user’s perspective, it seems like the overlay isn’t opening even though they attempted to open it

Yeah, I agree with you. If returning the as-is state with the same overlay key, it could be possible. 🤔

So, I think it would be better to throw an error to make it clear that this is not the correct usage. Additionally, we should update the documentation to inform users not to open the same custom ID multiple times.

Btw, this warning occurs when below senario:

1. calling overlay.open() with custom id
2. close the overlay without unmount
3. again open with custom id -> warning cuz it prefers to use the unique key on rendering the list
4. close it without unmount
5. ...

I think this case could be correct and possible. As a video in my comment above, it shows the above case.
That warning occurs because when triggering ADD action with custom id, that key is added to overlayOrderList even if it is already in overlayOrderList. And in above case, as calling close and not calling unmount, it can't remove the custom key from overlayOrderList. So I think we need to prevent it from being added to overlayOrderList if there is the same key.

How about this workaround? But I didn't try it so please check it out.

    case 'ADD': {
      const isExisted = state.overlayOrderList.includes(action.overlay.id);

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

[jgjgill]

@jungpaeng

Oh, I didn't think about the user and focused on eliminating errors. 😂
Throwing an error is certainly appropriate in this situation, both for the user and the library.
I also agree that this is not the correct usage. 🧐

So maybe the code would look something like this?

reducer.ts

    case 'ADD': {
      if (state.overlayOrderList.includes(action.overlay.id)) {
        throw new Error('The same overlayId exists.');
      }
    ...

Expected behavior

2024-07-12.5.39.59.mov

[jgjgill]

@ojj1123
Oh, the scenario you shared seems to be related to that issue as well. 🧐
Discussions are still ongoing, but at this point it looks like current might be fixed in close as well.

On the other hand, as the discussion progressed, I realized that the behavior of opening the same customId is using the library incorrectly.
So I thought it would be better to explicitly throw an error to discourage this usage rather than allowing it in this case. 🙇‍♂️

[ojj1123]

Yeah I don't think that "open and open repeatly" is the intended use case.
But I'm wondering open -> close w/o unmount -> again open is also wrong. If this library throws the error for opening an overlay with the same key, it throws the error on the use case I said:

1. open w/ the custom id
2. close w/o unmount
3. again open w/ the custom id -> error since there is already the same key.

[jgjgill]

Additionally, I assumed that executing close without unmount means the user is aware of this and intentionally keeps the overlay in memory.

This seems to be an example of that comment. I think it's right to throw the error.

[jungpaeng]

I'm outside right now, so I can't write in detail, but I wanted to leave a brief note. Thank you very much for your interest and for proposing a solution.

First, we need to prevent opening an overlay without closing it first. In this case, it seems appropriate to return an error.

However, reopening an overlay without unmounting it after closing should work. There are cases where not unmounting intentionally is done for performance optimization reasons.

Therefore, in the ADD phase of the reducer, we need to check if the overlay currently exists in overlayData and refer to the value of isOpen to make appropriate modifications.

I'll revisit this issue later. Thank you once again.

[jgjgill]

However, reopening an overlay without unmounting it after closing should work. There are cases where not unmounting intentionally is done for performance optimization reasons.

Ah, I understand. I was misunderstanding the problem situation (performance optimization) 😂.
As you said, we need some logic to modify overlayData if isOpen = false (and throw an error if isOpen=true).

Thanks for the quick feedback.

[jungpaeng]

@ojj1123 @jgjgill Hello. I have written PR #53 based on the feedback from both of you.

Could you please review it before I finalize it, if you have time?