Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: hidden fields instead of internal fields #1478

Closed
wants to merge 7 commits into from
13 changes: 8 additions & 5 deletions packages/@lwc/engine/src/framework/__tests__/vm.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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>`);

Expand All @@ -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', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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(elm, ViewModelReflection, vm);
setHiddenField(cmpRoot, ViewModelReflection, vm);
// VM is now initialized
(vm as VM).cmpRoot = cmpRoot;
if (process.env.NODE_ENV !== 'production') {
Expand Down
4 changes: 2 additions & 2 deletions packages/@lwc/engine/src/framework/def.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {
isFunction,
defineProperties,
} from '../shared/language';
import { getInternalField } from '../shared/fields';
import { getHiddenField } from '../shared/fields';
import { getAttrNameFromPropName } from './attributes';
import {
resolveCircularModuleDependency,
Expand Down Expand Up @@ -256,7 +256,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;
}
Expand Down
4 changes: 2 additions & 2 deletions packages/@lwc/engine/src/framework/modules/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions packages/@lwc/engine/src/framework/modules/props.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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;
Expand Down
10 changes: 5 additions & 5 deletions packages/@lwc/engine/src/framework/upgrade.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand All @@ -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();
}
Expand Down Expand Up @@ -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.
Expand All @@ -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) {
Expand All @@ -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);
});
Expand Down
14 changes: 7 additions & 7 deletions packages/@lwc/engine/src/framework/vm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {
isFalse,
isArray,
} from '../shared/language';
import { getInternalField, getHiddenField } from '../shared/fields';
import { getHiddenField } from '../shared/fields';
import {
ViewModelReflection,
addCallbackToNextTick,
Expand Down Expand Up @@ -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;
}
Expand All @@ -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');
Expand Down Expand Up @@ -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 {
Expand All @@ -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
Expand Down
57 changes: 16 additions & 41 deletions packages/@lwc/engine/src/shared/fields.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ekashida #860 (comment)
To give your some context as to why we were falling back to symbol based approach in compat mode

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I didn't get the existing comment because it was actually falling back to the internal field approach (which was also based on symbols), which would fall back to the string-based approach.


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];
}
}
6 changes: 3 additions & 3 deletions packages/@lwc/synthetic-shadow/src/faux-shadow/focus.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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',
Expand Down
12 changes: 6 additions & 6 deletions packages/@lwc/synthetic-shadow/src/faux-shadow/shadow-root.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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();
}
Expand Down Expand Up @@ -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.`
);
Expand All @@ -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);
Expand Down
6 changes: 3 additions & 3 deletions packages/@lwc/synthetic-shadow/src/faux-shadow/slot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
*/
import assert from '../shared/assert';
import { getAttribute, childrenGetter, setAttribute } from '../env/element';
import { createFieldName, getInternalField, setInternalField } from '../shared/fields';
import { createFieldName, getHiddenField, setHiddenField } from '../shared/fields';
import { dispatchEvent } from '../env/dom';
import {
ArrayIndexOf,
Expand Down Expand Up @@ -113,9 +113,9 @@ defineProperties(HTMLSlotElement.prototype, {
if (
isNodeShadowed(this) &&
type === 'slotchange' &&
!getInternalField(this, SlotChangeKey)
!getHiddenField(this, SlotChangeKey)
) {
setInternalField(this, SlotChangeKey, true);
setHiddenField(this, SlotChangeKey, true);
if (!observer) {
observer = initSlotObserver();
}
Expand Down
Loading