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 1 commit
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
// This should be replaced with something better
let numPopstateListeners = 0
const listenerCountDiv = document.createElement('div')
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 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
// This should be replaced with something better
let numPopstateListeners = 0
const listenerCountDiv = document.createElement('div')
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 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 () {
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)
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
20 changes: 9 additions & 11 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,18 +116,10 @@ export default class VueRouter {

const history = this.history

if (history instanceof HTML5History) {
history.transitionTo(history.getCurrentLocation())
} else if (history instanceof HashHistory) {
const setupHashListener = () => {
history.setupListeners()
}
history.transitionTo(
history.getCurrentLocation(),
setupHashListener,
setupHashListener
)
const setupListeners = () => {
history.setupListeners()
}
history.transitionTo(history.getCurrentLocation(), setupListeners, setupListeners)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's some new behavior here - previously transitionTo() would only get called during app initialization if using html5 or hash history, but now it is being called for abstract mode, too. I did so not because abstract mode needs this, but rather because it simplified the if/else code that existed before and appeared to have no harmful side effects.

Copy link
Member

Choose a reason for hiding this comment

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

For this part, it's better to keep the previous behavior because abstract mode cannot always directly transition: e.g. during SSR whereas History and Hash can retrieve the current location on browsers

Copy link
Member

Choose a reason for hiding this comment

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

I also realized we also need to keep the previous behavior for hash history for the hash event not to fire too early on some browsers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's better to keep the previous behavior because abstract mode cannot always directly transition:

👍 updated

I also realized we also need to keep the previous behavior for hash history for the hash event not to fire too early on some browsers

That behavior still exists in this PR, as far as I can tell. HashHistory does not set up listeners until setupListeners() is called, and setupListeners() is not called until the first history.transitionTo() finishes up (as shown in the line of code above)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thing that's different here is that now that behavior for HashHistory is also applying to Html5History - beforehand the popstate listener was set up before the first transition, but it now is set up only after the first transition.

I think that new behavior is completely fine. My guess as to why HashHistory needed that special behavior is that hashchange events are fired as a result of programmatic navigation (via location.hash = '#/new'), whereas the browser never fires popstate events never for programmatic navigation (via history.pushState(state, title, '/new'))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops I made the changes related to Abstract History weeks ago but forgot to push!! I commented and said "Updated" even though I didn't push. I have now pushed my changes.


history.listen(route => {
this.apps.forEach((app) => {
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