Skip to content

Commit

Permalink
fix: prevent memory leaks by removing app references (#2706)
Browse files Browse the repository at this point in the history
Fix #2639
  • Loading branch information
posva authored Apr 12, 2019
1 parent 627027f commit 8056105
Show file tree
Hide file tree
Showing 2 changed files with 108 additions and 1 deletion.
14 changes: 13 additions & 1 deletion src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,19 @@ export default class VueRouter {

this.apps.push(app)

// main app already initialized.
// set up app destroyed handler
// https://github.com/vuejs/vue-router/issues/2639
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 or null if no apps
// we do not release the router so it can be reused
if (this.app === app) this.app = this.apps[0] || null
})

// main app previously initialized
// return as we don't need to set up new history listener
if (this.app) {
return
}
Expand Down
95 changes: 95 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,97 @@ describe('router.push/replace callbacks', () => {
})
})
})

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

let router, app1, app2, app3

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

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

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

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

it('all apps point to the same router instance', () => {
expect(app1.$router).toBe(app2.$router)
expect(app2.$router).toBe(app3.$router)
})

it('should have all 3 registered apps', () => {
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.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 and replace current app', () => {
app1.$destroy()
expect(app3.$router.app).toBe(app2)
expect(app3.$router.apps.length).toBe(2)
expect(app3.$router.apps[0]).toBe(app2)
expect(app1.$router.apps[1]).toBe(app3)
})

it('should remove all apps', () => {
app1.$destroy()
app3.$destroy()
app2.$destroy()
expect(app3.$router.app).toBe(null)
expect(app3.$router.apps.length).toBe(0)
})

it('should keep current app if already defined', () => {
const app4 = new Vue({
router,
render (h) { return h('div') }
})
expect(app4.$router.app).toBe(app1)
expect(app4.$router.apps.length).toBe(4)
expect(app4.$router.apps[3]).toBe(app4)
})

it('should replace current app if none is assigned when creating the app', () => {
app1.$destroy()
app3.$destroy()
app2.$destroy()
const app4 = new Vue({
router,
render (h) { return h('div') }
})
expect(router.app).toBe(app4)
expect(app4.$router).toBe(router)
expect(app4.$router.apps.length).toBe(1)
expect(app4.$router.apps[0]).toBe(app4)
})
})

0 comments on commit 8056105

Please sign in to comment.