fix(watch): component should runCleanup() on unmount (#298)

* fix(watch): component should runCleaup() on unmount

close #293

* fix: dont push to vm._watchers manually

Author: LinusBorg

src/apis/watch.ts

@@ -139,6 +139,16 @@ function createVueWatcher(
   return vm._watchers[index];
 }
 
+// We have to monkeypatch the teardown function so Vue will run
+// runCleanup() when it tears down the watcher on unmmount.
+function patchWatcherTeardown(watcher: VueWatcher, runCleanup: () => void) {
+  const _teardown = watcher.teardown;
+  watcher.teardown = function(...args) {
+    _teardown.apply(watcher, args);
+    runCleanup();
+  };
+}
+
 function createWatcher(
   vm: ComponentInstance,
   source: WatcherSource<unknown> | WatcherSource<unknown>[] | SimpleEffect,
@@ -188,6 +198,8 @@ function createWatcher(
       before: runCleanup,
     });
 
+    patchWatcherTeardown(watcher, runCleanup);
+
     // enable the watcher update
     watcher.lazy = false;
 
@@ -201,7 +213,6 @@ function createWatcher(
 
     return () => {
       watcher.teardown();
-      runCleanup();
     };
   }
 
@@ -240,9 +251,12 @@ function createWatcher(
     sync: isSync,
   });
 
+  // Once again, we have to hack the watcher for proper teardown
+  const watcher = vm._watchers[vm._watchers.length - 1];
+  patchWatcherTeardown(watcher, runCleanup);
+
   return () => {
     stop();
-    runCleanup();
   };
 }
 

test/apis/watch.spec.js

@@ -13,10 +13,14 @@ describe('api/watch', () => {
   });
 
   it('should work', done => {
+    const onCleanupSpy = jest.fn();
     const vm = new Vue({
       setup() {
         const a = ref(1);
-        watch(a, spy);
+        watch(a, (n, o, _onCleanup) => {
+          spy(n, o, _onCleanup);
+          _onCleanup(onCleanupSpy);
+        });
         return {
           a,
         };
@@ -25,13 +29,21 @@ describe('api/watch', () => {
     }).$mount();
     expect(spy).toBeCalledTimes(1);
     expect(spy).toHaveBeenLastCalledWith(1, undefined, anyFn);
+    expect(onCleanupSpy).toHaveBeenCalledTimes(0);
     vm.a = 2;
     vm.a = 3;
     expect(spy).toBeCalledTimes(1);
     waitForUpdate(() => {
       expect(spy).toBeCalledTimes(2);
       expect(spy).toHaveBeenLastCalledWith(3, 1, anyFn);
-    }).then(done);
+      expect(onCleanupSpy).toHaveBeenCalledTimes(1);
+
+      vm.$destroy();
+    })
+      .then(() => {
+        expect(onCleanupSpy).toHaveBeenCalledTimes(2);
+      })
+      .then(done);
   });
 
   it('basic usage(value wrapper)', done => {
@@ -349,11 +361,13 @@ describe('api/watch', () => {
     let renderedText;
     it('should work', done => {
       let onCleanup;
+      const onCleanupSpy = jest.fn();
       const vm = new Vue({
         setup() {
           const count = ref(0);
           watchEffect(_onCleanup => {
             onCleanup = _onCleanup;
+            _onCleanup(onCleanupSpy);
             spy(count.value);
             renderedText = vm.$el.textContent;
           });
@@ -369,13 +383,19 @@ describe('api/watch', () => {
       expect(spy).not.toHaveBeenCalled();
       waitForUpdate(() => {
         expect(onCleanup).toEqual(anyFn);
+        expect(onCleanupSpy).toHaveBeenCalledTimes(0);
         expect(renderedText).toBe('0');
         expect(spy).toHaveBeenLastCalledWith(0);
         vm.count++;
       })
         .then(() => {
           expect(renderedText).toBe('1');
           expect(spy).toHaveBeenLastCalledWith(1);
+          expect(onCleanupSpy).toHaveBeenCalledTimes(1);
+          vm.$destroy();
+        })
+        .then(() => {
+          expect(onCleanupSpy).toHaveBeenCalledTimes(2);
         })
         .then(done);
     });