From 836c009d08e62af216956a8bd5288c7c1d92fbe6 Mon Sep 17 00:00:00 2001 From: Troy Morehouse Date: Tue, 5 Mar 2019 03:37:38 -0400 Subject: [PATCH 01/15] fix(router): remove app from $router.apps array once app destroyed (Fixes #2639) Prevent destroyed apps from causing memory leak in `$router.apps` array. Fixes #2639 --- src/index.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/index.js b/src/index.js index 69c3cc0c8..5c9d9abfc 100644 --- a/src/index.js +++ b/src/index.js @@ -91,7 +91,12 @@ export default class VueRouter { // main app already initialized. if (this.app) { - return + 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) + }) + return } this.app = app From 642e80d7434a70d8f83e14b17327938a1e0197b0 Mon Sep 17 00:00:00 2001 From: Troy Morehouse Date: Tue, 5 Mar 2019 03:42:12 -0400 Subject: [PATCH 02/15] lint --- src/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/index.js b/src/index.js index 5c9d9abfc..1d5afaca1 100644 --- a/src/index.js +++ b/src/index.js @@ -91,7 +91,7 @@ export default class VueRouter { // main app already initialized. if (this.app) { - app.$once('hook:destroyed', () { + 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) From a8cd5af4522f417a785f0770551d7a7f13d7c28f Mon Sep 17 00:00:00 2001 From: Troy Morehouse Date: Tue, 5 Mar 2019 03:44:32 -0400 Subject: [PATCH 03/15] lint --- src/index.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/index.js b/src/index.js index 1d5afaca1..726cce87a 100644 --- a/src/index.js +++ b/src/index.js @@ -91,12 +91,12 @@ export default class VueRouter { // main app already initialized. if (this.app) { - 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) - }) - return + 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) + }) + return } this.app = app From ed1d5d006d594476a1cbfa738ba26fef7893c1d0 Mon Sep 17 00:00:00 2001 From: Troy Morehouse Date: Fri, 29 Mar 2019 23:59:55 -0300 Subject: [PATCH 04/15] Update index.js --- src/index.js | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/index.js b/src/index.js index 726cce87a..af5934cf9 100644 --- a/src/index.js +++ b/src/index.js @@ -89,13 +89,16 @@ export default class VueRouter { this.apps.push(app) + // Clean out app from this.apps array once destroyed + app.$once('hook:destroyed', () => { + const index = this.apps.indexOf(app) + if (index > -1) this.apps.splice(index, 1) + // Ensure we still have a main app + if (this.apps[0]) this.app = this.apps[0] + }) + // main app already initialized. if (this.app) { - 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) - }) return } From 48b69fbba4051db87237ecbed3f82c3425cfc5c4 Mon Sep 17 00:00:00 2001 From: Troy Morehouse Date: Sat, 30 Mar 2019 01:08:59 -0300 Subject: [PATCH 05/15] better handling of last app destroyed Prevents history listeners from being set up multiple times --- src/index.js | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/index.js b/src/index.js index af5934cf9..a4c876f90 100644 --- a/src/index.js +++ b/src/index.js @@ -89,16 +89,22 @@ export default class VueRouter { this.apps.push(app) - // Clean out app from this.apps array once destroyed + // 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 - if (this.apps[0]) this.app = this.apps[0] + // 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 already initialized. + // 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 } From bca252bc3905c3fdfc2df612e025dfee62eea80d Mon Sep 17 00:00:00 2001 From: Troy Morehouse Date: Sat, 30 Mar 2019 02:06:14 -0300 Subject: [PATCH 06/15] add app destroy testing --- test/unit/specs/api.spec.js | 65 +++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+) diff --git a/test/unit/specs/api.spec.js b/test/unit/specs/api.spec.js index 5fce2e885..e777f910d 100644 --- a/test/unit/specs/api.spec.js +++ b/test/unit/specs/api.spec.js @@ -1,4 +1,5 @@ import Router from '../../../src/index' +import Vue from 'vue' describe('router.onReady', () => { it('should work', done => { @@ -185,3 +186,67 @@ describe('router.push/replace callbacks', () => { }) }) }) + +describe('router app destroy handling', () => { + it('should remove destroyed apps from this.apps', () => { + const router = new Router({ + mode: 'abstract', + routes: [ + { path: '/', component: { name: 'A' }} + ] + }) + + const options = { + router, + render(h) { return h('div') } + } + + expect(router.apps.length).toBe(0) + expect(router.app).toBe(null) + + // Add main app + const app1 = new Vue(options) + expect(router.app).toBe(app1) + expect(router.apps.length).toBe(1) + expect(router.app[0]).toBe(app1) + + // Add 2nd app + const app2 = new Vue(options) + expect(router.app).toBe(app1) + expect(router.apps.length).toBe(2) + expect(router.app[0]).toBe(app1) + expect(router.app[1]).toBe(app2) + + // Add 3rd app + const app3 = new Vue(options) + expect(router.app).toBe(app1) + expect(router.apps.length).toBe(3) + expect(router.app[0]).toBe(app1) + expect(router.app[1]).toBe(app2) + expect(router.app[2]).toBe(app3) + + // Destroy second app + app2.destroy() + expect(router.app).toBe(app1) + expect(router.apps.length).toBe(2) + expect(router.app[0]).toBe(app1) + expect(router.app[1]).toBe(app3) + + // Destroy 1st app + app1.destroy() + expect(router.app).toBe(app3) + expect(router.apps.length).toBe(1) + expect(router.app[0]).toBe(app3) + + // Destroy 3rd app + app3.destroy() + expect(router.app).toBe(app3) + expect(router.apps.length).toBe(0) + + // Add 4th app (should be the only app) + const app4 = new Vue(options) + expect(router.app).toBe(app4) + expect(router.apps.length).toBe(1) + expect(router.app[0]).toBe(app4) + }) +}) From 2696c1086968782bb43b75105cb19ccafb294fe0 Mon Sep 17 00:00:00 2001 From: Troy Morehouse Date: Sat, 30 Mar 2019 02:13:19 -0300 Subject: [PATCH 07/15] Update api.spec.js --- test/unit/specs/api.spec.js | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/test/unit/specs/api.spec.js b/test/unit/specs/api.spec.js index e777f910d..f96b9ae76 100644 --- a/test/unit/specs/api.spec.js +++ b/test/unit/specs/api.spec.js @@ -196,29 +196,33 @@ describe('router app destroy handling', () => { ] }) - const options = { - router, - render(h) { return h('div') } - } - expect(router.apps.length).toBe(0) expect(router.app).toBe(null) // Add main app - const app1 = new Vue(options) + const app1 = new Vue({ + router, + render (h) { return h('div') } + }) expect(router.app).toBe(app1) expect(router.apps.length).toBe(1) expect(router.app[0]).toBe(app1) // Add 2nd app - const app2 = new Vue(options) + const app2 = new Vue({ + router, + render (h) { return h('div') } + }) expect(router.app).toBe(app1) expect(router.apps.length).toBe(2) expect(router.app[0]).toBe(app1) expect(router.app[1]).toBe(app2) // Add 3rd app - const app3 = new Vue(options) + const app3 = new Vue({ + router, + render (h) { return h('div') } + }) expect(router.app).toBe(app1) expect(router.apps.length).toBe(3) expect(router.app[0]).toBe(app1) @@ -226,25 +230,28 @@ describe('router app destroy handling', () => { expect(router.app[2]).toBe(app3) // Destroy second app - app2.destroy() + app2.$destroy() expect(router.app).toBe(app1) expect(router.apps.length).toBe(2) expect(router.app[0]).toBe(app1) expect(router.app[1]).toBe(app3) // Destroy 1st app - app1.destroy() + app1.$destroy() expect(router.app).toBe(app3) expect(router.apps.length).toBe(1) expect(router.app[0]).toBe(app3) // Destroy 3rd app - app3.destroy() + app3.$destroy() expect(router.app).toBe(app3) expect(router.apps.length).toBe(0) // Add 4th app (should be the only app) - const app4 = new Vue(options) + const app4 = new Vue({ + router, + render (h) { return h('div') } + }) expect(router.app).toBe(app4) expect(router.apps.length).toBe(1) expect(router.app[0]).toBe(app4) From fe214b2aee4735e019a34cc81e5b8d6803c99874 Mon Sep 17 00:00:00 2001 From: Troy Morehouse Date: Sat, 30 Mar 2019 02:24:07 -0300 Subject: [PATCH 08/15] Update api.spec.js --- test/unit/specs/api.spec.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/unit/specs/api.spec.js b/test/unit/specs/api.spec.js index f96b9ae76..ca5e8b61d 100644 --- a/test/unit/specs/api.spec.js +++ b/test/unit/specs/api.spec.js @@ -189,6 +189,8 @@ describe('router.push/replace callbacks', () => { describe('router app destroy handling', () => { it('should remove destroyed apps from this.apps', () => { + Vue.use(Router) + const router = new Router({ mode: 'abstract', routes: [ From 87edd99d3db770eb91262528c4c5ddfd20eaa1e0 Mon Sep 17 00:00:00 2001 From: Troy Morehouse Date: Sat, 30 Mar 2019 02:46:10 -0300 Subject: [PATCH 09/15] Update api.spec.js --- test/unit/specs/api.spec.js | 119 ++++++++++++++++++++---------------- 1 file changed, 66 insertions(+), 53 deletions(-) diff --git a/test/unit/specs/api.spec.js b/test/unit/specs/api.spec.js index ca5e8b61d..d9e15b69c 100644 --- a/test/unit/specs/api.spec.js +++ b/test/unit/specs/api.spec.js @@ -188,74 +188,87 @@ describe('router.push/replace callbacks', () => { }) describe('router app destroy handling', () => { - it('should remove destroyed apps from this.apps', () => { - Vue.use(Router) + Vue.use(Router) - const router = new Router({ - mode: 'abstract', - routes: [ - { path: '/', component: { name: 'A' }} - ] - }) + const router = new Router({ + mode: 'abstract', + routes: [ + { path: '/', component: { name: 'A' }} + ] + }) - expect(router.apps.length).toBe(0) - expect(router.app).toBe(null) + // Add main app + const app1 = new Vue({ + router, + render (h) { return h('div') } + }) - // Add main app - const app1 = new Vue({ - router, - render (h) { return h('div') } - }) - expect(router.app).toBe(app1) - expect(router.apps.length).toBe(1) - expect(router.app[0]).toBe(app1) + // Add 2nd app + const app2 = new Vue({ + router, + render (h) { return h('div') } + }) - // Add 2nd app - const app2 = new Vue({ - router, - render (h) { return h('div') } - }) - expect(router.app).toBe(app1) - expect(router.apps.length).toBe(2) - expect(router.app[0]).toBe(app1) - expect(router.app[1]).toBe(app2) + // Add 3rd app + const app3 = new Vue({ + router, + render (h) { return h('div') } + }) - // Add 3rd app - const app3 = new Vue({ - router, - render (h) { return h('div') } - }) - expect(router.app).toBe(app1) - expect(router.apps.length).toBe(3) - expect(router.app[0]).toBe(app1) - expect(router.app[1]).toBe(app2) - expect(router.app[2]).toBe(app3) + it('router and apps should be defined', async () => { + expect(router).toBeDefined() + expect(router).toBeInstanceOf(Router) + expect(app1).toBeDefined() + expect(app1).toBeInstanceOf(Vue) + expect(app2).toBeDefined() + expect(app2).toBeInstanceOf(Vue) + expect(app3).toBeDefined() + expect(app3).toBeInstanceOf(Vue) + expect(app1.$router.apps).toBe(app2.$router.apps) + expect(app2.$router.apps).toBe(app3.$router.apps) + }) + + it('should have 3 registered apps', async () => { + 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) + }) - // Destroy second app + it('should remove 2nd destroyed app from this.apps', async () => { app2.$destroy() - expect(router.app).toBe(app1) - expect(router.apps.length).toBe(2) - expect(router.app[0]).toBe(app1) - expect(router.app[1]).toBe(app3) + await Vue.nextTick() + expect(app1.$router.app).toBe(app1) + expect(app1.$router.apps.length).toBe(2) + expect(app1.$router.app[0]).toBe(app1) + expect(app1.$router.app[1]).toBe(app3) + }) - // Destroy 1st app + it('should remove 1st destroyed app from this.apps and replace this.app', async () => { app1.$destroy() - expect(router.app).toBe(app3) - expect(router.apps.length).toBe(1) - expect(router.app[0]).toBe(app3) + await Vue.nextTick() + expect(app3.$router.app).toBe(app3) + expect(app3.$router.apps.length).toBe(1) + expect(app3.$router.app[0]).toBe(app3) + }) - // Destroy 3rd app + it('should remove last destroyed app from this.apps', async () => { app3.$destroy() - expect(router.app).toBe(app3) - expect(router.apps.length).toBe(0) + await Vue.nextTick() + expect(app3.$router.app).toBe(app3) + expect(app3.$router.apps.length).toBe(0) + }) - // Add 4th app (should be the only app) + it('should replace app with new app', async () => { const app4 = new Vue({ router, render (h) { return h('div') } }) - expect(router.app).toBe(app4) - expect(router.apps.length).toBe(1) - expect(router.app[0]).toBe(app4) + await Vue.nextTick() + expect(app4.$router.app).toBe(app4) + expect(app4.$router.apps.length).toBe(1) + expect(app4.$router.app[0]).toBe(app4) }) }) From aa204e9b38642ff1a3458610c77a4b8da00fd836 Mon Sep 17 00:00:00 2001 From: Troy Morehouse Date: Sat, 30 Mar 2019 02:52:54 -0300 Subject: [PATCH 10/15] Update api.spec.js --- test/unit/specs/api.spec.js | 16 ++++++---------- 1 file changed, 6 insertions(+), 10 deletions(-) diff --git a/test/unit/specs/api.spec.js b/test/unit/specs/api.spec.js index d9e15b69c..30b9a7752 100644 --- a/test/unit/specs/api.spec.js +++ b/test/unit/specs/api.spec.js @@ -215,7 +215,7 @@ describe('router app destroy handling', () => { render (h) { return h('div') } }) - it('router and apps should be defined', async () => { + it('router and apps should be defined', () => { expect(router).toBeDefined() expect(router).toBeInstanceOf(Router) expect(app1).toBeDefined() @@ -228,7 +228,7 @@ describe('router app destroy handling', () => { expect(app2.$router.apps).toBe(app3.$router.apps) }) - it('should have 3 registered apps', async () => { + it('should have 3 registered apps', () => { expect(app1.$router).toBeDefined() expect(app1.$router.app).toBe(app1) expect(app1.$router.apps.length).toBe(3) @@ -237,36 +237,32 @@ describe('router app destroy handling', () => { expect(app1.$router.apps[2]).toBe(app3) }) - it('should remove 2nd destroyed app from this.apps', async () => { + it('should remove 2nd destroyed app from this.apps', () => { app2.$destroy() - await Vue.nextTick() expect(app1.$router.app).toBe(app1) expect(app1.$router.apps.length).toBe(2) expect(app1.$router.app[0]).toBe(app1) expect(app1.$router.app[1]).toBe(app3) }) - it('should remove 1st destroyed app from this.apps and replace this.app', async () => { + it('should remove 1st destroyed app from this.apps and replace this.app', () => { app1.$destroy() - await Vue.nextTick() expect(app3.$router.app).toBe(app3) expect(app3.$router.apps.length).toBe(1) expect(app3.$router.app[0]).toBe(app3) }) - it('should remove last destroyed app from this.apps', async () => { + it('should remove last destroyed app from this.apps', () => { app3.$destroy() - await Vue.nextTick() expect(app3.$router.app).toBe(app3) expect(app3.$router.apps.length).toBe(0) }) - it('should replace app with new app', async () => { + it('should replace app with new app', () => { const app4 = new Vue({ router, render (h) { return h('div') } }) - await Vue.nextTick() expect(app4.$router.app).toBe(app4) expect(app4.$router.apps.length).toBe(1) expect(app4.$router.app[0]).toBe(app4) From b540487671b319e813d93910a86526baeb3b3a2a Mon Sep 17 00:00:00 2001 From: Troy Morehouse Date: Sat, 30 Mar 2019 02:59:37 -0300 Subject: [PATCH 11/15] Update api.spec.js --- test/unit/specs/api.spec.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/unit/specs/api.spec.js b/test/unit/specs/api.spec.js index 30b9a7752..ef3f3d51d 100644 --- a/test/unit/specs/api.spec.js +++ b/test/unit/specs/api.spec.js @@ -217,15 +217,17 @@ describe('router app destroy handling', () => { it('router and apps should be defined', () => { expect(router).toBeDefined() - expect(router).toBeInstanceOf(Router) + expect(router istanceof Router).toBe(true) expect(app1).toBeDefined() - expect(app1).toBeInstanceOf(Vue) + expect(app1 instanceof Vue).toBe(true) expect(app2).toBeDefined() - expect(app2).toBeInstanceOf(Vue) + expect(app2 instanceof Vue).toBe(true) expect(app3).toBeDefined() - expect(app3).toBeInstanceOf(Vue) + 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', () => { From a31748ee5220246aba7ec9b3494d960ab66491cb Mon Sep 17 00:00:00 2001 From: Troy Morehouse Date: Sat, 30 Mar 2019 03:02:16 -0300 Subject: [PATCH 12/15] Update api.spec.js --- test/unit/specs/api.spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/unit/specs/api.spec.js b/test/unit/specs/api.spec.js index ef3f3d51d..63b172d0e 100644 --- a/test/unit/specs/api.spec.js +++ b/test/unit/specs/api.spec.js @@ -217,7 +217,7 @@ describe('router app destroy handling', () => { it('router and apps should be defined', () => { expect(router).toBeDefined() - expect(router istanceof Router).toBe(true) + expect(router instanceof Router).toBe(true) expect(app1).toBeDefined() expect(app1 instanceof Vue).toBe(true) expect(app2).toBeDefined() From f49415e0cba77c860efcb2329a84856495895d9a Mon Sep 17 00:00:00 2001 From: Troy Morehouse Date: Sat, 30 Mar 2019 03:06:26 -0300 Subject: [PATCH 13/15] Update api.spec.js --- test/unit/specs/api.spec.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/test/unit/specs/api.spec.js b/test/unit/specs/api.spec.js index 63b172d0e..75abdb2cd 100644 --- a/test/unit/specs/api.spec.js +++ b/test/unit/specs/api.spec.js @@ -241,17 +241,19 @@ describe('router app destroy handling', () => { 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.app[0]).toBe(app1) - expect(app1.$router.app[1]).toBe(app3) + 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.app[0]).toBe(app3) + expect(app3.$router.apps[0]).toBe(app3) }) it('should remove last destroyed app from this.apps', () => { @@ -267,6 +269,6 @@ describe('router app destroy handling', () => { }) expect(app4.$router.app).toBe(app4) expect(app4.$router.apps.length).toBe(1) - expect(app4.$router.app[0]).toBe(app4) + expect(app4.$router.apps[0]).toBe(app4) }) }) From 009736d9f743ee3d03116f769275e3d7e49cdaa9 Mon Sep 17 00:00:00 2001 From: Eduardo San Martin Morote Date: Thu, 11 Apr 2019 12:07:06 +0200 Subject: [PATCH 14/15] refactor: code review --- src/index.js | 16 +++--- test/unit/specs/api.spec.js | 98 ++++++++++++++++++++----------------- 2 files changed, 62 insertions(+), 52 deletions(-) diff --git a/src/index.js b/src/index.js index a4c876f90..8ca98003c 100644 --- a/src/index.js +++ b/src/index.js @@ -90,21 +90,23 @@ export default class VueRouter { this.apps.push(app) // 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, unless this is the last app left - if (this.apps.length > 0) this.app = this.apps[0] + // ensure we still have a main app + if (this.app === app) this.app = this.apps[0] || null + // no more apps, the router can be released + if (!this.app) { + // TODO: destroy router, listeners? + // Maybe the user wants to reuse the router though + } }) // main app previously initialized + // return as we don't need to set up new history listener 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 } diff --git a/test/unit/specs/api.spec.js b/test/unit/specs/api.spec.js index 75abdb2cd..1fba4977b 100644 --- a/test/unit/specs/api.spec.js +++ b/test/unit/specs/api.spec.js @@ -190,48 +190,41 @@ describe('router.push/replace callbacks', () => { describe('router app destroy handling', () => { Vue.use(Router) - const router = new Router({ - mode: 'abstract', - routes: [ - { path: '/', component: { name: 'A' }} - ] - }) + let router, app1, app2, app3 - // Add main app - const app1 = new Vue({ - router, - render (h) { return h('div') } - }) + beforeEach(() => { + router = new Router({ + mode: 'abstract', + routes: [ + { path: '/', component: { name: 'A' }} + ] + }) - // Add 2nd app - const app2 = new Vue({ - router, - render (h) { return h('div') } - }) + // 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 - const app3 = new Vue({ - router, - render (h) { return h('div') } + // Add 3rd app + 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('all apps point to the same router instance', () => { + expect(app1.$router).toBe(app2.$router) + expect(app2.$router).toBe(app3.$router) }) - it('should have 3 registered apps', () => { - expect(app1.$router).toBeDefined() + 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) @@ -241,33 +234,48 @@ describe('router app destroy handling', () => { 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', () => { + it('should remove 1st destroyed app and replace current app', () => { app1.$destroy() - expect(app3.$router.app).toBe(app3) - expect(app3.$router.apps.length).toBe(1) - expect(app3.$router.apps[0]).toBe(app3) + 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 last destroyed app from this.apps', () => { + it('should remove all apps', () => { + app1.$destroy() app3.$destroy() - expect(app3.$router.app).toBe(app3) + app2.$destroy() + expect(app3.$router.app).toBe(null) expect(app3.$router.apps.length).toBe(0) }) - it('should replace app with new app', () => { + 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(app4.$router.app).toBe(app4) + expect(router.app).toBe(app4) + expect(app4.$router).toBe(router) expect(app4.$router.apps.length).toBe(1) expect(app4.$router.apps[0]).toBe(app4) }) From 47c0920d53d4f251a09446faaf924a7150bc6854 Mon Sep 17 00:00:00 2001 From: Eduardo San Martin Morote Date: Thu, 11 Apr 2019 12:09:47 +0200 Subject: [PATCH 15/15] refactor: remove unused code --- src/index.js | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/index.js b/src/index.js index 8ca98003c..b3eb25dd3 100644 --- a/src/index.js +++ b/src/index.js @@ -95,13 +95,9 @@ export default class VueRouter { // 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 + // 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 - // no more apps, the router can be released - if (!this.app) { - // TODO: destroy router, listeners? - // Maybe the user wants to reuse the router though - } }) // main app previously initialized