Skip to content

fix(router): prevent memory leaks by removing app references from $router.apps once app destroyed (Fixes #2639) #2640

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
wants to merge 14 commits into from
Closed
16 changes: 15 additions & 1 deletion src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,22 @@ export default class VueRouter {

this.apps.push(app)

// main app already initialized.
// set up app destroyed handler
app.$once('hook:destroyed', () => {
// clean out app from this.apps array once destroyed
const index = this.apps.indexOf(app)
if (index > -1) this.apps.splice(index, 1)
// ensure we still have a main app, unless this is the last app left
if (this.apps.length > 0) this.app = this.apps[0]
})

// main app previously initialized
if (this.app) {
if (this.app !== this.apps[0]) {
// main app had been destroyed, so replace it with this new app
this.app = this.apps[0]
}
// return as we don't need to set up new history listener
return
}

Expand Down
87 changes: 87 additions & 0 deletions test/unit/specs/api.spec.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import Router from '../../../src/index'
import Vue from 'vue'

describe('router.onReady', () => {
it('should work', done => {
Expand Down Expand Up @@ -185,3 +186,89 @@ describe('router.push/replace callbacks', () => {
})
})
})

describe('router app destroy handling', () => {
Vue.use(Router)

const router = new Router({
mode: 'abstract',
routes: [
{ path: '/', component: { name: 'A' }}
]
})

// Add main app
const app1 = new Vue({
router,
render (h) { return h('div') }
})

// Add 2nd app
const app2 = new Vue({
router,
render (h) { return h('div') }
})

// Add 3rd app
const app3 = new Vue({
router,
render (h) { return h('div') }
})

it('router and apps should be defined', () => {
expect(router).toBeDefined()
expect(router instanceof Router).toBe(true)
expect(app1).toBeDefined()
expect(app1 instanceof Vue).toBe(true)
expect(app2).toBeDefined()
expect(app2 instanceof Vue).toBe(true)
expect(app3).toBeDefined()
expect(app3 instanceof Vue).toBe(true)
expect(app1.$router.apps).toBe(app2.$router.apps)
expect(app2.$router.apps).toBe(app3.$router.apps)
expect(app1.$router.app).toBe(app2.$router.app)
expect(app2.$router.app).toBe(app3.$router.app)
})

it('should have 3 registered apps', () => {
expect(app1.$router).toBeDefined()
expect(app1.$router.app).toBe(app1)
expect(app1.$router.apps.length).toBe(3)
expect(app1.$router.apps[0]).toBe(app1)
expect(app1.$router.apps[1]).toBe(app2)
expect(app1.$router.apps[2]).toBe(app3)
})

it('should remove 2nd destroyed app from this.apps', () => {
app2.$destroy()
expect(app1.$router).toBeDefined()
expect(app1.$router.app).toBeDefined()
expect(app1.$router.app).toBe(app1)
expect(app1.$router.apps.length).toBe(2)
expect(app1.$router.apps[0]).toBe(app1)
expect(app1.$router.apps[1]).toBe(app3)
})

it('should remove 1st destroyed app from this.apps and replace this.app', () => {
app1.$destroy()
expect(app3.$router.app).toBe(app3)
expect(app3.$router.apps.length).toBe(1)
expect(app3.$router.apps[0]).toBe(app3)
})

it('should remove last destroyed app from this.apps', () => {
app3.$destroy()
expect(app3.$router.app).toBe(app3)
expect(app3.$router.apps.length).toBe(0)
})

it('should replace app with new app', () => {
const app4 = new Vue({
router,
render (h) { return h('div') }
})
expect(app4.$router.app).toBe(app4)
expect(app4.$router.apps.length).toBe(1)
expect(app4.$router.apps[0]).toBe(app4)
})
})