fix(engine): preserve order of event dispaching between CE and SR

packages/lwc-engine/src/framework/api.ts

@@ -1,12 +1,12 @@
 import assert from "./assert";
 import { freeze, isArray, isUndefined, isNull, isFunction, isObject, isString, ArrayPush, assign, create, forEach, StringSlice, StringCharCodeAt, isNumber, hasOwnProperty } from "./language";
-import { vmBeingRendered, invokeComponentCallback } from "./invoker";
+import { vmBeingRendered, invokeEventListener, EventListenerContext } from "./invoker";
 import { EmptyArray, SPACE_CHAR } from "./utils";
 import { renderVM, createVM, appendVM, removeVM, VM, getCustomElementVM } from "./vm";
 import { registerComponent } from "./def";
-import { ComponentConstructor, markComponentAsDirty, isValidEvent } from "./component";
+import { ComponentConstructor, markComponentAsDirty } from "./component";
 import { VNode, VNodeData, VNodes, VElement, VComment, VText, Hooks } from "../3rdparty/snabbdom/types";
-import { patchShadowDomEvent } from "./events";
+import { patchShadowDomEvent, isValidEventForCustomElement } from "./events";
 
 export interface RenderAPI {
     h(tagName: string, data: VNodeData, children: VNodes): VNode;
@@ -362,11 +362,10 @@ export function b(fn: EventListener): EventListener {
     }
     const vm: VM = vmBeingRendered;
     return function handler(event: Event) {
-        if (!isValidEvent(event)) {
-            return;
+        if (isValidEventForCustomElement(event)) {
+            patchShadowDomEvent(event);
+            invokeEventListener(vm, EventListenerContext.COMPONENT_LISTENER, fn, vm.component, event);
         }
-        patchShadowDomEvent(event);
-        invokeComponentCallback(vm, fn, [event]);
     };
 }
 

packages/lwc-engine/src/framework/component.ts

@@ -13,7 +13,6 @@ import { VNodes } from "../3rdparty/snabbdom/types";
 
 import { Template } from "./template";
 import { ShadowRoot } from "./root";
-import { getRootNode, isChildNode } from "./dom";
 export type ErrorCallback = (error: any, stack: string) => void;
 export interface Component {
     [ViewModelReflection]: VM;
@@ -92,15 +91,6 @@ export function clearReactiveListeners(vm: VM) {
     }
 }
 
-export function isValidEvent(event: Event): boolean {
-    // TODO: this is only needed if ShadowDOM is not used
-    if ((event as any).composed === true) {
-        return true;
-    }
-    // if the closest root contains the currentTarget, the event is valid
-    return isChildNode(getRootNode.call(event.target), event.currentTarget as Node);
-}
-
 export function renderComponent(vm: VM): VNodes {
     if (process.env.NODE_ENV !== 'production') {
         assert.vm(vm);

packages/lwc-engine/src/framework/def.ts

@@ -92,6 +92,7 @@ import {
     ComponentConstructor, ErrorCallback, Component
  } from './component';
 import { Template } from "./template";
+import { removeTemplateEventListener, addTemplateEventListener } from "./events";
 
 const CtorToDefMap: WeakMap<any, ComponentDef> = new WeakMap();
 
@@ -300,6 +301,16 @@ function removeAttributeNSPatched(this: VMElement, attrNameSpace: string, attrNa
     removeAttributeNS.apply(this, ArraySlice.call(arguments));
 }
 
+function addEventListenerPatched(this: EventTarget, type: string, listener: EventListener) {
+    const vm = getCustomElementVM(this as HTMLElement);
+    addTemplateEventListener(vm, type, listener);
+}
+
+function removeEventListenerPatched(this: EventTarget, type: string, listener: EventListener) {
+    const vm = getCustomElementVM(this as HTMLElement);
+    removeTemplateEventListener(vm, type, listener);
+}
+
 function assertPublicAttributeCollision(vm: VM, attrName: string) {
     if (process.env.NODE_ENV === 'production') {
         // this method should never leak to prod
@@ -388,7 +399,15 @@ function createCustomElementDescriptorMap(publicProps: PropsDef, publicMethodsCo
         querySelectorAll: {
             value: lightDomQuerySelectorAll,
             configurable: true,
-        }
+        },
+        addEventListener: {
+            value: addEventListenerPatched,
+            configurable: true, // TODO: issue #653: Remove configurable once locker-membrane is introduced
+        },
+        removeEventListener: {
+            value: removeEventListenerPatched,
+            configurable: true, // TODO: issue #653: Remove configurable once locker-membrane is introduced
+        },
     };
     // expose getters and setters for each public props on the Element
     for (const key in publicProps) {

packages/lwc-engine/src/framework/events.ts

@@ -92,7 +92,7 @@ function getWrappedRootListener(vm: VM, listener: EventListener): WrappedListene
                 // it is not composed and its is coming from from shadow
                 (composed === false && getRootNode.call(event.target) === currentTarget)) {
                     patchShadowDomEvent(event);
-                    invokeEventListener(vm, EventListenerContext.ROOT_LISTENER, listener, event);
+                    invokeEventListener(vm, EventListenerContext.ROOT_LISTENER, listener, undefined, event);
             }
         } as WrappedListener;
         wrappedListener!.placement = EventListenerContext.ROOT_LISTENER;
@@ -116,16 +116,9 @@ function getWrappedComponentsListener(vm: VM, listener: EventListener): WrappedL
     let wrappedListener = cmpEventListenerMap.get(listener);
     if (isUndefined(wrappedListener)) {
         wrappedListener = function(event: Event) {
-            const { composed, target, currentTarget } = event as any;
-            if (
-                // it is composed, and we should always get it
-                composed === true ||
-                // it is dispatched onto the custom element directly
-                target === currentTarget ||
-                // it is coming from an slotted element
-                isChildNode(getRootNode.call(target, event), currentTarget as Node)) {
-                    patchShadowDomEvent(event);
-                    invokeEventListener(vm, EventListenerContext.COMPONENT_LISTENER, listener, event);
+            if (isValidEventForCustomElement(event)) {
+                patchShadowDomEvent(event);
+                invokeEventListener(vm, EventListenerContext.COMPONENT_LISTENER, listener, undefined, event);
             }
         } as WrappedListener;
         wrappedListener!.placement = EventListenerContext.COMPONENT_LISTENER;
@@ -218,35 +211,65 @@ function detachDOMListener(vm: VM, type: string, wrappedListener: WrappedListene
     }
 }
 
-export function addCmpEventListener(vm: VM, type: string, listener: EventListener, options: any) {
+const NON_COMPOSED = { composed: false };
+export function isValidEventForCustomElement(event: Event): boolean {
+    const { target, currentTarget } = event;
+    const { composed } = event as any;
+    return (
+        // it is composed, and we should always get it, or
+        composed === true ||
+        // it is dispatched onto the custom element directly, or
+        target === currentTarget ||
+        // it is coming from an slotted element
+        isChildNode(getRootNode.call(target, NON_COMPOSED), currentTarget as Node)
+    );
+}
+
+export function addTemplateEventListener(vm: VM, type: string, listener: EventListener) {
     if (process.env.NODE_ENV !== 'production') {
         assert.vm(vm);
-        assert.invariant(!isRendering, `${vmBeingRendered}.render() method has side effects on the state of ${vm} by adding an event listener for "${type}".`);
-        assert.invariant(isFunction(listener), `Invalid second argument for this.template.addEventListener() in ${vm} for event "${type}". Expected an EventListener but received ${listener}.`);
     }
-    const wrappedListener = getWrappedComponentsListener(vm, listener);
-    attachDOMListener(vm, type, wrappedListener);
+    // not need to wrap this listener because it is already wrapped by api.b()
+    (listener as WrappedListener).placement = EventListenerContext.COMPONENT_LISTENER;
+    attachDOMListener(vm, type, (listener as WrappedListener));
+}
+
+export function removeTemplateEventListener(vm: VM, type: string, listener: EventListener) {
+    if (process.env.NODE_ENV !== 'production') {
+        assert.vm(vm);
+    }
+    detachDOMListener(vm, type, (listener as WrappedListener));
 }
 
-export function addRootEventListener(vm: VM, type: string, listener: EventListener, options: any) {
+export function addCmpEventListener(vm: VM, type: string, listener: EventListener) {
     if (process.env.NODE_ENV !== 'production') {
         assert.vm(vm);
         assert.invariant(!isRendering, `${vmBeingRendered}.render() method has side effects on the state of ${vm} by adding an event listener for "${type}".`);
         assert.invariant(isFunction(listener), `Invalid second argument for this.template.addEventListener() in ${vm} for event "${type}". Expected an EventListener but received ${listener}.`);
     }
-    const wrappedListener = getWrappedRootListener(vm, listener);
+    const wrappedListener = getWrappedComponentsListener(vm, listener);
     attachDOMListener(vm, type, wrappedListener);
 }
 
-export function removeCmpEventListener(vm: VM, type: string, listener: EventListener, options: any) {
+export function removeCmpEventListener(vm: VM, type: string, listener: EventListener) {
     if (process.env.NODE_ENV !== 'production') {
         assert.vm(vm);
     }
     const wrappedListener = getWrappedComponentsListener(vm, listener);
     detachDOMListener(vm, type, wrappedListener);
 }
 
-export function removeRootEventListener(vm: VM, type: string, listener: EventListener, options: any) {
+export function addRootEventListener(vm: VM, type: string, listener: EventListener) {
+    if (process.env.NODE_ENV !== 'production') {
+        assert.vm(vm);
+        assert.invariant(!isRendering, `${vmBeingRendered}.render() method has side effects on the state of ${vm} by adding an event listener for "${type}".`);
+        assert.invariant(isFunction(listener), `Invalid second argument for this.template.addEventListener() in ${vm} for event "${type}". Expected an EventListener but received ${listener}.`);
+    }
+    const wrappedListener = getWrappedRootListener(vm, listener);
+    attachDOMListener(vm, type, wrappedListener);
+}
+
+export function removeRootEventListener(vm: VM, type: string, listener: EventListener) {
     if (process.env.NODE_ENV !== 'production') {
         assert.vm(vm);
     }

packages/lwc-engine/src/framework/html-element.ts

@@ -1,7 +1,7 @@
 import assert from "./assert";
 import { ShadowRoot } from "./root";
 import { Component } from "./component";
-import { isObject, freeze, seal, defineProperty, defineProperties, getOwnPropertyNames, ArraySlice, isNull, forEach } from "./language";
+import { toString, isObject, freeze, seal, defineProperty, defineProperties, getOwnPropertyNames, ArraySlice, isNull, forEach } from "./language";
 import { addCmpEventListener, removeCmpEventListener } from "./events";
 import {
     getGlobalHTMLPropertiesInfo,
@@ -158,15 +158,23 @@ LWCElement.prototype = {
 
             if (arguments.length > 2) {
                 // TODO: can we synthetically implement `passive` and `once`? Capture is probably ok not supporting it.
-                assert.logWarning(`this.addEventListener() on ${vm} does not support more than 2 arguments. Options to make the listener passive, once or capture are not allowed at the top level of the component's fragment.`);
+                assert.logWarning(`this.addEventListener() on ${vm} does not support more than 2 arguments, instead received ${toString(options)}. Options to make the listener passive, once or capture are not allowed.`);
             }
         }
-        addCmpEventListener(vm, type, listener, options);
+        addCmpEventListener(vm, type, listener);
     },
 
     removeEventListener(type: string, listener: EventListener, options?: any) {
         const vm = getComponentVM(this);
-        removeCmpEventListener(vm, type, listener, options);
+        if (process.env.NODE_ENV !== 'production') {
+            assert.vm(vm);
+
+            if (arguments.length > 2) {
+                // TODO: can we synthetically implement `passive` and `once`? Capture is probably ok not supporting it.
+                assert.logWarning(`this.removeEventListener() on ${vm} does not support more than 2 arguments, instead received ${toString(options)}. Options to make the listener passive, once or capture are not allowed.`);
+            }
+        }
+        removeCmpEventListener(vm, type, listener);
     },
 
     setAttributeNS(ns: string, attrName: string, value: any): void {

packages/lwc-engine/src/framework/invoker.ts

@@ -6,7 +6,7 @@ import {
 import { evaluateTemplate } from "./template";
 import { isUndefined, isFunction } from "./language";
 import { getComponentStack, VM } from "./vm";
-import { ComponentConstructor } from "./component";
+import { ComponentConstructor, Component } from "./component";
 import { VNodes } from "../3rdparty/snabbdom/types";
 import { startMeasure, endMeasure } from "./performance-timing";
 
@@ -126,15 +126,15 @@ export enum EventListenerContext {
 
 export let componentEventListenerType: EventListenerContext | null = null;
 
-export function invokeEventListener(vm: VM, listenerContext: EventListenerContext, fn: EventListener, event: Event) {
+export function invokeEventListener(vm: VM, listenerContext: EventListenerContext, fn: EventListener, thisValue: undefined | Component, event: Event) {
     const { context, callHook } = vm;
     const ctx = currentContext;
     establishContext(context);
     let error;
     const componentEventListenerTypeInception = componentEventListenerType;
     componentEventListenerType = listenerContext;
     try {
-        callHook(undefined, fn, [event]);
+        callHook(thisValue, fn, [event]);
     } catch (e) {
         error = Object(e);
     } finally {

packages/lwc-engine/src/framework/modules/events.ts

@@ -1,6 +1,5 @@
 import { isUndefined } from "../language";
 import { VNode, Module } from "../../3rdparty/snabbdom/types";
-import { addEventListener, removeEventListener } from "../dom";
 
 function handleEvent(event: Event, vnode: VNode) {
     const { type } = event;
@@ -32,7 +31,8 @@ function removeAllEventListeners(vnode: InteractiveVNode) {
         const elm = vnode.elm as Element;
         let name;
         for (name in on) {
-            removeEventListener.call(elm, name, listener, false);
+            // intentionally using removeEventListener from elm to allow patching
+            elm.removeEventListener(name, listener);
         }
         vnode.listener = undefined;
     }
@@ -58,7 +58,8 @@ function createAllEventListeners(oldVnode: InteractiveVNode, vnode: InteractiveV
 
     let name;
     for (name in on) {
-        addEventListener.call(elm, name, listener, false);
+        // intentionally using addEventListener from elm to allow patching
+        elm.addEventListener(name, listener);
     }
 }
 

packages/lwc-engine/src/framework/root.ts

@@ -1,6 +1,6 @@
 import assert from "./assert";
 import { ViewModelReflection } from "./def";
-import { isUndefined, defineProperty, isNull, defineProperties, create, getOwnPropertyNames, forEach, hasOwnProperty } from "./language";
+import { isUndefined, defineProperty, isNull, defineProperties, create, getOwnPropertyNames, forEach, hasOwnProperty, toString } from "./language";
 import { getCustomElementComponent } from "./component";
 import { getShadowRootVM, VM } from "./vm";
 import { Component } from "./component";
@@ -107,12 +107,28 @@ export class Root implements ShadowRoot {
 
     addEventListener(type: string, listener: EventListener, options?: any) {
         const vm = getShadowRootVM(this);
-        addRootEventListener(vm, type, listener, options);
+        if (process.env.NODE_ENV !== 'production') {
+            assert.vm(vm);
+
+            if (arguments.length > 2) {
+                // TODO: can we synthetically implement `passive` and `once`? Capture is probably ok not supporting it.
+                assert.logWarning(`this.template.addEventListener() on ${vm} does not support more than 2 arguments, instead received ${toString(options)}. Options to make the listener passive, once or capture are not allowed.`);
+            }
+        }
+        addRootEventListener(vm, type, listener);
     }
 
     removeEventListener(type: string, listener: EventListener, options?: any) {
         const vm = getShadowRootVM(this);
-        removeRootEventListener(vm, type, listener, options);
+        if (process.env.NODE_ENV !== 'production') {
+            assert.vm(vm);
+
+            if (arguments.length > 2) {
+                // TODO: can we synthetically implement `passive` and `once`? Capture is probably ok not supporting it.
+                assert.logWarning(`this.template.removeEventListener() on ${vm} does not support more than 2 arguments, instead received ${toString(options)}. Options to make the listener passive, once or capture are not allowed.`);
+            }
+        }
+        removeRootEventListener(vm, type, listener);
     }
     toString(): string {
         const component = getCustomElementComponent(this);