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: memory leak due to design token bindings #7022

Open
m-akinc opened this issue Sep 11, 2024 · 2 comments · May be fixed by #7023
Open

fix: memory leak due to design token bindings #7022

m-akinc opened this issue Sep 11, 2024 · 2 comments · May be fixed by #7023
Labels
status:triage New Issue - needs triage

Comments

@m-akinc
Copy link
Contributor

m-akinc commented Sep 11, 2024

🐛 Bug Report

Given a component whose template uses a derived token value, as instances of that component are replaced in the DOM, the old, detached instances cannot be garbage collected, i.e. are leaked.

💻 Repro or Code Sample

See this codepen. Each time you click the button, it gets replaced by a new button instance. The old buttons, which are no longer referenced from the code or DOM, are permanently leaked. See below:

fast-memory-leak

🤔 Expected Behavior

Detached components should be garbage collected.

😯 Current Behavior

Memory is leaked.

💁 Possible Solution

The cause seems to be that DesignTokenNodes do not tear down their binding observers when their corresponding nodes are detached. I.e. this problem is resolved by adding the below for loop to DesignTokenNode.unbind():

    /**
     * Invoked when the DesignTokenNode.target is detached from the document
     */
    public unbind(): void {
        if (this.parent) {
            const parent = childToParent.get(this)!;
            parent.removeChild(this);
        }
        for (const token of this.bindingObservers.keys()) {
            this.tearDownBindingObserver(token);
        }
    }

🔦 Context

We have a custom table component with row components that are replaced when scrolled in and out of view. This quickly eats up large amounts of memory.

@m-akinc m-akinc added the status:triage New Issue - needs triage label Sep 11, 2024
@chrisdholt
Copy link
Member

Thanks @m-akinc - given the nature of this (memory leak), we'd welcome a PR to FE1 to help resolve this for you.

@m-akinc m-akinc linked a pull request Sep 11, 2024 that will close this issue
5 tasks
@m-akinc
Copy link
Contributor Author

m-akinc commented Sep 11, 2024

Thanks @m-akinc - given the nature of this (memory leak), we'd welcome a PR to FE1 to help resolve this for you.

Here's that PR: #7023

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:triage New Issue - needs triage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants