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(cdk/tree): retainining previous objects #30431

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

naaajii
Copy link
Contributor

@naaajii naaajii commented Feb 1, 2025

fixes component retaining old data in memory even when its not used causing memory usage to increase

fixes #30322

@naaajii naaajii requested a review from a team as a code owner February 1, 2025 18:55
@naaajii naaajii requested review from mmalerba and wagnermaciel and removed request for a team February 1, 2025 18:55
@andrewseguin andrewseguin added action: merge The PR is ready for merge by the caretaker target: patch This PR is targeted for the next patch release labels Feb 3, 2025
@andrewseguin andrewseguin self-assigned this Feb 3, 2025
@mmalerba
Copy link
Contributor

It looks like there might be a bug with this. There's an internal test failing because some nested child nodes have the same aria-level as their parent while the parent is collapsed. They're display: none while collapsed, so its probably not an issue from an accessibility perspective, but it causes treeHarness.getNodes({level: parentLevel}) to also pick up the hidden children

@naaajii
Copy link
Contributor Author

naaajii commented Feb 16, 2025

I might be wrong but this is how getNodes seems to be working its returning all nodes that matches the level we provide, I tried to make a reproduction. I can imagine the parents and child having the same level return as a whole from what I see in reproduction.

also, how does one set their own aria-levels on parents and children? it seems to be always setting the level that node directive sets?

@mmalerba
Copy link
Contributor

also, how does one set their own aria-levels on parents and children? it seems to be always setting the level that node directive sets?

right, I think its using the level that the node directive sets, and it seems like this change causes it to get the wrong level from the node, maybe because of clearing out the _levels map?

@andrewseguin
Copy link
Contributor

This seems to be an odd timing issue since the tree's _computeRenderingData relies on rxjs map and tap for iterating the nodes and setting these maps. The particular test failure we're seeing is because _levels is reset, then the harness searches for nodes with a particular level, then _levels is set in __calculateParents.

The harness should be looking after everything is flushed but the timing is off. For this case, I'd expect _levels to be cleared just before it is set in calculateParents. That function already resets the parents and ariaSets.

@naaajii
Copy link
Contributor Author

naaajii commented Feb 26, 2025

apolgies for a very late reply. I did clear _levels in calculate parent along with other maps. however for nested tree the data doesn't seem to be calcuated in _calculateParents so I appended to clear it before RxJs magic cause if I had put clear it inside it was triggering again & again to clear the cache. if this doesn't fix this issue, I will probably end up creating a similiar test and fix against it (I will also appreciate a reproduction if its possible so its much identical to the one I would create) this weekend.

fixes component retaining old data in memory even when its not used causing memory usage to increase

fixes angular#30322
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action: merge The PR is ready for merge by the caretaker area: cdk/tree target: patch This PR is targeted for the next patch release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug(Tree): Memory leak when changing data source
3 participants