Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

fix(panel): destroy the scope when the panelRef is destroyed #8848

Closed
wants to merge 1 commit into from

Conversation

crisbeto
Copy link
Member

The panel wasn't destroying its' scope within the destroy method, causing potential memory leaks.

Fixes #8845.

CC @ErinCoughlan

The panel wasn't destroying its' scope within the `destroy` method, causing potential memory leaks.

Fixes angular#8845.
@crisbeto crisbeto added the needs: review This PR is waiting on review from the team label Jun 24, 2016
@ErinCoughlan
Copy link
Contributor

LGTM Thanks @crisbeto

@clshortfuse
Copy link
Contributor

This could be an issue if you actually want to close the panel and not destroy it. I'm in the process of porting my custom component to panel. In the interest of reusing and precaching components, I would like a panel to close, but not destroy, and then call it to open again.

But I do think destroy should be the default operation, but there should be an option such as autoDestroy.

@ErinCoughlan
Copy link
Contributor

@clshortfuse Destroy is still a separate method from open/close and show/hide. If you want to reuse components, you would call any of those methods and only call destroy when you truly want to kill it.

@clshortfuse
Copy link
Contributor

Ah, I see it now. Should panels autoDestroy in that case then? I can foresee a lot of users not expecting that and inadvertently making memory leaks. If angular does destroy it when it gets garbage collected, then that's fine too.

@ErinCoughlan
Copy link
Contributor

I don't like autoDestroy since it adds additional configuration. I don't think it's any clearer to tell the users to set some field on the config object vs telling them to call a method. The panel is low level enough that anyone using it should create a directive that they can then use throughout their app, so they only need to know once.

I think making destroy behavior clearer in the documentation could help. All of the examples also include destroy so if someone forks it, they will have a working version.

@clshortfuse
Copy link
Contributor

+1 for docs. Perhaps even comments in the Basic Usage code.

@crisbeto
Copy link
Member Author

crisbeto commented Jun 29, 2016

I've mentioned in the docs for the close method that it doesn't destroy the instance.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs: review This PR is waiting on review from the team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants