Skip to content

Commit

Permalink
fix(engine): closes #524 relax attribute mutation assertion (#525)
Browse files Browse the repository at this point in the history 
* fix(engine): closes #524 relax attribute mutation assertion
  • Loading branch information
@caridy
caridy authored Jul 20, 2018
1 parent 4049e9e commit 0fad9ab
Show file tree
Hide file tree
Showing 4 changed files with 89 additions and 46 deletions.
25 changes: 22 additions & 3 deletions packages/lwc-engine/src/framework/__tests__/html-element.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -672,9 +672,21 @@ describe('html-element', () => {

it('should log error message when attribute is set via elm.setAttribute if reflective property is defined', () => {
jest.spyOn(assertLogger, 'logError');
class MyComponent extends LightningElement {}
const elm = createElement('x-foo', {is: MyComponent});
elm.setAttribute('tabindex', '0');
class Child extends LightningElement {}
function html($api, $cmp) {
return [
$api.c('x-child', Child, { attrs: { title: 'child title' }})
];
}
class Parent extends LightningElement {
render() {
return html;
}
renderedCallback() {
this.template.querySelector('x-child').setAttribute('tabindex', 0);
}
}
const elm = createElement('x-foo', {is: Parent});
document.body.appendChild(elm);

return Promise.resolve().then( () => {
Expand All @@ -683,6 +695,13 @@ describe('html-element', () => {
});
});

it('should not throw when accessing attribute in root elements', () => {
class Parent extends LightningElement {}
const elm = createElement('x-foo', {is: Parent});
document.body.appendChild(elm);
elm.setAttribute('tabindex', 1);
});

it('should delete existing attribute prior rendering', () => {
const def = class MyComponent extends LightningElement {};
const elm = createElement('x-foo', { is: def });
Expand Down
52 changes: 39 additions & 13 deletions packages/lwc-engine/src/framework/html-element.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { observeMutation, notifyMutation } from "./watcher";
import { dispatchEvent, BaseCustomElementProto, elementTagNameGetter } from "./dom-api";
import { patchComponentWithRestrictions, patchCustomElementWithRestrictions, patchShadowRootWithRestrictions } from "./restrictions";
import { lightDomQuerySelectorAll, lightDomQuerySelector } from "../faux-shadow/faux";
import { prepareForValidAttributeMutation } from "./restrictions";
import { unlockAttribute, lockAttribute } from "./restrictions";

const GlobalEvent = Event; // caching global reference to avoid poisoning

Expand Down Expand Up @@ -178,43 +178,69 @@ LightningElement.prototype = {
const wrappedListener = getWrappedComponentsListener(vm, listener);
vm.elm.removeEventListener(type, wrappedListener, options);
},
setAttributeNS(ns: string, attrName: string, value: any): void {
setAttributeNS(ns: string, attrName: string, value: any) {
const elm = getLinkedElement(this);
if (process.env.NODE_ENV !== 'production') {
assert.isFalse(isBeingConstructed(getComponentVM(this)), `Failed to construct '${this}': The result must not have attributes.`);
prepareForValidAttributeMutation(elm, attrName);
unlockAttribute(elm, attrName);
}
elm.setAttributeNS.apply(elm, arguments);
if (process.env.NODE_ENV !== 'production') {
lockAttribute(elm, attrName);
}
return elm.setAttributeNS.apply(elm, arguments);
},
removeAttributeNS(ns: string, attrName: string): void {
removeAttributeNS(ns: string, attrName: string) {
const elm = getLinkedElement(this);
if (process.env.NODE_ENV !== 'production') {
prepareForValidAttributeMutation(elm, attrName);
unlockAttribute(elm, attrName);
}
elm.removeAttributeNS.apply(elm, arguments);
if (process.env.NODE_ENV !== 'production') {
lockAttribute(elm, attrName);
}
return elm.removeAttributeNS.apply(elm, arguments);
},
removeAttribute(attrName: string) {
const elm = getLinkedElement(this);
if (process.env.NODE_ENV !== 'production') {
prepareForValidAttributeMutation(elm, attrName);
unlockAttribute(elm, attrName);
}
elm.removeAttribute.apply(elm, arguments);
if (process.env.NODE_ENV !== 'production') {
lockAttribute(elm, attrName);
}
},
setAttribute(attrName: string, value: any): void {
setAttribute(attrName: string, value: any) {
const elm = getLinkedElement(this);
if (process.env.NODE_ENV !== 'production') {
assert.isFalse(isBeingConstructed(getComponentVM(this)), `Failed to construct '${this}': The result must not have attributes.`);
prepareForValidAttributeMutation(elm, attrName);
unlockAttribute(elm, attrName);
}
elm.setAttribute.apply(elm, arguments);
if (process.env.NODE_ENV !== 'production') {
lockAttribute(elm, attrName);
}
return elm.setAttribute.apply(elm, arguments);
},
getAttribute(attrName: string): string | null {
const elm = getLinkedElement(this);
return elm.getAttribute.apply(elm, arguments);
if (process.env.NODE_ENV !== 'production') {
unlockAttribute(elm, attrName);
}
const value = elm.getAttribute.apply(elm, arguments);
if (process.env.NODE_ENV !== 'production') {
lockAttribute(elm, attrName);
}
return value;
},
getAttributeNS(ns: string, attrName: string) {
const elm = getLinkedElement(this);
return elm.getAttributeNS.apply(elm, arguments);
if (process.env.NODE_ENV !== 'production') {
unlockAttribute(elm, attrName);
}
const value = elm.getAttributeNS.apply(elm, arguments);
if (process.env.NODE_ENV !== 'production') {
lockAttribute(elm, attrName);
}
return value;
},
getBoundingClientRect(): ClientRect {
const elm = getLinkedElement(this);
Expand Down
7 changes: 5 additions & 2 deletions packages/lwc-engine/src/framework/modules/attrs.ts
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import assert from "../../shared/assert";
import { prepareForValidAttributeMutation } from '../restrictions';
import { unlockAttribute, lockAttribute } from '../restrictions';
import { isUndefined, keys, StringCharCodeAt, isNull } from '../../shared/language';
import { EmptyObject } from '../utils';
import { Module, VNode } from "../../3rdparty/snabbdom/types";
Expand Down Expand Up @@ -35,7 +35,7 @@ function updateAttrs(oldVnode: VNode, vnode: VNode) {
const old = (oldAttrs as any)[key];
if (old !== cur) {
if (process.env.NODE_ENV !== 'production') {
prepareForValidAttributeMutation(elm, key);
unlockAttribute(elm, key);
}
if (StringCharCodeAt.call(key, 3) === ColonCharCode) {
// Assume xml namespace
Expand All @@ -48,6 +48,9 @@ function updateAttrs(oldVnode: VNode, vnode: VNode) {
} else {
elm.setAttribute(key, cur as string);
}
if (process.env.NODE_ENV !== 'production') {
lockAttribute(elm, key);
}
}
}
}
Expand Down
51 changes: 23 additions & 28 deletions packages/lwc-engine/src/framework/restrictions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { getPropertyDescriptor, defineProperties, getOwnPropertyNames, forEach,
import { ComponentInterface } from "./component";
import { getGlobalHTMLPropertiesInfo, getPropNameFromAttrName } from "./attributes";
import { isBeingConstructed, isRendering, vmBeingRendered } from "./invoker";
import { getShadowRootVM, getNodeKey, getCustomElementVM, VM, getNodeOwnerKey } from "./vm";
import { getShadowRootVM, getCustomElementVM, VM, getNodeOwnerKey } from "./vm";
import {
getAttribute,
setAttribute,
Expand Down Expand Up @@ -108,7 +108,7 @@ function getShadowRootRestrictionsDescriptors(sr: ShadowRoot): PropertyDescripto
function getAttributePatched(this: HTMLElement, attrName: string): string | null {
if (process.env.NODE_ENV !== 'production') {
const vm = getCustomElementVM(this);
assertPublicAttributeCollision(vm, attrName);
assertAttributeReflectionCapability(vm, attrName);
}

return getAttribute.apply(this, ArraySlice.call(arguments));
Expand All @@ -117,8 +117,8 @@ function getAttributePatched(this: HTMLElement, attrName: string): string | null
function setAttributePatched(this: HTMLElement, attrName: string, newValue: any) {
const vm = getCustomElementVM(this);
if (process.env.NODE_ENV !== 'production') {
assertTemplateMutationViolation(vm, attrName);
assertPublicAttributeCollision(vm, attrName);
assertAttributeMutationCapability(vm, attrName);
assertAttributeReflectionCapability(vm, attrName);
}
setAttribute.apply(this, ArraySlice.call(arguments));
}
Expand All @@ -127,8 +127,8 @@ function setAttributeNSPatched(this: HTMLElement, attrNameSpace: string, attrNam
const vm = getCustomElementVM(this);

if (process.env.NODE_ENV !== 'production') {
assertTemplateMutationViolation(vm, attrName);
assertPublicAttributeCollision(vm, attrName);
assertAttributeMutationCapability(vm, attrName);
assertAttributeReflectionCapability(vm, attrName);
}
setAttributeNS.apply(this, ArraySlice.call(arguments));
}
Expand All @@ -137,8 +137,8 @@ function removeAttributePatched(this: HTMLElement, attrName: string) {
const vm = getCustomElementVM(this);
// marking the set is needed for the AOM polyfill
if (process.env.NODE_ENV !== 'production') {
assertTemplateMutationViolation(vm, attrName);
assertPublicAttributeCollision(vm, attrName);
assertAttributeMutationCapability(vm, attrName);
assertAttributeReflectionCapability(vm, attrName);
}
removeAttribute.apply(this, ArraySlice.call(arguments));
}
Expand All @@ -147,68 +147,63 @@ function removeAttributeNSPatched(this: HTMLElement, attrNameSpace: string, attr
const vm = getCustomElementVM(this);

if (process.env.NODE_ENV !== 'production') {
assertTemplateMutationViolation(vm, attrName);
assertPublicAttributeCollision(vm, attrName);
assertAttributeMutationCapability(vm, attrName);
assertAttributeReflectionCapability(vm, attrName);
}
removeAttributeNS.apply(this, ArraySlice.call(arguments));
}

function assertPublicAttributeCollision(vm: VM, attrName: string) {
function assertAttributeReflectionCapability(vm: VM, attrName: string) {
if (process.env.NODE_ENV === 'production') {
// this method should never leak to prod
throw new ReferenceError();
}
const propName = isString(attrName) ? getPropNameFromAttrName(StringToLowerCase.call(attrName)) : null;
const { def: { props: propsConfig } } = vm;
const { elm, def: { props: propsConfig } } = vm;

if (propsConfig && propName && propsConfig[propName]) {
if (!isUndefined(getNodeOwnerKey(elm)) && isAttributeLocked(elm, attrName) && propsConfig && propName && propsConfig[propName]) {
assert.logError(`Invalid attribute "${StringToLowerCase.call(attrName)}" for ${vm}. Instead access the public property with \`element.${propName};\`.`);
}
}

function assertTemplateMutationViolation(vm: VM, attrName: string) {
function assertAttributeMutationCapability(vm: VM, attrName: string) {
if (process.env.NODE_ENV === 'production') {
// this method should never leak to prod
throw new ReferenceError();
}
const { elm } = vm;
if (!isAttributeChangeControlled(attrName) && !isUndefined(getNodeOwnerKey(elm))) {
if (!isUndefined(getNodeOwnerKey(elm)) && isAttributeLocked(elm, attrName)) {
assert.logError(`Invalid operation on Element ${vm}. Elements created via a template should not be mutated using DOM APIs. Instead of attempting to update this element directly to change the value of attribute "${attrName}", you can update the state of the component, and let the engine to rehydrate the element accordingly.`);
}
// attribute change control must be released every time its value is checked
resetAttributeChangeControl();
}

let controlledAttributeChange: boolean = false;
let controlledElement: Element | null = null;
let controlledAttributeName: string | void;

function isAttributeChangeControlled(attrName: string): boolean {
function isAttributeLocked(elm: Element, attrName: string): boolean {
if (process.env.NODE_ENV === 'production') {
// this method should never leak to prod
throw new ReferenceError();
}
return controlledAttributeChange && attrName === controlledAttributeName;
return elm !== controlledElement || attrName !== controlledAttributeName;
}

function resetAttributeChangeControl() {
export function lockAttribute(elm: Element, key: string) {
if (process.env.NODE_ENV === 'production') {
// this method should never leak to prod
throw new ReferenceError();
}
controlledAttributeChange = false;
controlledElement = null;
controlledAttributeName = undefined;
}

export function prepareForValidAttributeMutation(elm: Element, key: string) {
export function unlockAttribute(elm: Element, key: string) {
if (process.env.NODE_ENV === 'production') {
// this method should never leak to prod
throw new ReferenceError();
}
if (!isUndefined(getNodeKey(elm))) {
// TODO: we should guarantee that the methods of the element are all patched
controlledAttributeChange = true;
controlledAttributeName = key;
}
controlledElement = elm;
controlledAttributeName = key;
}

function getCustomElementRestrictionsDescriptors(elm: HTMLElement): PropertyDescriptorMap {
Expand Down

0 comments on commit 0fad9ab

Please sign in to comment.