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(router): prevent memory leak by removing app reference from $router.apps once app destroyed (Fixes #2639) #2640

Open
wants to merge 3 commits into
base: dev
from

Conversation

Projects
None yet
2 participants
@tmorehouse
Copy link

tmorehouse commented Mar 5, 2019

Prevent memory leaks by removing app(s) from $router.apps array once app has been destroyed.

Does not remove main app (this.app) from this.apps when it is destroyed through, but maybe it should (remove from both $router.apps array and $router.app?).

Fixes #2639

tmorehouse added some commits Mar 5, 2019

fix(router): remove app from $router.apps array once app destroyed (F…
…ixes #2639)


Prevent destroyed apps from causing memory leak in `$router.apps` array.

Fixes #2639
@tmorehouse

This comment has been minimized.

Copy link
Author

tmorehouse commented Mar 5, 2019

Not quite sure why the netlify deploy is failing... Although it apperas to be failing on all new PRs for the last 4 days.

@tmorehouse tmorehouse changed the title fix(router): remove app from $router.apps array once app destroyed (Fixes #2639) fix(router): prevent memory leak by removing app reference from $router.apps once app destroyed (Fixes #2639) Mar 5, 2019

@@ -91,6 +91,11 @@ export default class VueRouter {

// main app already initialized.
if (this.app) {
app.$once('hook:destroyed', () => {

This comment has been minimized.

@posva

posva Mar 5, 2019

Member

if you add it inside the if, creating an app and then destroying the main app after it would still result in a memory leak, right?

Could you add some tests to api.spec.js or another new file checking the apps array is emptied correctly, please?

This comment has been minimized.

@tmorehouse

tmorehouse Mar 5, 2019

Author

The main app, would typically be destroyed last (I would think)

The reason for placing it inside of the if would be that it would t destroy the main app, only apps that came along after the main app was instantiated.

This comment has been minimized.

@tmorehouse

tmorehouse Mar 5, 2019

Author

I'll see what I can do a bout adding a few tests.

This comment has been minimized.

@tmorehouse

tmorehouse Mar 5, 2019

Author

Another question would be, what if the main app is destroyed y the other secondary apps are still alive? Should the main app be removed as well, and should the next app in the apps array (with the lowest index number) be copied to this.app ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.