Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(overlays): fix memory leaks of adopt-styles and OverlayMixin (#2300) #2302

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

riovir
Copy link
Contributor

@riovir riovir commented Jun 11, 2024

What I did

  1. Adjust the adopt-style styleCache to be a WeakMap preventing the renderRoots from being garbage collected
  2. Let the OverlayMixin unwire its OverlayController from the OverlayManager on teardown

Copy link

changeset-bot bot commented Jun 11, 2024

🦋 Changeset detected

Latest commit: 5e95cc0

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@lion/ui Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@riovir
Copy link
Contributor Author

riovir commented Jun 12, 2024

Will need to look closer into how to keep the OverlayController reference in the OverlayManager without never releasing it. Weak collections seem to be a no-go, as iteration is required. I'm hoping that carefully cutting and restoring the references in the connected / disconnected or init / teardown hooks will get the right results without introducing regression.

@Sciurus7
Copy link
Contributor

That is looking good!
It would be much appreciated if this PR could be handled and released relatively quickly. My company is working on a framework where this memory leak will wreak havoc unless fixed. Unfortunately it is nigh impossible to implement some temporary workaround in our own components that would apply the same fixes as this PR, so we are really dependent on this being fixed in Lion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants