refactor: hidden fields instead of internal fields (2) (#1485)

* chore: package-unique keys for engine and synthetic shadow

* refactor: remove internal fields in favor of hidden fields

* fix: avoid returning boolean false when field undefined

* test: use hidden fields instead of internal fields

* refactor(synthetic-shadow): hidden fields instead of internal fields

* chore(synthetic-shadow): "unique" string keys

* fix(engine): issue 1299 second attempt

* fix(engine): removing hasOwnProperty check

* chore: revert changes for vm access by getComponentConstructor

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

@@ -8,6 +8,7 @@ import { compileTemplate } from 'test-utils';
 import { createElement, LightningElement, registerDecorators } from '../main';
 import { ViewModelReflection } from '../utils';
 import { getComponentVM } from '../vm';
+import { getHiddenField } from '../../shared/fields';
 
 const emptyTemplate = compileTemplate(`<template></template>`);
 
@@ -16,24 +17,26 @@ describe('vm', () => {
         it('should have idx>0 (creation index) during construction', () => {
             class MyComponent1 extends LightningElement {}
             const elm = createElement('x-foo', { is: MyComponent1 });
-            expect(elm[ViewModelReflection].idx).toBeGreaterThan(0);
+            const hiddenFields = getHiddenField(elm, ViewModelReflection);
+            expect(hiddenFields.idx).toBeGreaterThan(0);
         });
 
         it('should have idx>0 after insertion', () => {
             class MyComponent2 extends LightningElement {}
             const elm = createElement('x-foo', { is: MyComponent2 });
             document.body.appendChild(elm);
-            expect(elm[ViewModelReflection].idx).toBeGreaterThan(0);
+            const hiddenFields = getHiddenField(elm, ViewModelReflection);
+            expect(hiddenFields.idx).toBeGreaterThan(0);
         });
 
         it('should preserve insertion index after removal of root', () => {
             class MyComponent3 extends LightningElement {}
             const elm = createElement('x-foo', { is: MyComponent3 });
             document.body.appendChild(elm);
-            const idx = elm[ViewModelReflection].idx;
-            expect(idx).toBeGreaterThan(0);
+            const hiddenFields = getHiddenField(elm, ViewModelReflection);
+            expect(hiddenFields.idx).toBeGreaterThan(0);
             document.body.removeChild(elm);
-            expect(elm[ViewModelReflection].idx).toBe(idx);
+            expect(hiddenFields.idx).toBeGreaterThan(0);
         });
 
         it('should assign bigger idx to children', () => {

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

@@ -34,7 +34,7 @@ import {
     getComponentAsString,
     getTemplateReactiveObserver,
 } from './component';
-import { setInternalField, setHiddenField } from '../shared/fields';
+import { setHiddenField } from '../shared/fields';
 import { ViewModelReflection, EmptyObject } from './utils';
 import { vmBeingConstructed, isBeingConstructed, isRendering, vmBeingRendered } from './invoker';
 import { getComponentVM, VM } from './vm';
@@ -282,8 +282,8 @@ function BaseLightningElementConstructor(this: LightningElement) {
     const cmpRoot = elm.attachShadow(shadowRootOptions);
     // linking elm, shadow root and component with the VM
     setHiddenField(component, ViewModelReflection, vm);
-    setInternalField(elm, ViewModelReflection, vm);
-    setInternalField(cmpRoot, ViewModelReflection, vm);
+    setHiddenField(cmpRoot, ViewModelReflection, vm);
+    setHiddenField(elm, ViewModelReflection, vm);
     // VM is now initialized
     (vm as VM).cmpRoot = cmpRoot;
     if (process.env.NODE_ENV !== 'production') {

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

@@ -27,7 +27,6 @@ import {
     isFunction,
     defineProperties,
 } from '../shared/language';
-import { getInternalField } from '../shared/fields';
 import { getAttrNameFromPropName } from './attributes';
 import {
     resolveCircularModuleDependency,
@@ -256,7 +255,7 @@ export function getComponentDef(Ctor: any, subclassComponentName?: string): Comp
 export function getComponentConstructor(elm: HTMLElement): ComponentConstructor | null {
     let ctor: ComponentConstructor | null = null;
     if (elm instanceof HTMLElement) {
-        const vm = getInternalField(elm, ViewModelReflection);
+        const vm = getHiddenField(elm, ViewModelReflection);
         if (!isUndefined(vm)) {
             ctor = vm.def.ctor;
         }
@@ -286,6 +285,7 @@ import {
     TrackDef,
 } from './decorators/register';
 import { defaultEmptyTemplate } from './secure-template';
+import { getHiddenField } from '../shared/fields';
 
 // Typescript is inferring the wrong function type for this particular
 // overloaded method: https://github.com/Microsoft/TypeScript/issues/27972

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

@@ -5,7 +5,7 @@
  * For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
  */
 import assert from '../shared/assert';
-import { isArray, isUndefined, isTrue, hasOwnProperty, isNull } from '../shared/language';
+import { isArray, isUndefined, isTrue, isNull } from '../shared/language';
 import { EmptyArray, ViewModelReflection, EmptyObject, useSyntheticShadow } from './utils';
 import {
     rerenderVM,
@@ -33,6 +33,7 @@ import {
     lockDomMutation,
 } from './restrictions';
 import { getComponentDef, setElementProto } from './def';
+import { getHiddenField } from '../shared/fields';
 
 const noop = () => void 0;
 
@@ -174,7 +175,7 @@ export function allocateChildrenHook(vnode: VCustomElement) {
 
 export function createViewModelHook(vnode: VCustomElement) {
     const elm = vnode.elm as HTMLElement;
-    if (hasOwnProperty.call(elm, ViewModelReflection)) {
+    if (!isUndefined(getHiddenField(elm, ViewModelReflection))) {
         // There is a possibility that a custom element is registered under tagName,
         // in which case, the initialization is already carry on, and there is nothing else
         // to do here since this hook is called right after invoking `document.createElement`.

packages/@lwc/engine/src/framework/modules/context.ts

@@ -7,7 +7,7 @@
 import { isUndefined, assign } from '../../shared/language';
 import { VNode } from '../../3rdparty/snabbdom/types';
 import { ViewModelReflection } from '../utils';
-import { getInternalField } from '../../shared/fields';
+import { getHiddenField } from '../../shared/fields';
 import { VM } from '../vm';
 
 function createContext(vnode: VNode) {
@@ -20,7 +20,7 @@ function createContext(vnode: VNode) {
     }
 
     const elm = vnode.elm as Element;
-    const vm: VM = getInternalField(elm, ViewModelReflection);
+    const vm: VM = getHiddenField(elm, ViewModelReflection);
 
     if (!isUndefined(vm)) {
         assign(vm.context, context);

packages/@lwc/engine/src/framework/modules/props.ts

@@ -6,7 +6,7 @@
  */
 import assert from '../../shared/assert';
 import { isUndefined, keys } from '../../shared/language';
-import { getInternalField } from '../../shared/fields';
+import { getHiddenField } from '../../shared/fields';
 import { ViewModelReflection } from '../utils';
 import { prepareForPropUpdate } from '../base-bridge-element';
 import { VNode } from '../../3rdparty/snabbdom/types';
@@ -36,7 +36,7 @@ function update(oldVnode: VNode, vnode: VNode) {
     }
 
     const elm = vnode.elm as Element;
-    const vm = getInternalField(elm, ViewModelReflection);
+    const vm = getHiddenField(elm, ViewModelReflection);
     const isFirstPatch = isUndefined(oldProps);
     const isCustomElement = !isUndefined(vm);
     const { sel } = vnode;

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

@@ -14,7 +14,7 @@ import {
     resolveCircularModuleDependency,
     ViewModelReflection,
 } from './utils';
-import { setInternalField, getInternalField, createFieldName } from '../shared/fields';
+import { setHiddenField, getHiddenField, createFieldName } from '../shared/fields';
 import { getComponentDef, setElementProto } from './def';
 import { patchCustomElementWithRestrictions } from './restrictions';
 import { GlobalMeasurementPhase, startGlobalMeasure, endGlobalMeasure } from './performance-timing';
@@ -27,7 +27,7 @@ function callNodeSlot(node: Node, slot: symbol): Node {
     if (process.env.NODE_ENV !== 'production') {
         assert.isTrue(node, `callNodeSlot() should not be called for a non-object`);
     }
-    const fn = getInternalField(node, slot);
+    const fn = getHiddenField(node, slot);
     if (!isUndefined(fn)) {
         fn();
     }
@@ -97,7 +97,7 @@ export function createElement(sel: string, options: CreateElementOptions): HTMLE
 
     // Create element with correct tagName
     const element = document.createElement(sel);
-    if (!isUndefined(getInternalField(element, ViewModelReflection))) {
+    if (!isUndefined(getHiddenField(element, ViewModelReflection))) {
         // There is a possibility that a custom element is registered under tagName,
         // in which case, the initialization is already carry on, and there is nothing else
         // to do here.
@@ -117,7 +117,7 @@ export function createElement(sel: string, options: CreateElementOptions): HTMLE
     // In case the element is not initialized already, we need to carry on the manual creation
     createVM(element, Ctor, { mode, isRoot: true, owner: null });
     // Handle insertion and removal from the DOM manually
-    setInternalField(element, ConnectingSlot, () => {
+    setHiddenField(element, ConnectingSlot, () => {
         const vm = getCustomElementVM(element);
         startGlobalMeasure(GlobalMeasurementPhase.HYDRATE, vm);
         if (vm.state === VMState.connected) {
@@ -127,7 +127,7 @@ export function createElement(sel: string, options: CreateElementOptions): HTMLE
         appendRootVM(vm);
         endGlobalMeasure(GlobalMeasurementPhase.HYDRATE, vm);
     });
-    setInternalField(element, DisconnectingSlot, () => {
+    setHiddenField(element, DisconnectingSlot, () => {
         const vm = getCustomElementVM(element);
         removeRootVM(vm);
     });

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

@@ -27,7 +27,7 @@ import {
     isFalse,
     isArray,
 } from '../shared/language';
-import { getInternalField, getHiddenField } from '../shared/fields';
+import { getHiddenField } from '../shared/fields';
 import {
     ViewModelReflection,
     addCallbackToNextTick,
@@ -526,7 +526,7 @@ function getErrorBoundaryVM(startingElement: Element | null): VM | undefined {
     let vm: VM | undefined;
 
     while (!isNull(elm)) {
-        vm = getInternalField(elm, ViewModelReflection);
+        vm = getHiddenField(elm, ViewModelReflection);
         if (!isUndefined(vm) && !isUndefined(vm.def.errorCallback)) {
             return vm;
         }
@@ -545,7 +545,7 @@ export function getErrorComponentStack(startingElement: Element): string {
     const wcStack: string[] = [];
     let elm: Element | null = startingElement;
     do {
-        const currentVm: VM | undefined = getInternalField(elm, ViewModelReflection);
+        const currentVm: VM | undefined = getHiddenField(elm, ViewModelReflection);
         if (!isUndefined(currentVm)) {
             const tagName = tagNameGetter.call(elm);
             const is = elm.getAttribute('is');
@@ -611,10 +611,10 @@ export function isNodeFromTemplate(node: Node): boolean {
 
 export function getCustomElementVM(elm: HTMLElement): VM {
     if (process.env.NODE_ENV !== 'production') {
-        const vm = getInternalField(elm, ViewModelReflection);
+        const vm = getHiddenField(elm, ViewModelReflection);
         assert.isTrue(vm && 'cmpRoot' in vm, `${vm} is not a vm.`);
     }
-    return getInternalField(elm, ViewModelReflection) as VM;
+    return getHiddenField(elm, ViewModelReflection) as VM;
 }
 
 export function getComponentVM(component: ComponentInterface): VM {
@@ -628,10 +628,10 @@ export function getComponentVM(component: ComponentInterface): VM {
 export function getShadowRootVM(root: ShadowRoot): VM {
     // TODO: #1299 - use a weak map instead of an internal field
     if (process.env.NODE_ENV !== 'production') {
-        const vm = getInternalField(root, ViewModelReflection);
+        const vm = getHiddenField(root, ViewModelReflection);
         assert.isTrue(vm && 'cmpRoot' in vm, `${vm} is not a vm.`);
     }
-    return getInternalField(root, ViewModelReflection) as VM;
+    return getHiddenField(root, ViewModelReflection) as VM;
 }
 
 // slow path routine

packages/@lwc/engine/src/shared/fields.ts

@@ -4,7 +4,7 @@
  * SPDX-License-Identifier: MIT
  * For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
  */
-import { defineProperty, create, isUndefined } from './language';
+import { create, isUndefined } from './language';
 
 /**
  * In IE11, symbols are expensive.
@@ -16,48 +16,23 @@ const hasNativeSymbolsSupport = Symbol('x').toString() === 'Symbol(x)';
 
 export function createFieldName(key: string): symbol {
     // @ts-ignore: using a string as a symbol for perf reasons
-    return hasNativeSymbolsSupport ? Symbol(key) : `$$lwc-${key}$$`;
+    return hasNativeSymbolsSupport ? Symbol(key) : `$$lwc-engine-${key}$$`;
 }
 
-export function setInternalField(o: object, fieldName: symbol, value: any) {
-    // TODO: #1299 - use a weak map instead
-    defineProperty(o, fieldName, {
-        value,
-    });
-}
+const hiddenFieldsMap: WeakMap<any, Record<symbol, any>> = new WeakMap();
 
-export function getInternalField(o: object, fieldName: symbol): any {
-    return o[fieldName];
+export function setHiddenField(o: any, fieldName: symbol, value: any) {
+    let valuesByField = hiddenFieldsMap.get(o);
+    if (isUndefined(valuesByField)) {
+        valuesByField = create(null) as (Record<symbol, any>);
+        hiddenFieldsMap.set(o, valuesByField);
+    }
+    valuesByField[fieldName] = value;
 }
 
-/**
- * Store fields that should be hidden from outside world
- * hiddenFieldsMap is a WeakMap.
- * It stores a hash of any given objects associative relationships.
- * The hash uses the fieldName as the key, the value represents the other end of the association.
- *
- * For example, if the association is
- *              ViewModel
- * Component-A --------------> VM-1
- * then,
- * hiddenFieldsMap : (Component-A, { Symbol(ViewModel) : VM-1 })
- *
- */
-const hiddenFieldsMap: WeakMap<any, Record<symbol, any>> = new WeakMap();
-export const setHiddenField = hasNativeSymbolsSupport
-    ? (o: any, fieldName: symbol, value: any): void => {
-          let valuesByField = hiddenFieldsMap.get(o);
-          if (isUndefined(valuesByField)) {
-              valuesByField = create(null) as (Record<symbol, any>);
-              hiddenFieldsMap.set(o, valuesByField);
-          }
-          valuesByField[fieldName] = value;
-      }
-    : setInternalField; // Fall back to symbol based approach in compat mode
-
-export const getHiddenField = hasNativeSymbolsSupport
-    ? (o: any, fieldName: symbol): any => {
-          const valuesByField = hiddenFieldsMap.get(o);
-          return !isUndefined(valuesByField) && valuesByField[fieldName];
-      }
-    : getInternalField; // Fall back to symbol based approach in compat mode
+export function getHiddenField(o: any, fieldName: symbol) {
+    const valuesByField = hiddenFieldsMap.get(o);
+    if (!isUndefined(valuesByField)) {
+        return valuesByField[fieldName];
+    }
+}

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

@@ -5,7 +5,7 @@
  * For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
  */
 import assert from '../shared/assert';
-import { createFieldName, getInternalField, setInternalField } from '../shared/fields';
+import { createFieldName, getHiddenField, setHiddenField } from '../shared/fields';
 import { windowAddEventListener, windowRemoveEventListener } from '../env/window';
 import {
     matches,
@@ -320,8 +320,8 @@ export function ignoreFocus(elm: HTMLElement) {
 
 function bindDocumentMousedownMouseupHandlers(elm: Node) {
     const ownerDocument = getOwnerDocument(elm);
-    if (!getInternalField(ownerDocument, DidAddMouseDownListener)) {
-        setInternalField(ownerDocument, DidAddMouseDownListener, true);
+    if (!getHiddenField(ownerDocument, DidAddMouseDownListener)) {
+        setHiddenField(ownerDocument, DidAddMouseDownListener, true);
         addEventListener.call(
             ownerDocument,
             'mousedown',

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

@@ -23,7 +23,7 @@ import {
     isNodeOwnedBy,
     isSlotElement,
 } from './traverse';
-import { getInternalField, setInternalField, createFieldName } from '../shared/fields';
+import { getHiddenField, setHiddenField, createFieldName } from '../shared/fields';
 import { getTextContent } from '../3rdparty/polymer/text-content';
 import { createStaticNodeList } from '../shared/static-node-list';
 import { DocumentPrototypeActiveElement, elementFromPoint, createComment } from '../env/document';
@@ -59,7 +59,7 @@ interface ShadowRootRecord {
 }
 
 function getInternalSlot(root: SyntheticShadowRootInterface | Element): ShadowRootRecord {
-    const record: ShadowRootRecord | undefined = getInternalField(root, InternalSlot);
+    const record: ShadowRootRecord | undefined = getHiddenField(root, InternalSlot);
     if (isUndefined(record)) {
         throw new TypeError();
     }
@@ -109,13 +109,13 @@ export function getShadowRoot(elm: Element): SyntheticShadowRootInterface {
 // since this check is harmless for nodes as well, and it speeds up things
 // to avoid casting before calling this method in few places.
 export function isHostElement(elm: Element | Node): boolean {
-    return !isUndefined(getInternalField(elm, InternalSlot));
+    return !isUndefined(getHiddenField(elm, InternalSlot));
 }
 
 let uid = 0;
 
 export function attachShadow(elm: Element, options: ShadowRootInit): SyntheticShadowRootInterface {
-    if (!isUndefined(getInternalField(elm, InternalSlot))) {
+    if (!isUndefined(getHiddenField(elm, InternalSlot))) {
         throw new Error(
             `Failed to execute 'attachShadow' on 'Element': Shadow root cannot be created on a host which already hosts a shadow tree.`
         );
@@ -131,8 +131,8 @@ export function attachShadow(elm: Element, options: ShadowRootInit): SyntheticSh
         host: elm,
         shadowRoot: sr,
     };
-    setInternalField(sr, InternalSlot, record);
-    setInternalField(elm, InternalSlot, record);
+    setHiddenField(sr, InternalSlot, record);
+    setHiddenField(elm, InternalSlot, record);
     const shadowResolver = () => sr;
     const x = (shadowResolver.nodeKey = uid++);
     setNodeKey(elm, x);