Skip to content

fix: Persist the original "parentNode" when "retrieving" the root View created by createEmbeddedView #1542

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

Merged
merged 3 commits into from
Oct 18, 2018

Conversation

VladimirAmiorkov
Copy link
Contributor

PR Checklist

What is the current behavior?

When using the Angular createEmbeddeedView from the ViewContainerRef, after calling clear() the created NativeScript Views are left in the memory

What is the new behavior?

When using the Angular createEmbeddeedView from the ViewContainerRef, after calling clear() the created NativeScript Views are left in the memory

Fixes/Implements/Closes #[Issue Number].
ProgressNS/nativescript-ui-feedback#825

parentLayout.removeChild(rootLayout);
rootLayout.parentNode = node;

This comment was marked as abuse.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @NathanaelA ,

The reason for this fix is the following:
Setup:

  • In components like RadListView/ListView the component itself is going to handle the adding of the created templates to the UI tree
  • In those cases it is necessary to "remove" the view that is created by the Angular's createEmbeddedView because by design it automatically adds the created element to the UI tree
  • This is done via the getSingleViewRecursive

So here is the source of this leak:

  • The created "queue" of parent > child relationship that is created inside nativescript-angular initially (before getSingleViewRecursive removes the view) sets the parentNode to the actual ContainerViewRef that is calling the createEmbeddedView
  • Due to the above reason we have to remove that view from the UI tree which itself resets the parentNode to undefined because the elements (the template View) is removed from its parent
  • Later on when the GC is triggered that View does not have its correct "parentNode" (the one that was set when the creation of createEmbeddedView was called) and it cannot be correctly removed from the "queue".

So this fix makes sure that even if we "remove a view" from the UI by using the getSingleViewRecursive its "representation" in the internal elements queue will be correct and will be GC collectable.

@ghost ghost assigned SvetoslavTsenov Oct 11, 2018
@vakrilov
Copy link
Contributor

The fix does look a bit mystical, but I think it is correct.

As @VladimirAmiorkov explained - we do this trick of creating an element with angular API (createEmbeddedView) and then detaching the NS-view form the original place that angular put it and put it somewhere else (in this case give it to the ListView to handle it).
However, in the process of detaching we were breaking the parent->child relation that angular renderer knows about. So it fails to clear the references when it needs to (on ViewContainerRef.clear()).

With this fix we preserve this relation trough parentNode so that clear() can do its job.

@sis0k0 Can you also take a look? Do you think this fix might have "side-effects"?

@NathanaelA

This comment was marked as abuse.

@vakrilov
Copy link
Contributor

Hey @NathanaelA, here is the plan:

  1. As soon as this gets merged in master it will be published in nativescript-angular@next
  2. nativescript-angular package versioning and releases are more in sync with @angular. We plan to release [email protected] using angular@7-RC somewhere around the end of the month
  3. When angular 7 official is out, we will publish [email protected]. From what we've tested from the RC builds - we don't expect any problems with migration issues so we ho[e that we can follow shortly after angular is published.

This fix should be in all of the above releases

@NathanaelA

This comment was marked as abuse.

@SvetoslavTsenov
Copy link
Contributor

test build_ng_routable_animations build_ng_animation_examples

@vakrilov vakrilov merged commit 0b8d2c5 into master Oct 18, 2018
@vakrilov vakrilov deleted the amiorkov/embedded-views-memory-leak-fix branch October 18, 2018 21:29
@ghost ghost removed the in progress label Oct 18, 2018
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.

4 participants