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.

Sign up for GitHub

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
Closed

refactor: hidden fields instead of internal fields #1478

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2d75d7d
chore: package-unique keys for engine and synthetic shadow
ekashida Aug 27, 2019
f2adc93
refactor: remove internal fields in favor of hidden fields
ekashida Aug 27, 2019
e52f953
fix: avoid returning boolean false when field undefined
ekashida Aug 27, 2019
0a80473
test: use hidden fields instead of internal fields
ekashida Aug 27, 2019
861a4c4
refactor(synthetic-shadow): hidden fields instead of internal fields
ekashida Aug 27, 2019
2968a25
chore(synthetic-shadow): "unique" string keys
ekashida Aug 28, 2019
e90b104
chore: revert changes for vm access by getComponentConstructor
ekashida Aug 29, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions packages/@lwc/engine/src/framework/__tests__/vm.spec.ts
View file
Open in desktop
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
5 changes: 3 additions & 2 deletions packages/@lwc/engine/src/framework/base-lightning-element.ts
View file
Open in desktop
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, setInternalField } from '../shared/fields';
Copy link
Collaborator

Choose a reason for hiding this comment

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

where is getInternalField used? I thought the idea was to remove that entirely.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I mentioned above that it was in vm.ts but it was actually in def.ts:

/**
 * EXPERIMENTAL: This function provides access to the component constructor,
 * given an HTMLElement. This API is subject to change or being removed.
 */
export function getComponentConstructor(elm: HTMLElement): ComponentConstructor | null {
    let ctor: ComponentConstructor | null = null;
    if (elm instanceof HTMLElement) {
        const vm = getInternalField(elm, ViewModelReflection);
        if (!isUndefined(vm)) {
            ctor = vm.def.ctor;
        }
    }
    return ctor;
}

I'm not sure why, but our perf tests blow up when we access the vm using getHiddenField here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, let me look at it.

import { ViewModelReflection, EmptyObject } from './utils';
import { vmBeingConstructed, isBeingConstructed, isRendering, vmBeingRendered } from './invoker';
import { getComponentVM, VM } from './vm';
Expand Down Expand Up @@ -282,8 +282,9 @@ function BaseLightningElementConstructor(this: LightningElement) {
const cmpRoot = elm.attachShadow(shadowRootOptions);
// linking elm, shadow root and component with the VM
setHiddenField(component, ViewModelReflection, vm);
setHiddenField(cmpRoot, ViewModelReflection, vm);
setHiddenField(elm, ViewModelReflection, vm);
setInternalField(elm, ViewModelReflection, vm);
setInternalField(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/modules/context.ts
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
56 changes: 20 additions & 36 deletions packages/@lwc/engine/src/shared/fields.ts
View file
Open in desktop
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, defineProperty, isUndefined } from './language';

/**
* In IE11, symbols are expensive.
Expand All @@ -16,48 +16,32 @@ 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}$$`;
}

const hiddenFieldsMap: WeakMap<any, Record<symbol, any>> = new WeakMap();

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;
}

export function getHiddenField(o: any, fieldName: symbol) {
const valuesByField = hiddenFieldsMap.get(o);
if (!isUndefined(valuesByField)) {
return valuesByField[fieldName];
}
}

export function setInternalField(o: object, fieldName: symbol, value: any) {
// TODO: #1299 - use a weak map instead
defineProperty(o, fieldName, {
value,
});
}

export function getInternalField(o: object, fieldName: symbol): any {
return o[fieldName];
}

/**
* 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
6 changes: 3 additions & 3 deletions packages/@lwc/synthetic-shadow/src/faux-shadow/focus.ts
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
25 changes: 16 additions & 9 deletions packages/@lwc/synthetic-shadow/src/shared/fields.ts
View file
Open in desktop
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 } from './language';
import { create, isUndefined } from './language';
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated to this pr: The fields util is replicated in 3 submodules(engine, synthetic-shadow, node-reactions). Is it a good time to pull it out as a shared library or submodule?
cc @caridy

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, we have an issue for that


/**
* In IE11, symbols are expensive.
Expand All @@ -16,16 +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-synthetic-shadow-${key}$$`;
}

export function setInternalField(o: object, fieldName: symbol, value: any) {
// TODO: #1299 - improve this to use a WeakMap
defineProperty(o, fieldName, {
value,
});
const hiddenFieldsMap: WeakMap<any, Record<symbol, any>> = new WeakMap();

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;
}

export function getInternalField(o: object, fieldName: symbol): any {
return o[fieldName];
export function getHiddenField(o: any, fieldName: symbol) {
const valuesByField = hiddenFieldsMap.get(o);
if (!isUndefined(valuesByField)) {
return valuesByField[fieldName];
}
}
Loading