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

Ability to detach and reattach popstate listeners #3152

Closed
joeldenning opened this issue Mar 22, 2020 · 4 comments · Fixed by #3172
Closed

Ability to detach and reattach popstate listeners #3152

joeldenning opened this issue Mar 22, 2020 · 4 comments · Fixed by #3172
Labels
feature request micro frontends Features to handle micro frontends

Comments

@joeldenning
Copy link
Contributor

joeldenning commented Mar 22, 2020

Version

3.1.6

Reproduction link

https://codesandbox.io/s/nostalgic-mahavira-y0m7q

Steps to reproduce

Memory leak

Call new VueRouter() multiple times. It is impossible to clean up its popstate listener.

Basepath bug

If you have multiple vue applications, each using vue-router, they must all share the same base path, even if you ensure only one of them is active at a time by destroying / remounting when navigating between them. Otherwise they will all attempt to ensure that every route starts with their base path.

Same goes for if you have one vue-router and one react-router. The vue-router will force all urls to start with its base path within the react-owned pages, even if you have destroyed the vue application before navigating to the react application.

What is expected?

I think the best solution is to call window.addEventListener('popstate', handler) when const intance = new Vue({router}) is called, instead of when new VueRouter() is called. And then call window.removeEventListener('popstate', handler) when instance.$destroy() is called.

Alternatively, it could be cleaned up like so:

// Alternative solution (not my preferred one, but works)

const router = new VueRouter({...})
// later, to clean up event listeners:
router.detach()
// later, to reattach event listeners
router.attach()

What is actually happening?

window.addEventListener is called during new VueRouter(), and there is no way to ever remove that event listener.


See related single-spa/single-spa#488. I am a maintainer of single-spa, which has dozens of organizations using one or more instances of vue-router. For us, cleaning up the event listener when the application is unmounted is important to being able to have isolated apps on the same page.

@posva posva changed the title [Memory leak and bug] popstate listeners are never cleaned up Ability to detach and reattach popstate listeners Mar 22, 2020
@posva
Copy link
Member

posva commented Mar 22, 2020

This is related to #3152

I'm not sure something as simple as attaching and detaching the router will work in all cases. Maybe removing the popstate listener if all instances are destroyed and attaching it again if no popstate listener is attached when the router is injected in an app would work. Like said at #2341 (comment)

@joeldenning
Copy link
Contributor Author

Thanks for the quick response. I agree that removing the listener once all Vue applications are destroyed would be a good approach. I’ll take a deeper look at the code to see if I can put together a PR that fixes this.

@joeldenning
Copy link
Contributor Author

I have put together a fix for this at #3172

@joeldenning
Copy link
Contributor Author

Another bug that I've seen related to this is that if you call new Vue({router}) during a PopStateEvent handler, the beforeEnter hook for routes will fire _twice_ instead of once. This would not happen if the popstate listener for vue-router was cleaned up when the vue instance is destroyed.

posva pushed a commit that referenced this issue May 19, 2020
…3172)

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

* fix(tests): Adding test assertions

* fix(feedback): addressing @posva's feedback

* fix(index): posva's feedback

* feat(tests): adding multi-app test

* fix(feedback): posva's feedback

* fix(feedback): unmounting apps with buttons outside of the app

Close #3152
Close #2341
@posva posva added the micro frontends Features to handle micro frontends label Aug 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request micro frontends Features to handle micro frontends
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants