You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
#3017 has been up in draft for a little while which was already a replacement for #2980 which was reverted in #3015 and as we've been seeing this for a while over in kuma-land I've also seen mention of this elsewhere.
So as a potential alternative, I decided to put a draft up using our SharedPool which aims to enforce the DOM container as a shared singleton.
The changes to ToastManager itself are very minor and it all just relies on the SharedPool functionality instead, making it very easy to grok whats going on in the ToastManager without having to worry about the implementation. Just acquire things and release them when you are done, once the last release happens the "thing" gets destroyed.
I believe the constraints here were to not change the new ToastManager() API even though we essentially require a singleton. This PR keeps the multiple ToastManagers but implements the singleton inside, thus maintaining the API.
SharedPool itself was built many moons ago for our own application and I've just copy/pasta'ed it over here as is. I'd probably fiddle with it a bit more now if I had more chance, but it's easier to just drop in what we have over in kuma in here also.
Its basically a MultiMap with lifecycle hooks so you can tell it how to to create, acquire, release and finally destroy things when they are no longer in use. In our application we have many many singletons using this, but here we just need the one, keyed by the previous id of kongponents-toaster-container.
The DOM id itself I now randomise, via Date, so you its harder to fiddle with from the outside. I don't think this impacts SSR judging by the code here previous to the PR, but then again the "fiddle prevention" is probably not super necessary in the real-world.
I added something that you can play with in the sandbox: I add a bunch of ToastManager instances all at once, and then slowly remove all but the one we are using and you can see that having multiple managers doesn't produce multiple root containers. I then added a button to remove the ToastManager we are actually using. If you push this button either at the end or before others are removed, everything is cleaned up properly once all the ToastManagers have been destroyed (you'll notice the DOM element is removed)
I might have totally missed something here, so let me know if so. Also more than happy to close this if it's not the desired approach, just lemme know!
There's probably a few more things to say here, but if its not needed then its not worth me going over, but do let me know if anyone has any more questions.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
johncowen
requested review from
a team,
Justineo,
adamdehaven and
jillztom
as code owners
#3017 has been up in draft for a little while which was already a replacement for #2980 which was reverted in #3015 and as we've been seeing this for a while over in kuma-land I've also seen mention of this elsewhere.
So as a potential alternative, I decided to put a draft up using our SharedPool which aims to enforce the DOM container as a shared singleton.
The changes to
ToastManageritself are very minor and it all just relies on the SharedPool functionality instead, making it very easy to grok whats going on in theToastManagerwithout having to worry about the implementation. Justacquirethings andreleasethem when you are done, once the lastreleasehappens the "thing" getsdestroyed.I believe the constraints here were to not change the
new ToastManager()API even though we essentially require a singleton. This PR keeps the multipleToastManagers but implements the singleton inside, thus maintaining the API.SharedPool itself was built many moons ago for our own application and I've just copy/pasta'ed it over here as is. I'd probably fiddle with it a bit more now if I had more chance, but it's easier to just drop in what we have over in kuma in here also.
Its basically a MultiMap with lifecycle hooks so you can tell it how to to
create,acquire,releaseand finallydestroythings when they are no longer in use. In our application we have many many singletons using this, but here we just need the one, keyed by the previousidofkongponents-toaster-container.The DOM
iditself I now randomise, viaDate, so you its harder to fiddle with from the outside. I don't think this impacts SSR judging by the code here previous to the PR, but then again the "fiddle prevention" is probably not super necessary in the real-world.I added something that you can play with in the sandbox: I add a bunch of ToastManager instances all at once, and then slowly remove all but the one we are using and you can see that having multiple managers doesn't produce multiple root containers. I then added a button to remove the
ToastManagerwe are actually using. If you push this button either at the end or before others are removed, everything is cleaned up properly once all the ToastManagers have been destroyed (you'll notice the DOM element is removed)I might have totally missed something here, so let me know if so. Also more than happy to close this if it's not the desired approach, just lemme know!
There's probably a few more things to say here, but if its not needed then its not worth me going over, but do let me know if anyone has any more questions.