fix(engine): fixes #1199 and #1198 - disconnecting bug (#1202) (#1209)

* fix(engine): issues #1199 and #1198 when disconnecting

* fix(engine): PR 1202 feedback

* fix(engine): undefined elm needs protection

* fix(engine): adding tests for PR 1202

* fix(engine): test fix for native shadow

packages/@lwc/engine/src/framework/hooks.ts

@@ -17,7 +17,7 @@ import {
     appendVM,
     runWithBoundaryProtection,
 } from './vm';
-import { VNode, VNodes, VCustomElement, VElement } from '../3rdparty/snabbdom/types';
+import { VNode, VCustomElement, VElement } from '../3rdparty/snabbdom/types';
 import modEvents from './modules/events';
 import modAttrs from './modules/attrs';
 import modProps from './modules/props';
@@ -177,11 +177,12 @@ export function updateChildrenHook(oldVnode: VElement, vnode: VElement) {
 }
 
 export function allocateChildrenHook(vnode: VCustomElement) {
+    const elm = vnode.elm as HTMLElement;
+    const vm = getCustomElementVM(elm);
+    const { children } = vnode;
+    vm.aChildren = children;
     if (isTrue(vnode.owner.fallback)) {
         // slow path
-        const elm = vnode.elm as HTMLElement;
-        const vm = getCustomElementVM(elm);
-        const children = vnode.children as VNodes;
         allocateInSlot(vm, children);
         // every child vnode is now allocated, and the host should receive none directly, it receives them via the shadow!
         vnode.children = EmptyArray;

packages/@lwc/engine/src/framework/vm.ts

@@ -28,14 +28,15 @@ import {
     defineProperty,
     StringToLowerCase,
     isFalse,
+    isArray,
 } from '../shared/language';
 import { getInternalField, getHiddenField } from '../shared/fields';
 import { ViewModelReflection, addCallbackToNextTick, EmptyObject, EmptyArray } from './utils';
 import { invokeServiceHook, Services } from './services';
 import { invokeComponentCallback } from './invoker';
 import { ShadowRootInnerHTMLSetter, ShadowRootHostGetter } from '../env/dom';
 
-import { VNodeData, VNodes, VCustomElement } from '../3rdparty/snabbdom/types';
+import { VNodeData, VNodes, VCustomElement, VNode } from '../3rdparty/snabbdom/types';
 import { Template } from './template';
 import { ComponentDef } from './def';
 import { ComponentInterface } from './component';
@@ -81,7 +82,10 @@ export interface UninitializedVM {
     /** Component state, analogous to Element.isConnected */
     state: VMState;
     data: VNodeData;
+    /** Shadow Children List */
     children: VNodes;
+    /** Adopted Children List */
+    aChildren: VNodes;
     velements: VCustomElement[];
     cmpTemplate?: Template;
     cmpProps: any;
@@ -164,8 +168,13 @@ function resetComponentStateWhenRemoved(vm: VM) {
     if (process.env.NODE_ENV !== 'production') {
         assert.isTrue(vm && 'cmpRoot' in vm, `${vm} is not a vm.`);
     }
-    runDisconnectedCallback(vm);
-    runChildrenDisconnectedCallback(vm);
+    const { state } = vm;
+    if (state !== VMState.disconnected) {
+        runDisconnectedCallback(vm);
+        // Spec: https://dom.spec.whatwg.org/#concept-node-remove (step 14-15)
+        runShadowChildNodesDisconnectedCallback(vm);
+        runLightChildNodesDisconnectedCallback(vm);
+    }
 }
 
 // this method is triggered by the diffing algo only when a vnode from the
@@ -235,6 +244,7 @@ export function createVM(elm: HTMLElement, Ctor: ComponentConstructor, options:
         getHook,
         component: undefined,
         children: EmptyArray,
+        aChildren: EmptyArray,
         velements: EmptyArray,
         // used to track down all object-key pairs that makes this vm reactive
         deps: [],
@@ -400,10 +410,7 @@ function runConnectedCallback(vm: VM) {
 function runDisconnectedCallback(vm: VM) {
     if (process.env.NODE_ENV !== 'production') {
         assert.isTrue(vm && 'cmpRoot' in vm, `${vm} is not a vm.`);
-    }
-    const { state } = vm;
-    if (state === VMState.disconnected) {
-        return; // nothing to do since it was already disconnected
+        assert.isTrue(vm.state !== VMState.disconnected, `${vm} must be inserted.`);
     }
     if (isFalse(vm.isDirty)) {
         // this guarantees that if the component is reused/reinserted,
@@ -433,13 +440,13 @@ function runDisconnectedCallback(vm: VM) {
     }
 }
 
-function runChildrenDisconnectedCallback(vm: VM) {
+function runShadowChildNodesDisconnectedCallback(vm: VM) {
     if (process.env.NODE_ENV !== 'production') {
         assert.isTrue(vm && 'cmpRoot' in vm, `${vm} is not a vm.`);
     }
     const { velements: vCustomElementCollection } = vm;
-    // reporting disconnection for every child
-    for (let i = 0, len = vCustomElementCollection.length; i < len; i += 1) {
+    // reporting disconnection for every child in inverse order since they are inserted in reserved order
+    for (let i = vCustomElementCollection.length - 1; i >= 0; i -= 1) {
         const elm = vCustomElementCollection[i].elm;
         // There are two cases where the element could be undefined:
         // * when there is an error during the construction phase, and an
@@ -449,8 +456,38 @@ function runChildrenDisconnectedCallback(vm: VM) {
         //   into it, as a result, the custom element was never initialized.
         if (!isUndefined(elm)) {
             const childVM = getCustomElementVM(elm as HTMLElement);
-            runDisconnectedCallback(childVM);
-            runChildrenDisconnectedCallback(childVM);
+            resetComponentStateWhenRemoved(childVM);
+        }
+    }
+}
+
+function runLightChildNodesDisconnectedCallback(vm: VM) {
+    if (process.env.NODE_ENV !== 'production') {
+        assert.isTrue(vm && 'cmpRoot' in vm, `${vm} is not a vm.`);
+    }
+    const { aChildren: adoptedChildren } = vm;
+    recursivelyDisconnectChildren(adoptedChildren);
+}
+
+/**
+ * The recursion doesn't need to be a complete traversal of the vnode graph,
+ * instead it can be partial, when a custom element vnode is found, we don't
+ * need to continue into its children because by attempting to disconnect the
+ * custom element itself will trigger the removal of anything slotted or anything
+ * defined on its shadow.
+ */
+function recursivelyDisconnectChildren(vnodes: VNodes) {
+    for (let i = 0, len = vnodes.length; i < len; i += 1) {
+        const vnode: VCustomElement | VNode | null = vnodes[i];
+        if (!isNull(vnode) && isArray(vnode.children) && !isUndefined(vnode.elm)) {
+            // vnode is a VElement with children
+            if (isUndefined((vnode as any).ctor)) {
+                // it is a VElement, just keep looking (recursively)
+                recursivelyDisconnectChildren(vnode.children);
+            } else {
+                // it is a VCustomElement, disconnect it and ignore its children
+                resetComponentStateWhenRemoved(getCustomElementVM(vnode.elm as HTMLElement));
+            }
         }
     }
 }
@@ -473,7 +510,7 @@ export function resetShadowRoot(vm: VM) {
         ShadowRootInnerHTMLSetter.call(vm.cmpRoot, '');
     }
     // disconnecting any known custom element inside the shadow of the this vm
-    runChildrenDisconnectedCallback(vm);
+    runShadowChildNodesDisconnectedCallback(vm);
 }
 
 export function scheduleRehydration(vm: VM) {

packages/integration-karma/test/component/disconnect-callback/index.spec.js

@@ -0,0 +1,36 @@
+import { createElement } from 'test-utils';
+
+import Container from 'x/container';
+
+function resetTimingBuffer() {
+    window.timingBuffer = [];
+}
+
+beforeEach(() => {
+    resetTimingBuffer();
+});
+
+afterEach(() => {
+    delete window.timingBuffer;
+});
+
+it('should disconnect on the right order (issue #1199 and #1198)', () => {
+    const elm = createElement('x-container', { is: Container });
+    document.body.appendChild(elm);
+    expect(window.timingBuffer.length).toEqual(15);
+
+    resetTimingBuffer();
+    elm.hide = true;
+
+    return Promise.resolve().then(() => {
+        expect(window.timingBuffer).toEqual([
+            'parent:disconnectedCallback',
+            'ownChild:disconnectedCallback',
+            'grandChild:disconnectedCallback',
+            'adoptedChild:disconnectedCallback',
+            'grandChild:disconnectedCallback',
+        ]);
+
+        resetTimingBuffer();
+    });
+});

packages/integration-karma/test/component/disconnect-callback/x/adoptedChild/adoptedChild.html

@@ -0,0 +1,3 @@
+<template>
+    <x-grand-child></x-grand-child>
+</template>

packages/integration-karma/test/component/disconnect-callback/x/adoptedChild/adoptedChild.js

@@ -0,0 +1,20 @@
+import { LightningElement } from 'lwc';
+
+export default class AdoptedChild extends LightningElement {
+    constructor() {
+        super();
+        window.timingBuffer.push('adoptedChild:constructor');
+    }
+
+    connectedCallback() {
+        window.timingBuffer.push('adoptedChild:connectedCallback');
+    }
+
+    disconnectedCallback() {
+        window.timingBuffer.push('adoptedChild:disconnectedCallback');
+    }
+
+    renderedCallback() {
+        window.timingBuffer.push('adoptedChild:renderedCallback');
+    }
+}

packages/integration-karma/test/component/disconnect-callback/x/container/container.html

@@ -0,0 +1,7 @@
+<template>
+    <template if:false={hide}>
+        <x-parent>
+            <x-adopted-child></x-adopted-child>
+        </x-parent>
+    </template>
+</template>

packages/integration-karma/test/component/disconnect-callback/x/container/container.js

@@ -0,0 +1,5 @@
+import { LightningElement, api } from 'lwc';
+
+export default class Container extends LightningElement {
+    @api hide = false;
+}

packages/integration-karma/test/component/disconnect-callback/x/grandChild/grandChild.html

@@ -0,0 +1,2 @@
+<template></template>
+

packages/integration-karma/test/component/disconnect-callback/x/grandChild/grandChild.js

@@ -0,0 +1,20 @@
+import { LightningElement } from 'lwc';
+
+export default class GrandChild extends LightningElement {
+    constructor() {
+        super();
+        window.timingBuffer.push('grandChild:constructor');
+    }
+
+    connectedCallback() {
+        window.timingBuffer.push('grandChild:connectedCallback');
+    }
+
+    disconnectedCallback() {
+        window.timingBuffer.push('grandChild:disconnectedCallback');
+    }
+
+    renderedCallback() {
+        window.timingBuffer.push('grandChild:renderedCallback');
+    }
+}

packages/integration-karma/test/component/disconnect-callback/x/ownChild/ownChild.html

@@ -0,0 +1,3 @@
+<template>
+    <x-grand-child></x-grand-child>
+</template>

packages/integration-karma/test/component/disconnect-callback/x/ownChild/ownChild.js

@@ -0,0 +1,20 @@
+import { LightningElement } from 'lwc';
+
+export default class OwnChild extends LightningElement {
+    constructor() {
+        super();
+        window.timingBuffer.push('ownChild:constructor');
+    }
+
+    connectedCallback() {
+        window.timingBuffer.push('ownChild:connectedCallback');
+    }
+
+    disconnectedCallback() {
+        window.timingBuffer.push('ownChild:disconnectedCallback');
+    }
+
+    renderedCallback() {
+        window.timingBuffer.push('ownChild:renderedCallback');
+    }
+}

packages/integration-karma/test/component/disconnect-callback/x/parent/parent.html

@@ -0,0 +1,5 @@
+<template>
+    <x-own-child></x-own-child>
+    <slot></slot>
+</template>
+

packages/integration-karma/test/component/disconnect-callback/x/parent/parent.js

@@ -0,0 +1,20 @@
+import { LightningElement } from 'lwc';
+
+export default class Parent extends LightningElement {
+    constructor() {
+        super();
+        window.timingBuffer.push('parent:constructor');
+    }
+
+    connectedCallback() {
+        window.timingBuffer.push('parent:connectedCallback');
+    }
+
+    disconnectedCallback() {
+        window.timingBuffer.push('parent:disconnectedCallback');
+    }
+
+    renderedCallback() {
+        window.timingBuffer.push('parent:renderedCallback');
+    }
+}