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 leaks by removing app references #2706

Merged
merged 15 commits into from
Apr 12, 2019
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
}

Choose a reason for hiding this comment

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

With this.app being set to null (if all apps are cleared out), this early return doesn't happen, and the code below it will be executed again.

Will that be an issue? IF so, maybe a flag needs to be set so that the following code is not executed a second time.

Copy link
Member Author

Choose a reason for hiding this comment

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

but this is called when a router is injected in a created component, it's only executed once per root instance

Choose a reason for hiding this comment

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

Doesn't init get called for each app instance, as it is how the apps get added to this.apps array:

  init (app: any /* Vue component instance */) {
    process.env.NODE_ENV !== 'production' && assert(
      install.installed,
      `not installed. Make sure to call \`Vue.use(VueRouter)\` ` +
      `before creating root instance.`
    )

    this.apps.push(app)

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, but once per root instance

Choose a reason for hiding this comment

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

Ah, correct... so it should be good.

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)
})
})