chore: upgrade typescript from 3.1.6 to 3.3.3 #1045

ekashida · 2019-02-10T00:58:55Z

Things to note:

Type inference regression with overloads
apply invocation with the arguments object is no longer valid (disabled the error instead of converting to array because this is a very common practice)

Does this PR introduce a breaking change?

Yes
No

src/3rdparty/polymer/outer-html.ts(43,46): error TS2345: Argument of type '(c: any) => "&" | "<" | ">" | """ | " " | undefined' is not assignable to parameter of type '(substring: string, ...args: any[]) => string'. Type '"&" | "<" | ">" | """ | " " | undefined' is not assignable to type 'string'. Type 'undefined' is not assignable to type 'string'. src/3rdparty/polymer/outer-html.ts(47,46): error TS2345: Argument of type '(c: any) => "&" | "<" | ">" | """ | " " | undefined' is not assignable to parameter of type '(substring: string, ...args: any[]) => string'.

src/faux-shadow/events.ts(62,11): error TS2322: Type 'Element | null' is not assignable to type 'EventTarget'. Type 'null' is not assignable to type 'EventTarget'. src/faux-shadow/events.ts(119,23): error TS2322: Type 'Element | null' is not assignable to type 'EventTarget'. Type 'null' is not assignable to type 'EventTarget'. src/faux-shadow/events.ts(213,37): error TS2345: Argument of type 'Element | null' is not assignable to parameter of type 'HTMLElement'. Type 'null' is not assignable to type 'HTMLElement'.

src/faux-shadow/focus.ts(137,9): error TS2740: Type 'NodeListOf<Element>' is missing the following properties from type 'HTMLElement[]': pop, push, concat, join, and 24 more. src/faux-shadow/focus.ts(148,5): error TS2322: Type 'Element | null' is not assignable to type 'HTMLElement | null'. Type 'Element' is missing the following properties from type 'HTMLElement': accessKey, accessKeyLabel, autocapitalize, dir, and 109 more. src/faux-shadow/focus.ts(207,11): error TS2322: Type 'Element | null' is not assignable to type 'EventTarget'. Type 'null' is not assignable to type 'EventTarget'. src/faux-shadow/focus.ts(219,11): error TS2322: Type 'EventTarget | null' is not assignable to type 'EventTarget'. Type 'null' is not assignable to type 'EventTarget'. src/faux-shadow/focus.ts(248,11): error TS2322: Type 'Element | null' is not assignable to type 'EventTarget'. Type 'null' is not assignable to type 'EventTarget'. src/faux-shadow/focus.ts(250,11): error TS2322: Type 'EventTarget | null' is not assignable to type 'EventTarget'. Type 'null' is not assignable to type 'EventTarget'. src/faux-shadow/focus.ts(313,33): error TS2345: Argument of type 'Element' is not assignable to parameter of type 'HTMLElement'.

src/faux-shadow/iframe.ts(10,47): error TS2345: Argument of type 'IArguments' is not assignable to parameter of type '[any, string, (Transferable[] | undefined)?]'. Type 'IArguments' is missing the following properties from type '[any, string, (Transferable[] | undefined)?]': 0, 1, pop, push, and 26 more. src/faux-shadow/iframe.ts(13,40): error TS2345: Argument of type 'IArguments' is not assignable to parameter of type '[]'. Type 'IArguments' is missing the following properties from type '[]': pop, push, concat, join, and 24 more. src/faux-shadow/iframe.ts(16,41): error TS2345: Argument of type 'IArguments' is not assignable to parameter of type '[]'. src/faux-shadow/iframe.ts(19,41): error TS2345: Argument of type 'IArguments' is not assignable to parameter of type '[]'.

src/faux-shadow/shadow-root.ts(70,5): error TS2740: Type 'DocumentFragment' is missing the following properties from type 'SyntheticShadowRootInterface': mode, delegatesFocus, host, innerHTML, and 7 more. src/faux-shadow/shadow-root.ts(120,17): error TS2322: Type 'Element | null' is not assignable to type 'Element'. Type 'null' is not assignable to type 'Element'.

src/faux-shadow/slot.ts(72,5): error TS2740: Type '{}' is missing the following properties from type 'Node[]': length, pop, push, concat, and 26 more. src/faux-shadow/slot.ts(81,5): error TS2322: Type '{}' is not assignable to type 'Node[]'.

src/faux-shadow/traverse.ts(296,5): error TS2740: Type '{}' is missing the following properties from type 'Element[]': length, pop, push, concat, and 26 more. src/faux-shadow/traverse.ts(333,5): error TS2322: Type 'typeof PatchedHTMLIframeElement' is not assignable to type 'HTMLIFrameElementConstructor'. Types of property 'prototype' are incompatible. Type 'PatchedHTMLIframeElement' is not assignable to type 'HTMLIFrameElement'. Types of property 'contentWindow' are incompatible. Type '{ postMessage(): void; blur(): void; close(): void; focus(): void; readonly closed: boolean; readonly frames: Window; readonly length: number; location: Location; readonly opener: any; readonly parent: Window; readonly self: Window; readonly top: Window; readonly window: Window; }' is not assignable to type 'Window'. src/faux-shadow/traverse.ts(334,13): error TS2416: Property 'contentWindow' in type 'PatchedHTMLIframeElement' is not assignable to the same property in base type 'HTMLIFrameElement'. Type '{ postMessage(): void; blur(): void; close(): void; focus(): void; readonly closed: boolean; readonly frames: Window; readonly length: number; location: Location; readonly opener: any; readonly parent: Window; readonly self: Window; readonly top: Window; readonly window: Window; }' is missing the following properties from type 'Window': Blob, URL, URLSearchParams, applicationCache, and 216 more.

src/framework/attributes.ts(193,83): error TS2345: Argument of type '"aria-"' is not assignable to parameter of type '(substring: string, ...args: any[]) => string'.

src/framework/base-lightning-element.ts(226,37): error TS2345: Argument of type 'IArguments' is not assignable to parameter of type '[string, string]'. Type 'IArguments' is missing the following properties from type '[string, string]': 0, 1, pop, push, and 26 more. src/framework/base-lightning-element.ts(232,51): error TS2345: Argument of type 'IArguments' is not assignable to parameter of type '[string]'. src/framework/base-lightning-element.ts(239,53): error TS2345: Argument of type 'IArguments' is not assignable to parameter of type '[string | null, string]'. src/framework/base-lightning-element.ts(341,7): error TS2322: Type '{}' is not assignable to type 'PropertyDescriptorMap'. Index signature is missing in type '{}'.

src/framework/restrictions.ts(190,61): error TS2345: Argument of type 'IArguments' is not assignable to parameter of type '[string, EventListener | EventListenerObject | null, (boolean | AddEventListenerOptions | undefined)?]'. Type 'IArguments' is missing the following properties from type '[string, EventListener | EventListenerObject | null, (boolean | AddEventListenerOptions | undefined)?]': 0, 1, pop, push, and 26 more. src/framework/restrictions.ts(197,58): error TS2345: Argument of type 'IArguments' is not assignable to parameter of type '[string]'. Type 'IArguments' is missing the following properties from type '[string]': 0, pop, push, concat, and 25 more. src/framework/restrictions.ts(204,61): error TS2345: Argument of type 'IArguments' is not assignable to parameter of type '[string]'. src/framework/restrictions.ts(348,61): error TS2345: Argument of type 'IArguments' is not assignable to parameter of type '[string, EventListenerOrEventListenerObject, (boolean | AddEventListenerOptions | undefined)?]'. Type 'IArguments' is missing the following properties from type '[string, EventListenerOrEventListenerObject, (boolean | AddEventListenerOptions | undefined)?]': 0, 1, pop, push, and 26 more. src/framework/restrictions.ts(400,50): error TS2345: Argument of type 'IArguments' is not assignable to parameter of type '[string, any]'. Type 'IArguments' is missing the following properties from type '[string, any]': 0, 1, pop, push, and 26 more.

src/framework/def.ts(229,7): error TS2322: Type '{}' is not assignable to type 'PropsDef'. Index signature is missing in type '{}'.

src/faux-shadow/custom-element.ts(91,42): error TS2339: Property 'blur' does not exist on type 'Element'.

src/framework/services.ts(57,32): error TS2345: Argument of type 'ComponentInterface | undefined' is not assignable to parameter of type 'object'. Type 'undefined' is not assignable to type 'object'.

src/framework/stylesheet.ts(27,9): error TS2339: Property 'type' does not exist on type 'HTMLElement'. src/framework/stylesheet.ts(112,9): error TS2322: Type 'void' is not assignable to type 'VNode | null'. src/framework/stylesheet.ts(123,55): error TS2345: Argument of type '{}' is not assignable to parameter of type 'string'.

src/framework/template.ts(136,54): error TS2345: Argument of type 'ComponentInterface | undefined' is not assignable to parameter of type 'object'. Type 'undefined' is not assignable to type 'object'.

src/framework/vm.ts(66,26): error TS2345: Argument of type 'any[] | undefined' is not assignable to parameter of type 'any[]'. Type 'undefined' is not assignable to type 'any[]'. src/framework/vm.ts(498,5): error TS2322: Type 'Element | null' is not assignable to type 'HTMLElement | null'. Type 'Element' is missing the following properties from type 'HTMLElement': accessKey, accessKeyLabel, autocapitalize, dir, and 109 more. src/framework/vm.ts(513,37): error TS2345: Argument of type 'Element | null' is not assignable to parameter of type 'ShadowRoot | null'. Type 'Element' is missing the following properties from type 'ShadowRoot': host, mode, activeElement, styleSheets, and 6 more.

src/polyfills/aria-properties/polyfill.ts(69,86): error TS2345: Argument of type '"aria-"' is not assignable to parameter of type '(substring: string, ...args: any[]) => string'.

src/polyfills/document-shadow/polyfill.ts(36,49): error TS2345: Argument of type 'Element | null' is not assignable to parameter of type 'Node'. Type 'null' is not assignable to type 'Node'. src/polyfills/document-shadow/polyfill.ts(39,17): error TS2531: Object is possibly 'null'.

src/shared/assert.ts(98,73): error TS2345: Argument of type '"\n"' is not assignable to parameter of type '{ [Symbol.split](string: string, limit?: number | undefined): string[]; }'.

salesforce-best-lwc-internal · 2019-02-10T01:11:16Z

Benchmark results

Base commit: 116cf6e | Target commit: 3c021e4

lwc-engine-benchmark

table-append-1k	metric	base(`116cf6e`)	target(`3c021e4`)	trend
benchmark-table/append/1k	duration	149.80 (±5.65 ms)	156.65 (±4.45 ms)	+6.8ms (4.6%) 👎

table-clear-1k	metric	base(`116cf6e`)	target(`3c021e4`)	trend
benchmark-table/clear/1k	duration	6.40 (±0.40 ms)	6.80 (±0.35 ms)	+0.4ms (6.2%) 👎

table-create-10k	metric	base(`116cf6e`)	target(`3c021e4`)	trend
benchmark-table/create/10k	duration	898.20 (±7.95 ms)	914.05 (±6.50 ms)	+15.8ms (1.8%) 👎

table-create-1k	metric	base(`116cf6e`)	target(`3c021e4`)	trend
benchmark-table/create/1k	duration	119.05 (±3.15 ms)	121.40 (±2.70 ms)	+2.4ms (2.0%) 👎

table-update-10th-1k	metric	base(`116cf6e`)	target(`3c021e4`)	trend
benchmark-table/update-10th/1k	duration	76.10 (±2.65 ms)	78.40 (±3.85 ms)	+2.3ms (3.0%) 👎

tablecmp-append-1k	metric	base(`116cf6e`)	target(`3c021e4`)	trend
benchmark-table-component/append/1k	duration	258.60 (±7.00 ms)	260.10 (±5.75 ms)	+1.5ms (0.6%) 👌

tablecmp-clear-1k	metric	base(`116cf6e`)	target(`3c021e4`)	trend
benchmark-table-component/clear/1k	duration	13.10 (±1.55 ms)	13.00 (±1.85 ms)	-0.1ms (0.8%) 👌

tablecmp-create-10k	metric	base(`116cf6e`)	target(`3c021e4`)	trend
benchmark-table-component/create/10k	duration	1760.85 (±13.15 ms)	1805.45 (±13.85 ms)	+44.6ms (2.5%) 👎

tablecmp-create-1k	metric	base(`116cf6e`)	target(`3c021e4`)	trend
benchmark-table-component/create/1k	duration	214.20 (±6.05 ms)	215.45 (±4.70 ms)	+1.3ms (0.6%) 👌

tablecmp-update-10th-1k	metric	base(`116cf6e`)	target(`3c021e4`)	trend
benchmark-table-component/update-10th/1k	duration	70.10 (±4.95 ms)	70.40 (±3.30 ms)	+0.3ms (0.4%) 👌

wc-append-1k	metric	base(`116cf6e`)	target(`3c021e4`)	trend
benchmark-table-wc/append/1k	duration	260.60 (±6.00 ms)	264.90 (±5.70 ms)	+4.3ms (1.7%) 👎

wc-clear-1k	metric	base(`116cf6e`)	target(`3c021e4`)	trend
benchmark-table-wc/clear/1k	duration	23.20 (±2.45 ms)	23.95 (±2.65 ms)	+0.8ms (3.2%) 👌

wc-create-10k	metric	base(`116cf6e`)	target(`3c021e4`)	trend
benchmark-table-wc/create/10k	duration	2001.90 (±13.40 ms)	2050.70 (±14.40 ms)	+48.8ms (2.4%) 👎

wc-create-1k	metric	base(`116cf6e`)	target(`3c021e4`)	trend
benchmark-table-wc/create/1k	duration	225.70 (±5.75 ms)	231.30 (±4.75 ms)	+5.6ms (2.5%) 👎

wc-update-10th-1k	metric	base(`116cf6e`)	target(`3c021e4`)	trend
benchmark-table-wc/update-10th/1k	duration	70.10 (±4.20 ms)	70.40 (±3.15 ms)	+0.3ms (0.4%) 👌

apapko · 2019-02-10T03:07:54Z

packages/@lwc/engine/src/faux-shadow/focus.ts

@@ -139,7 +139,7 @@ function getTabbableSegments(host: HTMLElement): QuerySegments {
    };
 }

-export function getActiveElement(host: HTMLElement): HTMLElement | null {
+export function getActiveElement(host: HTMLElement): Element | null {


why did the return type change?

The activeElement read-only property of the Document and ShadowRoot interfaces returns the Element within the DOM or shadow DOM tree that currently has focus.

yes, it could be an element.

apapko · 2019-02-10T03:09:09Z

packages/@lwc/engine/src/faux-shadow/focus.ts

-    const target: EventTarget = eventTargetGetter.call(event);
+    // currentTarget should always be defined
+    const host = eventCurrentTargetGetter.call(event) as EventTarget;
+    const target = eventTargetGetter.call(event);


do we not need 'as EventTarget' here as well?

const target = eventTargetGetter.call(event) as EventTarget

I think the answer is that target and relatedTarget are always there... while currentTarget is not... it could be null, so I think this is correct because in this method, we are checking the event during the dispatch phase, which always have a currentTarget.

apapko · 2019-02-10T03:13:52Z

packages/@lwc/engine/src/faux-shadow/focus.ts

@@ -307,10 +310,14 @@ function stopFocusIn(evt) {

 function handleFocusMouseDown(evt) {
    const target = eventTargetGetter.call(evt);
+    if (isNull(target)) {


can we add a comment describing why the target would be null here

yeah, I don't have an explanation for this one.

Not sure what I did here either, it seems that asserting the type HTMLElement is enough.

apapko · 2019-02-10T03:15:34Z

packages/@lwc/engine/src/faux-shadow/iframe.ts

@@ -7,16 +7,20 @@
 export function wrapIframeWindow(win: Window) {
    return {
        postMessage() {
-            return win.postMessage.apply(win, arguments);
+            const args = arguments as unknown;


Would casting to any not work?

const args = arguments as any[]

Isn't easier to just ArraySlice arguments here?

Or maybe change the signature method signature to:

{ postMessage(...args) { return win.postMessage.apply(win, args); }, }

@pmdartus I don't remember the transpiled code for (...args), but I remember it was some weird transformation that was pretty slow... maybe I'm getting confused with the default value.

I was just trying to avoid the array conversion in general since this is not a problem in javascript. In terms of performance, the postMessage method seems questionable but the other ones are probably safe.

For the info, the rest parameter syntax is transformed an array copying using a standard for loop. Based on the this benchmark, the slice is 40% faster than a pure array iteration.

apapko · 2019-02-10T03:19:05Z

packages/@lwc/engine/src/faux-shadow/shadow-root.ts

@@ -67,7 +67,7 @@ export function attachShadow(elm: HTMLElement, options: ShadowRootInit): Synthet
    if (process.env.NODE_ENV === 'test') {
        elm['$$ShadowRoot$$'] = sr; // tslint:disable-line
    }
-    return sr;
+    return sr as SyntheticShadowRootInterface;


it's interesting that the return type is not matching the type here. Is typescript expecting 'as' casting during the initial value assignment on line 53?

The type is set to DocumentFragment on line 53 but we're saying that the return type is SyntheticShadowRootInterface.

apapko · 2019-02-10T03:21:22Z

packages/@lwc/engine/src/faux-shadow/traverse.ts

    return class PatchedHTMLIframeElement extends Ctor {
+        // @ts-ignore type-mismatch


we should probably log a work item to keep track of these @ts-ingores.

While I don't think we can fix the type mismatch on PatchedHTMLIframeElement. We can fix the one of contentWindow.

The contentWindow should wrap the current contentWindow and return another object of type Window. The wrapIframeWindow needs its return type to be Window.

apapko · 2019-02-10T03:24:30Z

packages/@lwc/engine/src/framework/base-lightning-element.ts

        const elm = getLinkedElement(this);
        if (process.env.NODE_ENV !== 'production') {
            assert.isFalse(isBeingConstructed(getComponentVM(this)), `Failed to construct '${getComponentAsString(this)}': The result must not have attributes.`);
        }
        unlockAttribute(elm, attrName);
+        // Typescript does not like it when you treat the `arguments` object as an array


can we use the same workaround you had earlier

const args = arguments as unknown; elm.setAttributeNS.apply(elm, args as any [])

I think disabling the rule along with a comment is cleaner, but I'm also not opposed to array conversion. I was just worried about needless performance hits with the latter.

@caridy @pmdartus thoughts?

There is no performance hit at runtime, types are only for the typescript compiler, those are not used at runtime. I personally prefer the type casting to any[], but I would defer the decision to @caridy.

I think @ekashida is referencing to ArraySlice call for argument, which is going to be slower for sure.

Will leave this as-is unless anyone has further objections.

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

@@ -23,7 +23,7 @@ export type StylesheetFactory = (hostSelector: string, shadowSelector: string, n
 const CachedStyleFragments: Record<string, DocumentFragment> = create(null);

 function createStyleElement(styleContent: string): Element {
-    const elm = createElement.call(document, 'style');
+    const elm = createElement.call(document, 'style') as HTMLStyleElement;


apapko · 2019-02-10T03:28:51Z

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

    const vnodes: VNodes = html.call(undefined, api, component, cmpSlots, context.tplCache);

    const { styleVNode } = context;
-    if (!isUndefined(context.styleVNode)) {
+    if (styleVNode) {


should this be isNull(styleVNode)?

I guess you meant !isNull(styleVNode). What is the benefit of if (!isNull(styleVNode)) over if (styleVNode) here? The type system already ensures that the only falsy value it can have is null, doesn't it?

I think it is a perf optimization

apapko · 2019-02-10T03:31:17Z

packages/@lwc/engine/src/polyfills/document-shadow/polyfill.ts

            let node = DocumentPrototypeActiveElement.call(this);

            if (isNull(node)) {
                return node;
            }

-            while (!isUndefined(getNodeOwnerKey(node))) {
+            while (!isUndefined(getNodeOwnerKey(node as Node))) {


should the 'as' casting be done during declaration on line 30?

That would be messy since parentElementGetter returns an Element.

That would be messy since parentElementGetter returns an Element.

@apapko What I meant by this is that node is referenced in this function several times as an Element (line 37 as well as the return value) and we would need to handle those. Does that make sense?

caridy · 2019-02-10T03:50:58Z

packages/@lwc/engine/src/faux-shadow/events.ts

@@ -115,12 +116,13 @@ export function patchEvent(event: Event) {
    if (!isUndefined(originalRelatedTargetDescriptor)) {
        defineProperty(event, 'relatedTarget', {
            get(this: ComposableEvent): EventTarget | null | undefined {
-                const eventContext = eventToContextMap.get(this);
-                const originalCurrentTarget: EventTarget = eventCurrentTargetGetter.call(this);
+                // currentTarget is always defined


I don't think this is true... if you hold onto the event instance, and access relatedTarget in the next tick, what's the value of currentTarget?

You are right, currentTarget is null in this case.

caridy · 2019-02-10T03:51:21Z

packages/@lwc/engine/src/faux-shadow/events.ts

@@ -209,7 +211,8 @@ function domListener(evt: Event) {
    let immediatePropagationStopped = false;
    let propagationStopped = false;
    const { type, stopImmediatePropagation, stopPropagation } = evt;
-    const currentTarget = eventCurrentTargetGetter.call(evt);
+    // currentTarget is always defined


here on the other hand, it is always defined!

caridy · 2019-02-10T03:57:35Z

packages/@lwc/engine/src/faux-shadow/focus.ts

@@ -295,6 +297,7 @@ function willTriggerFocusInEvent(target: HTMLElement): boolean {
 }

 function stopFocusIn(evt) {
+    // currentTarget should always be defined


yes, here this is true.

caridy · 2019-02-10T04:00:07Z

packages/@lwc/engine/src/faux-shadow/shadow-root.ts

@@ -117,7 +117,7 @@ const ShadowRootDescriptors = {
            // activeElement must be child of the host and owned by it
            let node = activeElement;
            while (!isNodeOwnedBy(host, node)) {
-                node = parentElementGetter.call(node);
+                node = parentElementGetter.call(node) as HTMLElement;


there is no guarantee that parentElement exists, why forcing it?

If activeElement is not null and is contained by the host (which is an HTMLElement), then doesn't it mean that node can never be null since we would stop bubbling up at the host?

caridy · 2019-02-10T04:03:30Z

packages/@lwc/engine/src/framework/base-lightning-element.ts

@@ -193,46 +193,58 @@ BaseLightningElement.prototype = {
        const wrappedListener = getWrappedComponentsListener(vm, listener);
        vm.elm.removeEventListener(type, wrappedListener, options);
    },
-    setAttributeNS(ns: string, attrName: string, value: any) {
+    setAttributeNS(ns: string | null, attrName: string, value: string) {


can this ns argument really be null?

This is the definition in dom.d.ts: setAttributeNS(namespace: string | null, qualifiedName: string, value: string): void;

caridy · 2019-02-10T04:12:01Z

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

@@ -53,6 +53,9 @@ export function invokeServiceHook(vm: VM, cbs: ServiceCallback[]) {
        assert.isTrue(isArray(cbs) && cbs.length > 0, `Optimize invokeServiceHook() to be invoked only when needed`);
    }
    const { component, data, def, context } = vm;
+    if (isUndefined(component)) {


component can't never be undefined at this point... this is a problem with few things of vm that are defined as possible undefined because they need to be defined right after... but this affects the whole codebase where it complains about undefined values.

I was thinking about this, there are two possible solutions:

having VM and UninitializedVM types... or something like that...

using the trick in TS to define an empty obj to be fill out right after... but that might not solve the issue because some stuff are added to VM in the super call for LightningElement

I like the idea of having 2 types with one for construction and another one for runtime.

caridy · 2019-02-10T04:13:23Z

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

@@ -133,10 +133,13 @@ export function evaluateTemplate(vm: VM, html: Template): Array<VNode|null> {
        // validating slots in every rendering since the allocated content might change over time
        validateSlots(vm, html);
    }
+
+    // TODO: Can `component` ever be undefined at this point?
+    // @ts-ignore type-mismatch


never... same explanation that above

caridy · 2019-02-10T04:15:11Z

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

@@ -451,8 +451,8 @@ function getErrorBoundaryVMFromOwnElement(vm: VM): VM | undefined {
    return getErrorBoundaryVM(elm);
 }

-function getErrorBoundaryVM(startingElement: HTMLElement | null): VM | undefined {
-    let elm: HTMLElement | null = startingElement;
+function getErrorBoundaryVM(startingElement: Element | null): VM | undefined {


I don't think we ever get a native element to throw something that is captured by the boundary, not sure

I can change it back but then does that mean that getParentOrHostElement should always return HTMLElement?
https://github.com/salesforce/lwc/pull/1045/files#diff-7ccfccbe4c1028889b761acec660f1afR443

caridy

This is looking very good, just minor stuff and few questions.

pmdartus · 2019-02-11T16:41:53Z

packages/@lwc/engine/src/faux-shadow/events.ts

@@ -115,12 +116,13 @@ export function patchEvent(event: Event) {
    if (!isUndefined(originalRelatedTargetDescriptor)) {
        defineProperty(event, 'relatedTarget', {
            get(this: ComposableEvent): EventTarget | null | undefined {
-                const eventContext = eventToContextMap.get(this);
-                const originalCurrentTarget: EventTarget = eventCurrentTargetGetter.call(this);
+                // currentTarget is always defined


You are right, currentTarget is null in this case.

pmdartus · 2019-02-11T16:44:09Z

packages/@lwc/engine/src/faux-shadow/events.ts

@@ -209,7 +211,8 @@ function domListener(evt: Event) {
    let immediatePropagationStopped = false;
    let propagationStopped = false;
    const { type, stopImmediatePropagation, stopPropagation } = evt;
-    const currentTarget = eventCurrentTargetGetter.call(evt);
+    // currentTarget is always defined
+    const currentTarget = eventCurrentTargetGetter.call(evt) as HTMLElement;


Why not casting it to HTMLElement instead of just EventTarget?

We could do that, but then we would have to change the signature of getEventMap:

function getEventMap(elm: HTMLElement): ListenerMap {...}

I think it makes sense, event handlers are associated with an EventTarget.

pmdartus · 2019-02-11T16:49:32Z

packages/@lwc/engine/src/faux-shadow/focus.ts

-    const host: EventTarget = eventCurrentTargetGetter.call(event);
-    const target: EventTarget = eventTargetGetter.call(event);
+    // currentTarget should always be defined
+    const host = eventCurrentTargetGetter.call(event) as EventTarget;


Instead of as EventTarget, you should add ! at the end to make it non-null:

const host = eventCurrentTargetGetter.call(event)!

It would avoid having to update the type you cast to if the method happened to change signature.

pmdartus · 2019-02-11T16:50:13Z

packages/@lwc/engine/src/faux-shadow/focus.ts

-    const target: EventTarget = eventTargetGetter.call(event);
-    const relatedTarget: EventTarget = focusEventRelatedTargetGetter.call(event);
+    // currentTarget should always be defined
+    const host = eventCurrentTargetGetter.call(event) as EventTarget;


Use the ! instead of the explicit type cast.

pmdartus · 2019-02-11T16:56:04Z

packages/@lwc/engine/src/faux-shadow/iframe.ts

@@ -7,16 +7,20 @@
 export function wrapIframeWindow(win: Window) {
    return {
        postMessage() {
-            return win.postMessage.apply(win, arguments);
+            const args = arguments as unknown;


Or maybe change the signature method signature to:

{ postMessage(...args) { return win.postMessage.apply(win, args); }, }

pmdartus · 2019-02-11T17:57:46Z

packages/@lwc/engine/src/faux-shadow/traverse.ts

    return class PatchedHTMLIframeElement extends Ctor {
+        // @ts-ignore type-mismatch


While I don't think we can fix the type mismatch on PatchedHTMLIframeElement. We can fix the one of contentWindow.

The contentWindow should wrap the current contentWindow and return another object of type Window. The wrapIframeWindow needs its return type to be Window.

pmdartus · 2019-02-11T18:00:07Z

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

@@ -53,6 +53,9 @@ export function invokeServiceHook(vm: VM, cbs: ServiceCallback[]) {
        assert.isTrue(isArray(cbs) && cbs.length > 0, `Optimize invokeServiceHook() to be invoked only when needed`);
    }
    const { component, data, def, context } = vm;
+    if (isUndefined(component)) {


I like the idea of having 2 types with one for construction and another one for runtime.

salesforce-best-lwc-internal · 2019-02-12T03:26:16Z

Benchmark results

Base commit: 116cf6e | Target commit: 5e00030

lwc-engine-benchmark

table-append-1k	metric	base(`116cf6e`)	target(`5e00030`)	trend
benchmark-table/append/1k	duration	149.80 (±5.65 ms)	149.75 (±5.70 ms)	-0.1ms (0.0%) 👌

table-clear-1k	metric	base(`116cf6e`)	target(`5e00030`)	trend
benchmark-table/clear/1k	duration	6.40 (±0.40 ms)	6.25 (±0.55 ms)	-0.2ms (2.3%) 👌

table-create-10k	metric	base(`116cf6e`)	target(`5e00030`)	trend
benchmark-table/create/10k	duration	898.20 (±7.95 ms)	896.25 (±7.00 ms)	-2.0ms (0.2%) 👌

table-create-1k	metric	base(`116cf6e`)	target(`5e00030`)	trend
benchmark-table/create/1k	duration	119.05 (±3.15 ms)	121.30 (±3.25 ms)	+2.3ms (1.9%) 👌

table-update-10th-1k	metric	base(`116cf6e`)	target(`5e00030`)	trend
benchmark-table/update-10th/1k	duration	76.10 (±2.65 ms)	76.20 (±3.45 ms)	+0.1ms (0.1%) 👌

tablecmp-append-1k	metric	base(`116cf6e`)	target(`5e00030`)	trend
benchmark-table-component/append/1k	duration	258.60 (±7.00 ms)	241.55 (±16.50 ms)	-17.1ms (6.6%) 👍

tablecmp-clear-1k	metric	base(`116cf6e`)	target(`5e00030`)	trend
benchmark-table-component/clear/1k	duration	13.10 (±1.55 ms)	12.45 (±1.75 ms)	-0.7ms (5.0%) 👌

tablecmp-create-10k	metric	base(`116cf6e`)	target(`5e00030`)	trend
benchmark-table-component/create/10k	duration	1760.85 (±13.15 ms)	1802.95 (±18.35 ms)	+42.1ms (2.4%) 👎

tablecmp-create-1k	metric	base(`116cf6e`)	target(`5e00030`)	trend
benchmark-table-component/create/1k	duration	214.20 (±6.05 ms)	211.75 (±5.85 ms)	-2.4ms (1.1%) 👌

tablecmp-update-10th-1k	metric	base(`116cf6e`)	target(`5e00030`)	trend
benchmark-table-component/update-10th/1k	duration	70.10 (±4.95 ms)	69.75 (±5.70 ms)	-0.3ms (0.5%) 👌

wc-append-1k	metric	base(`116cf6e`)	target(`5e00030`)	trend
benchmark-table-wc/append/1k	duration	260.60 (±6.00 ms)	258.35 (±5.80 ms)	-2.3ms (0.9%) 👌

wc-clear-1k	metric	base(`116cf6e`)	target(`5e00030`)	trend
benchmark-table-wc/clear/1k	duration	23.20 (±2.45 ms)	22.45 (±2.95 ms)	-0.8ms (3.2%) 👌

wc-create-10k	metric	base(`116cf6e`)	target(`5e00030`)	trend
benchmark-table-wc/create/10k	duration	2001.90 (±13.40 ms)	2026.75 (±13.70 ms)	+24.8ms (1.2%) 👎

wc-create-1k	metric	base(`116cf6e`)	target(`5e00030`)	trend
benchmark-table-wc/create/1k	duration	225.70 (±5.75 ms)	228.20 (±4.20 ms)	+2.5ms (1.1%) 👌

wc-update-10th-1k	metric	base(`116cf6e`)	target(`5e00030`)	trend
benchmark-table-wc/update-10th/1k	duration	70.10 (±4.20 ms)	71.95 (±5.10 ms)	+1.8ms (2.6%) 👌

caridy

I'm good with this, but a lot of test failures atm

salesforce-best-lwc-internal · 2019-02-13T02:45:01Z

Benchmark results

Base commit: f5a3a71 | Target commit: 2ffd1ab

lwc-engine-benchmark

table-append-1k	metric	base(`f5a3a71`)	target(`2ffd1ab`)	trend
benchmark-table/append/1k	duration	151.85 (±4.85 ms)	152.30 (±4.30 ms)	+0.5ms (0.3%) 👌

table-clear-1k	metric	base(`f5a3a71`)	target(`2ffd1ab`)	trend
benchmark-table/clear/1k	duration	6.10 (±0.50 ms)	6.30 (±0.40 ms)	+0.2ms (3.3%) 👌

table-create-10k	metric	base(`f5a3a71`)	target(`2ffd1ab`)	trend
benchmark-table/create/10k	duration	895.35 (±6.60 ms)	892.10 (±7.30 ms)	-3.3ms (0.4%) 👍

table-create-1k	metric	base(`f5a3a71`)	target(`2ffd1ab`)	trend
benchmark-table/create/1k	duration	118.70 (±3.35 ms)	119.80 (±2.60 ms)	+1.1ms (0.9%) 👌

table-update-10th-1k	metric	base(`f5a3a71`)	target(`2ffd1ab`)	trend
benchmark-table/update-10th/1k	duration	74.15 (±2.20 ms)	85.00 (±3.45 ms)	+10.8ms (14.6%) 👎

tablecmp-append-1k	metric	base(`f5a3a71`)	target(`2ffd1ab`)	trend
benchmark-table-component/append/1k	duration	251.40 (±14.15 ms)	248.45 (±11.60 ms)	-3.0ms (1.2%) 👌

tablecmp-clear-1k	metric	base(`f5a3a71`)	target(`2ffd1ab`)	trend
benchmark-table-component/clear/1k	duration	11.75 (±1.20 ms)	12.80 (±1.80 ms)	+1.1ms (8.9%) 👌

tablecmp-create-10k	metric	base(`f5a3a71`)	target(`2ffd1ab`)	trend
benchmark-table-component/create/10k	duration	1770.35 (±11.60 ms)	1787.50 (±16.25 ms)	+17.2ms (1.0%) 👎

tablecmp-create-1k	metric	base(`f5a3a71`)	target(`2ffd1ab`)	trend
benchmark-table-component/create/1k	duration	210.85 (±5.80 ms)	218.00 (±6.05 ms)	+7.1ms (3.4%) 👎

tablecmp-update-10th-1k	metric	base(`f5a3a71`)	target(`2ffd1ab`)	trend
benchmark-table-component/update-10th/1k	duration	69.30 (±4.80 ms)	68.50 (±4.30 ms)	-0.8ms (1.2%) 👌

wc-append-1k	metric	base(`f5a3a71`)	target(`2ffd1ab`)	trend
benchmark-table-wc/append/1k	duration	261.85 (±4.65 ms)	260.45 (±6.40 ms)	-1.4ms (0.5%) 👌

wc-clear-1k	metric	base(`f5a3a71`)	target(`2ffd1ab`)	trend
benchmark-table-wc/clear/1k	duration	21.95 (±2.10 ms)	22.40 (±2.85 ms)	+0.4ms (2.1%) 👌

wc-create-10k	metric	base(`f5a3a71`)	target(`2ffd1ab`)	trend
benchmark-table-wc/create/10k	duration	2036.95 (±15.80 ms)	2042.70 (±17.00 ms)	+5.8ms (0.3%) 👌

wc-create-1k	metric	base(`f5a3a71`)	target(`2ffd1ab`)	trend
benchmark-table-wc/create/1k	duration	230.35 (±5.25 ms)	230.70 (±6.10 ms)	+0.3ms (0.2%) 👌

wc-update-10th-1k	metric	base(`f5a3a71`)	target(`2ffd1ab`)	trend
benchmark-table-wc/update-10th/1k	duration	70.90 (±5.60 ms)	71.55 (±6.35 ms)	+0.7ms (0.9%) 👌

ekashida added 21 commits February 9, 2019 16:57

chore: bump typescript from 3.1.6 to 3.3.3

cf34a97

chore: fix tsc error for slot.ts

e6aa2e4

src/faux-shadow/slot.ts(72,5): error TS2740: Type '{}' is missing the following properties from type 'Node[]': length, pop, push, concat, and 26 more. src/faux-shadow/slot.ts(81,5): error TS2322: Type '{}' is not assignable to type 'Node[]'.

chore: fix tsc error for attributes.ts

ffc3871

src/framework/attributes.ts(193,83): error TS2345: Argument of type '"aria-"' is not assignable to parameter of type '(substring: string, ...args: any[]) => string'.

chore: fix tsc error for def.ts

0bbd026

src/framework/def.ts(229,7): error TS2322: Type '{}' is not assignable to type 'PropsDef'. Index signature is missing in type '{}'.

chore: fix tsc error for custom-element.ts

86efd58

src/faux-shadow/custom-element.ts(91,42): error TS2339: Property 'blur' does not exist on type 'Element'.

chore: fix tsc error for services.ts

bd79a36

src/framework/services.ts(57,32): error TS2345: Argument of type 'ComponentInterface | undefined' is not assignable to parameter of type 'object'. Type 'undefined' is not assignable to type 'object'.

chore: fix tsc error for template.ts

9853784

src/framework/template.ts(136,54): error TS2345: Argument of type 'ComponentInterface | undefined' is not assignable to parameter of type 'object'. Type 'undefined' is not assignable to type 'object'.

chore: fix tsc error for aria-properties/polyfill.ts

e11dfe0

src/polyfills/aria-properties/polyfill.ts(69,86): error TS2345: Argument of type '"aria-"' is not assignable to parameter of type '(substring: string, ...args: any[]) => string'.

chore: fix tsc error for assert.ts

1cd7032

src/shared/assert.ts(98,73): error TS2345: Argument of type '"\n"' is not assignable to parameter of type '{ [Symbol.split](string: string, limit?: number | undefined): string[]; }'.

chore: fix tsc error for property-trap.ts

3c021e4

apapko reviewed Feb 10, 2019

View reviewed changes

caridy reviewed Feb 10, 2019

View reviewed changes

pmdartus reviewed Feb 11, 2019

View reviewed changes

chore: review feedback

5e00030

pmdartus approved these changes Feb 12, 2019

View reviewed changes

apapko approved these changes Feb 12, 2019

View reviewed changes

ekashida added 3 commits February 12, 2019 14:02

chore: review feedback

520129e

chore: introduce UninitializedVM interface

a22774e

chore: review feedback

0a8fce8

caridy approved these changes Feb 13, 2019

View reviewed changes

fix: test failures

2ffd1ab

ekashida merged commit 3617361 into master Feb 13, 2019

ekashida deleted the ekashida/upgrade-ts branch February 13, 2019 02:48

		return class PatchedHTMLIframeElement extends Ctor {
		// @ts-ignore type-mismatch

chore: upgrade typescript from 3.1.6 to 3.3.3 #1045

chore: upgrade typescript from 3.1.6 to 3.3.3 #1045

Conversation

ekashida commented Feb 10, 2019 • edited Loading

Does this PR introduce a breaking change?

salesforce-best-lwc-internal bot commented Feb 10, 2019

Benchmark results

lwc-engine-benchmark

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekashida Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ekashida Feb 12, 2019 • edited Loading

Choose a reason for hiding this comment

caridy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

salesforce-best-lwc-internal bot commented Feb 12, 2019

Benchmark results

lwc-engine-benchmark

caridy left a comment

Choose a reason for hiding this comment

salesforce-best-lwc-internal bot commented Feb 13, 2019

Benchmark results

lwc-engine-benchmark

ekashida commented Feb 10, 2019 •

edited

Loading

ekashida Feb 12, 2019 •

edited

Loading

ekashida Feb 12, 2019 •

edited

Loading