Skip to content

Provide a way to un-sync (#64) #66

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

Merged
merged 1 commit into from
Sep 1, 2017
Merged

Conversation

nkoterba
Copy link
Contributor

Currently, vuex-router-sync only publicly exposes a single method: ‘sync’. This method creates a store.watch as well as a router.beforeHook. No publicly available method existed to effectively ‘unsync’ vuex-router-sync and remove the watch and hook, which created ‘dangling’ refrences and a potential memory leak. Users of this library may need an ‘unsync’ function if they, for example, only use Vue.js in specific portions of their webapp (e.g. with React, Angular, etc.) and when users navigate away from that portion they want to remove/clean-up all Vue.js resources and components. Includes a unit test. Had to bump vue-router to version 2.5.0 which was the first version to allow de-registering hook functions.

Currently, vuex-router-sync only publicly exposes a single method: ‘sync’.  This method creates a store.watch as well as a router.beforeHook.  No publicly available method existed to effectively ‘unsync’ vuex-router-sync and remove the watch and hook, which created ‘dangling’ refrences and a potential memory leak.  Users of this library may need an ‘unsync’ function if they, for example, only use Vue.js in specific portions of their webapp (e.g. with React, Angular, etc.) and when users navigate away from that portion they want to remove/clean-up all Vue.js resources and components.  Includes a unit test.  Had to bump vue-router to version 2.5.0 which was the first version to allow de-registering hook functions.
package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "vuex-router-sync",
"version": "4.2.0",
"version": "4.3.0",
Copy link
Member

Choose a reason for hiding this comment

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

don't change the version, we will change this when releasing 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. I've reset my head prior to the version change.

Copy link
Member

@LinusBorg LinusBorg left a comment

Choose a reason for hiding this comment

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

LGTM

@LinusBorg
Copy link
Member

@posva: We can merge, agreed?

@posva
Copy link
Member

posva commented Aug 28, 2017

@LinusBorg 👍 ok for me! Shouldn't we wait for a little review from Evan?

@yyx990803 yyx990803 merged commit 693aa76 into vuejs:master Sep 1, 2017
@yyx990803
Copy link
Member

Thanks!

@nkoterba nkoterba deleted the feature/unsync branch September 6, 2017 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants