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

feat(history): Remove event listeners when all apps are destroyed. #3172

Merged
merged 7 commits into from
May 19, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions examples/basic/app.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,30 @@
import Vue from 'vue'
import VueRouter from 'vue-router'

// track number of popstate listeners
joeldenning marked this conversation as resolved.
Show resolved Hide resolved
let numPopstateListeners = 0
const listenerCountDiv = document.createElement('div')
listenerCountDiv.id = 'popstate-count'
listenerCountDiv.textContent = numPopstateListeners + ' popstate listeners'
document.body.appendChild(listenerCountDiv)

const originalAddEventListener = window.addEventListener
const originalRemoveEventListener = window.removeEventListener
window.addEventListener = function (name, handler) {
if (name === 'popstate') {
listenerCountDiv.textContent =
++numPopstateListeners + ' popstate listeners'
}
return originalAddEventListener.apply(this, arguments)
}
window.removeEventListener = function (name, handler) {
if (name === 'popstate') {
listenerCountDiv.textContent =
--numPopstateListeners + ' popstate listeners'
}
return originalRemoveEventListener.apply(this, arguments)
}

// 1. Use plugin.
// This installs <router-view> and <router-link>,
// and injects $router and $route to all router-enabled child components
Expand Down Expand Up @@ -55,6 +79,7 @@ new Vue({
<pre id="query-t">{{ $route.query.t }}</pre>
<pre id="hash">{{ $route.hash }}</pre>
<router-view class="view"></router-view>
<button id="teardown-app" v-on:click="teardown">Teardown app</button>
</div>
`,

Expand All @@ -66,6 +91,10 @@ new Vue({
} else {
this.$router.push('/', increment)
}
},
teardown () {
this.$destroy()
this.$el.innerHTML = ''
}
}
}).$mount('#app')
33 changes: 32 additions & 1 deletion examples/hash-mode/app.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,30 @@
import Vue from 'vue'
import VueRouter from 'vue-router'

// track number of popstate listeners
let numPopstateListeners = 0
const listenerCountDiv = document.createElement('div')
listenerCountDiv.id = 'popstate-count'
listenerCountDiv.textContent = numPopstateListeners + ' popstate listeners'
document.body.appendChild(listenerCountDiv)

const originalAddEventListener = window.addEventListener
const originalRemoveEventListener = window.removeEventListener
window.addEventListener = function (name, handler) {
if (name === 'popstate') {
listenerCountDiv.textContent =
++numPopstateListeners + ' popstate listeners'
}
return originalAddEventListener.apply(this, arguments)
}
window.removeEventListener = function (name, handler) {
if (name === 'popstate') {
listenerCountDiv.textContent =
--numPopstateListeners + ' popstate listeners'
}
return originalRemoveEventListener.apply(this, arguments)
}

// 1. Use plugin.
// This installs <router-view> and <router-link>,
// and injects $router and $route to all router-enabled child components
Expand Down Expand Up @@ -46,6 +70,13 @@ new Vue({
<pre id="query-t">{{ $route.query.t }}</pre>
<pre id="hash">{{ $route.hash }}</pre>
<router-view class="view"></router-view>
<button id="teardown-app" v-on:click="teardown">Teardown app</button>
</div>
`
`,
methods: {
teardown () {
this.$destroy()
this.$el.innerHTML = ''
}
}
}).$mount('#app')
15 changes: 15 additions & 0 deletions src/history/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,16 @@ export class History {
readyCbs: Array<Function>
readyErrorCbs: Array<Function>
errorCbs: Array<Function>
listeners: Array<Function>
cleanupListeners: Function

// implemented by sub-classes
+go: (n: number) => void
+push: (loc: RawLocation) => void
+replace: (loc: RawLocation) => void
+ensureURL: (push?: boolean) => void
+getCurrentLocation: () => string
+setupListeners: Function

constructor (router: Router, base: ?string) {
this.router = router
Expand All @@ -41,6 +44,7 @@ export class History {
this.readyCbs = []
this.readyErrorCbs = []
this.errorCbs = []
this.listeners = []
}

listen (cb: Function) {
Expand Down Expand Up @@ -208,6 +212,17 @@ export class History {
hook && hook(route, prev)
})
}

setupListeners () {
// Default implementation is empty
}

teardownListeners () {
this.listeners.forEach(cleanupListener => {
cleanupListener()
})
this.listeners = []
}
}

function normalizeBase (base: ?string): string {
Expand Down
41 changes: 25 additions & 16 deletions src/history/hash.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,31 +20,40 @@ export class HashHistory extends History {
// this is delayed until the app mounts
// to avoid the hashchange listener being fired too early
setupListeners () {
if (this.listeners.length > 0) {
return
}

const router = this.router
const expectScroll = router.options.scrollBehavior
const supportsScroll = supportsPushState && expectScroll

if (supportsScroll) {
setupScroll()
this.listeners.push(setupScroll())
}

window.addEventListener(
supportsPushState ? 'popstate' : 'hashchange',
() => {
const current = this.current
if (!ensureSlash()) {
return
}
this.transitionTo(getHash(), route => {
if (supportsScroll) {
handleScroll(this.router, route, current, true)
}
if (!supportsPushState) {
replaceHash(route.fullPath)
}
})
const handleRoutingEvent = () => {
const current = this.current
if (!ensureSlash()) {
return
}
this.transitionTo(getHash(), route => {
if (supportsScroll) {
handleScroll(this.router, route, current, true)
}
if (!supportsPushState) {
replaceHash(route.fullPath)
}
})
}
const eventType = supportsPushState ? 'popstate' : 'hashchange'
window.addEventListener(
eventType,
handleRoutingEvent
)
this.listeners.push(() => {
window.removeEventListener(eventType, handleRoutingEvent)
})
}

push (location: RawLocation, onComplete?: Function, onAbort?: Function) {
Expand Down
25 changes: 21 additions & 4 deletions src/history/html5.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,24 +8,37 @@ import { setupScroll, handleScroll } from '../util/scroll'
import { pushState, replaceState, supportsPushState } from '../util/push-state'

export class HTML5History extends History {
+initLocation: string

constructor (router: Router, base: ?string) {
super(router, base)

this.initLocation = getLocation(this.base)
}

setupListeners () {
posva marked this conversation as resolved.
Show resolved Hide resolved
if (this.listeners.length > 0) {
return
}

const router = this.router
const expectScroll = router.options.scrollBehavior
const supportsScroll = supportsPushState && expectScroll

if (supportsScroll) {
setupScroll()
this.listeners.push(setupScroll())
}

const initLocation = getLocation(this.base)
posva marked this conversation as resolved.
Show resolved Hide resolved
window.addEventListener('popstate', e => {
const handleRoutingEvent = () => {
const expectScroll = router.options.scrollBehavior
const supportsScroll = supportsPushState && expectScroll
Copy link
Member

Choose a reason for hiding this comment

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

can't we remove these lines since they are right above? They shouldn't change


const current = this.current

// Avoiding first `popstate` event dispatched in some browsers but first
// history route not updated since async guard at the same time.
const location = getLocation(this.base)
if (this.current === START && location === initLocation) {
if (this.current === START && location === this.initLocation) {
return
}

Expand All @@ -34,6 +47,10 @@ export class HTML5History extends History {
handleScroll(router, route, current, true)
}
})
}
window.addEventListener('popstate', handleRoutingEvent)
this.listeners.push(() => {
window.removeEventListener('popstate', handleRoutingEvent)
})
}

Expand Down
18 changes: 9 additions & 9 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,12 @@ export default class VueRouter {
// 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

if (this.apps.length === 0) {
Copy link
Member

Choose a reason for hiding this comment

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

we should be able to just check !this.app

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 done

// clean up event listeners
// https://github.com/vuejs/vue-router/issues/2341
this.history.teardownListeners()
}
})

// main app previously initialized
Expand All @@ -110,17 +116,11 @@ export default class VueRouter {

const history = this.history

if (history instanceof HTML5History) {
history.transitionTo(history.getCurrentLocation())
} else if (history instanceof HashHistory) {
const setupHashListener = () => {
if (history instanceof HTML5History || history instanceof HashHistory) {
const setupListeners = () => {
history.setupListeners()
}
history.transitionTo(
history.getCurrentLocation(),
setupHashListener,
setupHashListener
)
history.transitionTo(history.getCurrentLocation(), setupListeners, setupListeners)
}

history.listen(route => {
Expand Down
17 changes: 11 additions & 6 deletions src/util/scroll.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,10 @@ export function setupScroll () {
const stateCopy = extend({}, window.history.state)
stateCopy.key = getStateKey()
window.history.replaceState(stateCopy, '', absolutePath)
window.addEventListener('popstate', e => {
saveScrollPosition()
if (e.state && e.state.key) {
setStateKey(e.state.key)
}
})
window.addEventListener('popstate', handlePopState)
return () => {
window.removeEventListener('popstate', handlePopState)
}
}

export function handleScroll (
Expand Down Expand Up @@ -86,6 +84,13 @@ export function saveScrollPosition () {
}
}

function handlePopState (e) {
saveScrollPosition()
if (e.state && e.state.key) {
setStateKey(e.state.key)
}
}

function getScrollPosition (): ?Object {
const key = getStateKey()
if (key) {
Expand Down
5 changes: 5 additions & 0 deletions test/e2e/specs/basic.js
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,11 @@ module.exports = {
.assert.cssClassPresent('li:nth-child(8)', 'exact-active')
.assert.attributeEquals('li:nth-child(8) a', 'class', '')

// Listener cleanup
.assert.containsText('#popstate-count', '1 popstate listeners')
.click('#teardown-app')
.assert.containsText('#popstate-count', '0 popstate listeners')

.end()
}
}
6 changes: 6 additions & 0 deletions test/e2e/specs/hash-mode.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,12 @@ module.exports = {
.waitForElementVisible('#app', 1000)
.assert.containsText('.view', 'unicode: ñ')
.assert.containsText('#query-t', '%')

// Listener cleanup
.assert.containsText('#popstate-count', '1 popstate listeners')
.click('#teardown-app')
.assert.containsText('#popstate-count', '0 popstate listeners')

.end()
}
}