fix(ktoaster): prevent duplicate containers via a shared pool v2

[johncowen]

Alternative to #3041

This one makes the entire ToastManager a singleton instead of just its DOM target.

It does this by creating a new proxy ToastManager class which can be instantiated multiple times, this new ToastManager instantiates the old ToastManager as a singleton, so we only ever have one ToastManager.

The one caveat with this is the zIndex option can't really be guaranteed to be set it we have a ToastManager singleton, but looking at the code it never worked as intended anyway (it set the zIndex of the Toasters inside of the DOM target, which form what I see is pointless)

I found the PR where this zIndex option was added (#2130) and it looks like it wasn't a specific ask anyway as its in amongst a more general refactor of "add zIndex to all overlaying components" so I'm guessing no-one is using this anyway (and if they are its not working as intended).

I deprecated the options argument here also, and I'd see this approach as backwards compatible because, if I'm not mistaken, the zIndexing wasn't working as intended anyway (and can't be supported properly if we want a shared singleton DOM target), although there maybe other opinions on this. Personally I prefer this approach to #3041, but I'd be happy with either.

Small note: I spotted quite a few things/potential issues here with other things in the older code, but didn't want to mix that up with this singleton task.

[netlify]

✅ Deploy Preview for kongponents-sandbox ready!

Name	Link
🔨 Latest commit	a2b439a
🔍 Latest deploy log	https://app.netlify.com/projects/kongponents-sandbox/deploys/698232283cd66200086e3304
😎 Deploy Preview	https://deploy-preview-3045--kongponents-sandbox.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[netlify]

✅ Deploy Preview for kongponents ready!

Name	Link
🔨 Latest commit	a2b439a
🔍 Latest deploy log	https://app.netlify.com/projects/kongponents/deploys/698232280b7e3a0008ff3c44
😎 Deploy Preview	https://deploy-preview-3045--kongponents.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify project configuration.

[johncowen]

Taking out of draft so its clear I'd be fine merging this - also good to close if not desired

[Justineo]

Why do we need a state machine here? Overall, this feels like it might be adding more complexity than necessary for what seems to be a fairly simple issue.

Could we just reuse the toast container if it’s already been created?

[johncowen]

Why do we need a state machine here?

It just makes the lifecycle easier to understand in the code - I can look at the implementation and its super clear that "This is what it does on creation", "This is what it does when I ask for another one", "This is what it does on destruction" etc etc - the user code is extremely clear.

And to be fair there's an extremely simple state machine. no? - Its pretty much just a switch statement.

The more important thing is the MultiMap to track usage and be able to instantiate and clean up correctly which is the main issue.

Overall, this feels like it might be adding more complexity than necessary for what seems to be a fairly simple issue.

From what I understand we need to enforce a singleton and include a destructor (for which we need to track usage), but without changing the existing new ToastManager() API.

const pool = new SharedPool<string, InternalToastManager>((state, id, item) => {
  switch (state) {
    case 'creating':
      return new InternalToastManager()
    case 'acquiring':
      return item
    case 'releasing':
      return item
    case 'destroying':
      item.destroy()
      return item
  }
})

In my eyes the above isn't that complex and makes perfect sense when I read it - I can totally understand whats happening and I changed very little original code.

If you are concerned about the SharedPool/MultiMap I copied over from Kuma, then I guess as a thing to handle lifecycles I'd say its also pretty simple really - just 50 LOC including comments.

Could we just reuse the toast container if it’s already been created?

Including the initial implementation, I think we are on the 3rd attempt to solve this issue correctly (we've had a revert and there's another draft thats been up for a few weeks). So yeah I guess 🤷 , but feels like its not been that straight forwards for whatever reason (I guess the cleanup). So I figured I'd give it a go myself with the two options I've PRed.

As I said happy to close these PRs if its not the desired approach but I'm pretty sure this solves every detail of the issue (unless I've missed something)

Let me know if you have any more q's 👍

[Justineo]

Thanks for the detailed explanation. I agree that the state machine itself is relatively simple and readable. However, for this case I still think not introducing a new abstraction is the simpler approach.

From my perspective, the actual requirements here are quite small: we just need to ensure the toast container exists when a toast is opened, and to avoid creating a new container if one already exists. A direct DOM-based check already handles both.

Also, if a page ends up loading multiple Kongponents bundles (even if it’s not ideal), in-memory ref counting would be isolated per bundle while still operating on the same actual DOM container, which makes the lifecycle harder to coordinate. Using the DOM container as the source of truth keeps this straightforward.

Given that, I feel the simpler solution is sufficient for the intended behavior.

[johncowen]

Also, if a page ends up loading multiple Kongponents bundles (even if it’s not ideal), in-memory ref counting would be isolated per bundle while still operating on the same actual DOM container, which makes the lifecycle harder to coordinate. Using the DOM container as the source of truth keeps this straightforward.

I noted that here:

#3041 (comment)

But to be fair, if multiple Kongponents bundles are installed adding two DOM nodes due to this are the least of your worries as its pretty likely all the CSS breaks. This has caused several incidents as far as I'm aware. We specifically have tests in place to prevent multiple kong dependencies from being installed and prevent the CSS from breaking, but we are getting into a different discussion here. My assumption was that we assume a single kongponents installation, but if thats not the case I would guess it would be pretty trivial to move pool somewhere where its guaranteed to be a per-app singleton itself.

From my perspective, the actual requirements here are quite small: we just need to ensure the toast container exists when a toast is opened, and to avoid creating a new container if one already exists. A direct DOM-based check already handles both.

How are you going to handle destruction? Or are you just going to skip that?

FYI: There is a slightly simpler version that could be done seeing as we only need one singleton/destructor here, but seeing as I just copy/pasta'ed the code for SharedPool over from kuma (i.e. a poor mans dependency), that's actually much simpler than reinventing the wheel for a single usecase or implementing something new that doesn't require multiple singletons.

Anyway I'm going to go ahead and close these, thanks for the feedback! I'll keep an eye out for a different fix.

[Justineo]

How are you going to handle destruction? Or are you just going to skip that?

Maybe we can still remove the container whenever a ToastManager is destroyed, as long as a new one is created before showing the next toaster.

I think we can start by revisiting Max’s draft to see if a simple solution already addresses the problem.

[johncowen]

I think we can start by revisiting Max’s draft to see if a simple solution already addresses the problem.

I think this was my point of the PRs I put up. I don't want to sounds dismissive, but as far as I'm aware the draft is the third go at getting this right with what I think you'd consider a "simple solution" and that's been up for a while and I'd assumed there was some reason for that - who knows 🤷 . Additionally from what I understood that Max has been pushed for time to get that through, and again not to sound dismissive but I don't follow why its not just a fix it, approve and merge given the simplicity 🤷.

We reported the issue a good while back (~2 months ago I believe) and I've seen others now reporting the same issue. So I thought I'd give a couple of options using reusable/purposeful code that was already written moons ago that I know works. Quick copy paste and issue solved.

But as I said if either of my approaches isn't desired then, I'll just continue to keep an eye out for an alternative fix 👍

[johncowen]

FYI: #3017 (comment)

We've decided to just skip destruction. Seems like its not that simple after all! 😅

[johncowen]

(just wanted GH interface to show a new commit here so I had to open and then close again real quick)

[kongponents-bot]

Preview package from this PR in consuming application

In consuming application project install preview version of kongponents generated by this PR:

@kong/kongponents@pr-3045

[johncowen]

Note: We wanted to reopen this to generate a PR release to try some things out downstream

[kongponents-bot]

🔴 PR audit failed. 🔴

🔥 Renovate Security PRs detected.

There are 1 open renovate security PRs older than 3 days.

This PR cannot be merged until all renovate security PRs created more than 3 days ago are resolved.

🔥 PNPM Audit issues detected.

┌─────────────────────┬────────────────────────────────────────────────────────┐
│ high                │ node-tar Vulnerable to Arbitrary File                  │
│                     │ Creation/Overwrite via Hardlink Path Traversal         │
├─────────────────────┼────────────────────────────────────────────────────────┤
│ Package             │ tar                                                    │
├─────────────────────┼────────────────────────────────────────────────────────┤
│ Vulnerable versions │ <7.5.7                                                 │
├─────────────────────┼────────────────────────────────────────────────────────┤
│ Patched versions    │ >=7.5.7                                                │
├─────────────────────┼────────────────────────────────────────────────────────┤
│ Paths               │ .>nuxt>@nuxt/nitro-server>nitropack>@vercel/           │
│                     │ nft>@mapbox/node-pre-gyp>tar                           │
├─────────────────────┼────────────────────────────────────────────────────────┤
│ More info           │ https://github.com/advisories/GHSA-34x7-hfp2-rc4v      │
└─────────────────────┴────────────────────────────────────────────────────────┘
1 vulnerabilities found
Severity: 1 high

PR with those issues cannot be merged.

How to resolve:

• Check open renovate PRs for updates to the dependencies mentioned in the audit report
• try to update dependencies listed in the audit report to the latest versions
• use pnpm audit --fix to automatically fix issues

🔥 No test coverage detected.

This PR does not include any test coverage changes, but it modifies source code. Please add appropriate tests to cover the changes made in this PR.

If you believe this is a false positive or if there are valid reasons for not including test coverage changes, please request an exemption by adding the test-coverage-exempt label to the PR and ensure it is approved by one of those managers jillztom, nateslo, erichsend, lahabana, hangrao, ryanmoore, elen4, DaniellaFreese, mfollett, ValeryG.