Fixing Vue.$destroy(remove) does not trigger "detached" hook on second level children

Please see issue for description about the problem and what this change is supposed to fix - #1966
I know that it doesn't look good to modify tests when they fail because of a change, but I think that the lifecycle hook order should be different than what the existing test validates. The problem is again described in the issue summary

Author: tgeorgiev

src/instance/internal/lifecycle.js

@@ -150,6 +150,30 @@ export default function (Vue) {
       }
       return
     }
+
+    var destroyReady
+    var pendingRemoval
+
+    var self = this
+    // Cleanup should be called either synchronously or asynchronoysly as
+    // callback of this.$remove(), or if remove and deferCleanup are false.
+    // In any case it should be called after all other removing, unbinding and
+    // turning of is done
+    var cleanupIfPossible = function () {
+      if (destroyReady && !pendingRemoval && !deferCleanup) {
+        self._cleanup()
+      }
+    }
+
+    // remove DOM element
+    if (remove && this.$el) {
+      pendingRemoval = true
+      this.$remove(function () {
+        pendingRemoval = false
+        cleanupIfPossible()
+      })
+    }
+
     this._callHook('beforeDestroy')
     this._isBeingDestroyed = true
     var i
@@ -183,15 +207,9 @@ export default function (Vue) {
     if (this.$el) {
       this.$el.__vue__ = null
     }
-    // remove DOM element
-    var self = this
-    if (remove && this.$el) {
-      this.$remove(function () {
-        self._cleanup()
-      })
-    } else if (!deferCleanup) {
-      this._cleanup()
-    }
+
+    destroyReady = true
+    cleanupIfPossible()
   }
 
   /**

test/unit/specs/api/lifecycle_spec.js

@@ -192,6 +192,37 @@ describe('Lifecycle API', function () {
       expect(opts.detached).toHaveBeenCalled()
     })
 
+    // #1966
+    it('grandchild hooks', function () {
+      var grandChildBeforeDestroy = jasmine.createSpy()
+      var grandChildDestroyed = jasmine.createSpy()
+      var grandChildDetached = jasmine.createSpy()
+
+      var opts = {
+        template: '<div><test></test></div>',
+        components: {
+          test: {
+            template: '<div><test-inner></test-inner></div>',
+            components: {
+              'test-inner': {
+                beforeDestroy: grandChildBeforeDestroy,
+                destroyed: grandChildDestroyed,
+                detached: grandChildDetached
+              }
+            }
+          }
+        }
+      }
+      var el = opts.el = document.createElement('div')
+      document.body.appendChild(el)
+      var vm = new Vue(opts)
+      vm.$destroy(true)
+
+      expect(grandChildBeforeDestroy).toHaveBeenCalled()
+      expect(grandChildDestroyed).toHaveBeenCalled()
+      expect(grandChildDetached).toHaveBeenCalled()
+    })
+
     it('parent', function () {
       var parent = new Vue()
       var child = new Vue({ parent: parent })

test/unit/specs/misc_spec.js

@@ -119,7 +119,56 @@ describe('Misc', function () {
     expect(logs.join()).toBe('0,5,6,5,6,1')
     logs = []
     vm.$destroy(true)
-    expect(logs.join()).toBe('3,8,9,8,9,2,7,7,4')
+    expect(logs.join()).toBe('2,7,7,3,8,9,8,9,4')
+    Vue.options.replace = false
+  })
+
+  // #1966
+  it('call lifecycle hooks for child and grandchild components', function () {
+    Vue.options.replace = true
+    var el = document.createElement('div')
+    var logs = []
+    function log (n) {
+      return function () {
+        logs.push(n)
+      }
+    }
+    document.body.appendChild(el)
+    var vm = new Vue({
+      el: el,
+      attached: log(0),
+      ready: log(1),
+      detached: log(2),
+      beforeDestroy: log(3),
+      destroyed: log(4),
+      template: '<div><test></test></div>',
+      components: {
+        test: {
+          attached: log(5),
+          ready: log(6),
+          detached: log(7),
+          beforeDestroy: log(8),
+          destroyed: log(9),
+          template: '<div><test-inner></test-inner></div>',
+          components: {
+            'test-inner': {
+              attached: log(10),
+              ready: log(11),
+              detached: log(12),
+              beforeDestroy: log(13),
+              destroyed: log(14),
+              template: '<span>hi</span>'
+            }
+          }
+
+        }
+      }
+    })
+    expect(vm.$el.innerHTML).toBe('<div><span>hi</span></div>')
+    expect(logs.join()).toBe('0,5,10,11,6,1')
+    logs = []
+    vm.$destroy(true)
+    expect(logs.join()).toBe('2,7,12,3,8,13,14,9,4')
     Vue.options.replace = false
   })