feat(history): Remove event listeners when all apps are destroyed. #3172
Conversation
src/index.js
Outdated
| } | ||
| history.transitionTo(history.getCurrentLocation(), setupListeners, setupListeners) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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'))
There was a problem hiding this comment.
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.
|
This is ready for another review. |
|
Sorry for the delay on this. I will give it a deeper look and hopefully get it merged soon |
There was a problem hiding this comment.
This is a great PR! Thanks a lot for the thought and work put on this.
I think we could create a new e2e test specifically for multi apps and add a few more cases. Similar to what I have for vue-router-next (https://github.com/vuejs/vue-router-next/blob/master/e2e/multi-app/index.ts) where we can mount and unmount apps on demand. The code is a bit different because it's for Vue 3, but let me know if you need help for it. I can also be available on Discord to chat about it
src/index.js
Outdated
| @@ -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) { | |||
There was a problem hiding this comment.
we should be able to just check !this.app
|
Thanks for the review - I have updated this with your feedback. |
src/history/html5.js
Outdated
| const expectScroll = router.options.scrollBehavior | ||
| const supportsScroll = supportsPushState && expectScroll |
There was a problem hiding this comment.
can't we remove these lines since they are right above? They shouldn't change
examples/multi-app/app.js
Outdated
|
|
||
| methods: { | ||
| teardown () { | ||
| this.$destroy() |
There was a problem hiding this comment.
It's better to keep this code outside of the instance being destroyed:
- adding unmount buttons next to the mounting ones
- keep an array of the instances created by
new Vue - remove the entry from the array when destroying
|
I updated this with your feedback. |
|
when function setupListeners execute after "hook:destroyed" event, this cannot work correctly! |
This resolves #3152 and resolves #2341
This is my first PR to vue-router and I am not sure if this was the best approach to solving the problem - please let me know! I also need to think of a good way of testing this behavior in either the e2e or unit tests. The unit tests only use the the abstract mode, but the feature only applies to html5/hash modes. And for e2e tests, I need to brainstorm the best way to verify that the hashchange and popstate listeners are being removed.