fix(engine): Make engine more resilient to invalid constructors (#1121)

* wip: intial commit fix constructor

* chore: reenable tsc in package.json

* chore: remove unnecessary null type

* chore: gate createElement with circular deps resolution

* fix: circular dependency in prototype lookup

* fix: improve error message

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

@@ -18,15 +18,14 @@ import {
     assign,
     freeze,
     create,
-    ArrayIndexOf,
-    ArrayPush,
     getOwnPropertyNames,
     getPrototypeOf,
     isNull,
     setPrototypeOf,
     ArrayReduce,
     isUndefined,
     getOwnPropertyDescriptor,
+    isFunction,
 } from '../shared/language';
 import { getInternalField } from '../shared/fields';
 import { getAttrNameFromPropName } from './attributes';
@@ -84,31 +83,12 @@ function getCtorProto(Ctor: any, subclassComponentName: string): ComponentConstr
     return proto as ComponentConstructor;
 }
 
-function isElementComponent(Ctor: any, subclassComponentName, protoSet?: any[]): boolean {
-    protoSet = protoSet || [];
-    if (!Ctor || ArrayIndexOf.call(protoSet, Ctor) >= 0) {
-        return false; // null, undefined, or circular prototype definition
-    }
-    const proto = getCtorProto(Ctor, subclassComponentName);
-    if ((proto as any) === BaseLightningElement) {
-        return true;
-    }
-    getComponentDef(proto, subclassComponentName); // ensuring that the prototype chain is already expanded
-    ArrayPush.call(protoSet, Ctor);
-    return isElementComponent(proto, subclassComponentName, protoSet);
-}
-
 function createComponentDef(
     Ctor: ComponentConstructor,
     meta: ComponentMeta,
     subclassComponentName: string
 ): ComponentDef {
     if (process.env.NODE_ENV !== 'production') {
-        assert.isTrue(
-            isElementComponent(Ctor, subclassComponentName),
-            `${Ctor} is not a valid component, or does not extends LightningElement from "lwc". You probably forgot to add the extend clause on the class declaration.`
-        );
-
         // local to dev block
         const ctorName = Ctor.name;
         // Removing the following assert until https://bugs.webkit.org/show_bug.cgi?id=190140 is fixed.
@@ -191,35 +171,72 @@ function createComponentDef(
     return def;
 }
 
-export function isComponentConstructor(Ctor: any): boolean {
-    return isElementComponent(Ctor, Ctor.name);
+export function isComponentConstructor(ctor: any): ctor is ComponentConstructor {
+    if (!isFunction(ctor)) {
+        return false;
+    }
+
+    // Fast path: LightningElement is part of the prototype chain of the constructor.
+    if (ctor.prototype instanceof BaseLightningElement) {
+        return true;
+    }
+
+    // Slow path: LightningElement is not part of the prototype chain of the constructor, we need
+    // climb up the constructor prototype chain to check in case there are circular dependencies
+    // to resolve.
+    let current = ctor;
+    do {
+        if (isCircularModuleDependency(current)) {
+            const circularResolved = resolveCircularModuleDependency(current);
+
+            // If the circular function returns itself, that's the signal that we have hit the end of the proto chain,
+            // which must always be a valid base constructor.
+            if (circularResolved === current) {
+                return true;
+            }
+
+            current = circularResolved;
+        }
+
+        if (current === BaseLightningElement) {
+            return true;
+        }
+    } while (!isNull(current) && (current = getPrototypeOf(current)));
+
+    // Finally return false if the LightningElement is not part of the prototype chain.
+    return false;
 }
 
 function getOwnValue(o: any, key: string): any | undefined {
     const d = getOwnPropertyDescriptor(o, key);
     return d && d.value;
 }
 
-export function getComponentDef(
-    Ctor: ComponentConstructor,
-    subclassComponentName?: string
-): ComponentDef {
+export function getComponentDef(Ctor: any, subclassComponentName?: string): ComponentDef {
     let def = CtorToDefMap.get(Ctor);
-    if (def) {
-        return def;
-    }
-    let meta = getComponentRegisteredMeta(Ctor);
-    if (isUndefined(meta)) {
-        // TODO: remove this workaround:
-        // this is temporary until
-        // all tests are updated to call registerComponent:
-        meta = {
-            template: undefined,
-            name: Ctor.name,
-        };
+
+    if (isUndefined(def)) {
+        if (!isComponentConstructor(Ctor)) {
+            throw new TypeError(
+                `${Ctor} is not a valid component, or does not extends LightningElement from "lwc". You probably forgot to add the extend clause on the class declaration.`
+            );
+        }
+
+        let meta = getComponentRegisteredMeta(Ctor);
+        if (isUndefined(meta)) {
+            // TODO: remove this workaround:
+            // this is temporary until
+            // all tests are updated to call registerComponent:
+            meta = {
+                template: undefined,
+                name: Ctor.name,
+            };
+        }
+
+        def = createComponentDef(Ctor, meta, subclassComponentName || Ctor.name);
+        CtorToDefMap.set(Ctor, def);
     }
-    def = createComponentDef(Ctor, meta, subclassComponentName || Ctor.name);
-    CtorToDefMap.set(Ctor, def);
+
     return def;
 }
 

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

@@ -10,9 +10,9 @@ import {
     assign,
     isNull,
     isObject,
+    isFunction,
     isTrue,
     isFalse,
-    isFunction,
     toString,
 } from '../shared/language';
 import {
@@ -24,7 +24,7 @@ import {
     VMState,
 } from './vm';
 import { ComponentConstructor } from './component';
-import { resolveCircularModuleDependency, isCircularModuleDependency, EmptyObject } from './utils';
+import { EmptyObject, isCircularModuleDependency, resolveCircularModuleDependency } from './utils';
 import { setInternalField, getInternalField, createFieldName } from '../shared/fields';
 import { isNativeShadowRootAvailable } from '../env/dom';
 import { patchCustomElementProto } from './patch';
@@ -70,6 +70,14 @@ assign(Node.prototype, {
     },
 });
 
+type ShadowDomMode = 'open' | 'closed';
+
+interface CreateElementOptions {
+    is: ComponentConstructor;
+    fallback?: boolean;
+    mode?: ShadowDomMode;
+}
+
 /**
  * This method is almost identical to document.createElement
  * (https://developer.mozilla.org/en-US/docs/Web/API/Document/createElement)
@@ -81,7 +89,7 @@ assign(Node.prototype, {
  * If the value of `is` attribute is not a constructor,
  * then it throws a TypeError.
  */
-export function createElement(sel: string, options: any): HTMLElement {
+export function createElement(sel: string, options: CreateElementOptions): HTMLElement {
     if (!isObject(options) || isNull(options)) {
         throw new TypeError(
             `"createElement" function expects an object as second parameter but received "${toString(
@@ -90,23 +98,18 @@ export function createElement(sel: string, options: any): HTMLElement {
         );
     }
 
-    let Ctor = (options as any).is as ComponentConstructor;
-
+    let Ctor = options.is;
     if (!isFunction(Ctor)) {
-        throw new TypeError(`"is" value must be a function but received "${toString(Ctor)}".`);
-    }
-
-    if (isCircularModuleDependency(Ctor)) {
-        Ctor = resolveCircularModuleDependency(Ctor);
+        throw new TypeError(
+            `"createElement" function expects a "is" option with a valid component constructor.`
+        );
     }
 
-    let { mode, fallback } = options as any;
-    // TODO: for now, we default to open, but eventually it should default to 'closed'
-    if (mode !== 'closed') {
-        mode = 'open';
-    }
-    // TODO: for now, we default to true, but eventually it should default to false
-    fallback = isUndefined(fallback) || isTrue(fallback) || isFalse(isNativeShadowRootAvailable);
+    const mode = options.mode !== 'closed' ? 'open' : 'closed';
+    const fallback =
+        isUndefined(options.fallback) ||
+        isTrue(options.fallback) ||
+        isFalse(isNativeShadowRootAvailable);
 
     // Create element with correct tagName
     const element = document.createElement(sel);
@@ -116,8 +119,14 @@ export function createElement(sel: string, options: any): HTMLElement {
         // to do here.
         return element;
     }
+
+    if (isCircularModuleDependency(Ctor)) {
+        Ctor = resolveCircularModuleDependency(Ctor);
+    }
+
     const def = getComponentDef(Ctor);
     setElementProto(element, def);
+
     if (isTrue(fallback)) {
         patchCustomElementProto(element, {
             def,

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

@@ -16,7 +16,7 @@ import {
     ArrayMap,
 } from '../shared/language';
 import { createVM, appendRootVM, removeRootVM, getCustomElementVM, CreateVMInit } from './vm';
-import { resolveCircularModuleDependency, isCircularModuleDependency, EmptyObject } from './utils';
+import { EmptyObject } from './utils';
 import { getComponentDef } from './def';
 import { tagNameGetter } from '../env/element';
 import { isNativeShadowRootAvailable } from '../env/dom';
@@ -29,9 +29,6 @@ export function buildCustomElementConstructor(
     Ctor: ComponentConstructor,
     options?: ShadowRootInit
 ): HTMLElementConstructor {
-    if (isCircularModuleDependency(Ctor)) {
-        Ctor = resolveCircularModuleDependency(Ctor);
-    }
     const { props, bridge: BaseElement } = getComponentDef(Ctor);
     const normalizedOptions: CreateVMInit = {
         fallback: false,

packages/integration-karma/test/api/buildCustomElementConstructor/index.spec.js

@@ -8,17 +8,18 @@ import LifecycleParent from 'x/lifecycleParent';
 const SUPPORTS_CUSTOM_ELEMENTS = !process.env.COMPAT && 'customElements' in window;
 
 function testInvalidOptions(type, obj) {
-    // TODO: investigate in prod
-    xit(`throws a ReferenceError if constructor is a ${type}`, () => {
-        expect(() => buildCustomElementConstructor(obj)).toThrowError(ReferenceError);
+    it(`throws a ReferenceError if constructor is a ${type}`, () => {
+        expect(() => buildCustomElementConstructor(obj)).toThrowError(
+            TypeError,
+            /.+ is not a valid component, or does not extends LightningElement from "lwc". You probably forgot to add the extend clause on the class declaration\./
+        );
     });
 }
 
-// TODO - #1010 buildCustomElementConstructor throws an obscure error when passing null or undefined
-// testInvalidOptions('undefined', undefined);
-// testInvalidOptions('null', null);
-
+testInvalidOptions('undefined', undefined);
+testInvalidOptions('null', null);
 testInvalidOptions('String', 'x-component');
+testInvalidOptions('Function', function() {});
 testInvalidOptions('Class not extending LightningElement', class Component {});
 testInvalidOptions('Object without the is property', {});
 

packages/integration-karma/test/api/createElement/index.spec.js

@@ -1,32 +1,60 @@
-import { LightningElement, createElement } from 'lwc';
+import { createElement, LightningElement } from 'lwc';
 
-class Component extends LightningElement {}
+import Test from 'x/test';
+import ShadowRootGetter from 'x/shadowRootGetter';
 
 function testInvalidOptions(type, option) {
     it(`throws a TypeError if option is a ${type}`, () => {
-        expect(() => createElement('x-component', option)).toThrowError(TypeError);
+        expect(() => createElement('x-component', option)).toThrowError(
+            TypeError,
+            /"createElement" function expects an object as second parameter but received/
+        );
     });
 }
 
 testInvalidOptions('undefined', undefined);
 testInvalidOptions('null', null);
 testInvalidOptions('String', 'x-component');
 testInvalidOptions('Class not extending LightningElement', class Component {});
-testInvalidOptions('Object without the is property', {});
+
+function testInvalidIsValue(type, isValue) {
+    it(`throws a TypeError if option.is is a ${type}`, () => {
+        expect(() => createElement('x-component', { is: isValue })).toThrowError(
+            TypeError,
+            '"createElement" function expects a "is" option with a valid component constructor.'
+        );
+    });
+}
+
+testInvalidIsValue('undefined', undefined);
+testInvalidIsValue('null', null);
+testInvalidIsValue('String', 'x-component');
+
+function testInvalidComponentConstructor(type, isValue) {
+    it(`throws a TypeError if option.is is a ${type}`, () => {
+        expect(() => createElement('x-component', { is: isValue })).toThrowError(
+            TypeError,
+            /.+ is not a valid component, or does not extends LightningElement from "lwc". You probably forgot to add the extend clause on the class declaration./
+        );
+    });
+}
+
+testInvalidComponentConstructor('Function', function() {});
+testInvalidComponentConstructor('Class not extending LightningElement', class Component {});
 
 it('returns an HTMLElement', () => {
-    const elm = createElement('x-component', { is: Component });
+    const elm = createElement('x-component', { is: Test });
     expect(elm instanceof HTMLElement).toBe(true);
 });
 
 it('should create an element with a synthetic shadow root by default', () => {
-    const elm = createElement('x-component', { is: Component });
+    const elm = createElement('x-component', { is: Test });
     expect(elm.shadowRoot.constructor.name).toBe('SyntheticShadowRoot');
 });
 
 it('supports component constructors in circular dependency', () => {
     function Circular() {
-        return Component;
+        return Test;
     }
     Circular.__circular__ = true;
 
@@ -37,7 +65,7 @@ it('supports component constructors in circular dependency', () => {
 if (process.env.NATIVE_SHADOW) {
     it('should create an element with a native shadow root if fallback is false', () => {
         const elm = createElement('x-component', {
-            is: Component,
+            is: Test,
             fallback: false,
         });
 
@@ -47,29 +75,23 @@ if (process.env.NATIVE_SHADOW) {
 
     it('should create a shadowRoot in open mode when mode in not specified', () => {
         const elm = createElement('x-component', {
-            is: Component,
+            is: Test,
             fallback: false,
         });
         expect(elm.shadowRoot.mode).toBe('open');
     });
 
     it('should create a shadowRoot in closed mode if the mode is passed as closed', () => {
-        // Since the shadowRoot property is not attached to the element in closed mode, we need to retrieve it by
-        // accessing the template property from the class constructor.
-        let shadowRoot;
-        class ClosedShadowComponent extends LightningElement {
-            constructor() {
-                super();
-                shadowRoot = this.template;
-            }
-        }
-
-        createElement('x-component', {
-            is: ClosedShadowComponent,
+        const elm = createElement('x-shadow-root-getter', {
+            is: ShadowRootGetter,
             fallback: false,
             mode: 'closed',
         });
 
+        // Since the shadowRoot property is not attached to the element in closed mode, we need to
+        // retrieve it by accessing the template property from the class.
+        const shadowRoot = elm.getShadowRoot();
+
         expect(shadowRoot instanceof ShadowRoot);
         expect(shadowRoot.mode).toBe('closed');
     });