From 0fad9abfcda462b669c59324499dbdc6bef837e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Caridy=20Pati=C3=B1o?= Date: Fri, 20 Jul 2018 11:20:36 -0400 Subject: [PATCH] fix(engine): closes #524 relax attribute mutation assertion (#525) * fix(engine): closes #524 relax attribute mutation assertion --- .../framework/__tests__/html-element.spec.ts | 25 +++++++-- .../lwc-engine/src/framework/html-element.ts | 52 ++++++++++++++----- .../lwc-engine/src/framework/modules/attrs.ts | 7 ++- .../lwc-engine/src/framework/restrictions.ts | 51 ++++++++---------- 4 files changed, 89 insertions(+), 46 deletions(-) diff --git a/packages/lwc-engine/src/framework/__tests__/html-element.spec.ts b/packages/lwc-engine/src/framework/__tests__/html-element.spec.ts index c60625f197..8cf684dc7c 100644 --- a/packages/lwc-engine/src/framework/__tests__/html-element.spec.ts +++ b/packages/lwc-engine/src/framework/__tests__/html-element.spec.ts @@ -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( () => { @@ -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 }); diff --git a/packages/lwc-engine/src/framework/html-element.ts b/packages/lwc-engine/src/framework/html-element.ts index 415744bc4f..b7ca16be2c 100644 --- a/packages/lwc-engine/src/framework/html-element.ts +++ b/packages/lwc-engine/src/framework/html-element.ts @@ -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 @@ -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); diff --git a/packages/lwc-engine/src/framework/modules/attrs.ts b/packages/lwc-engine/src/framework/modules/attrs.ts index f3965e23c0..f2e0a775f5 100644 --- a/packages/lwc-engine/src/framework/modules/attrs.ts +++ b/packages/lwc-engine/src/framework/modules/attrs.ts @@ -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"; @@ -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 @@ -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); + } } } } diff --git a/packages/lwc-engine/src/framework/restrictions.ts b/packages/lwc-engine/src/framework/restrictions.ts index ed1380c234..79f71439cc 100644 --- a/packages/lwc-engine/src/framework/restrictions.ts +++ b/packages/lwc-engine/src/framework/restrictions.ts @@ -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, @@ -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)); @@ -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)); } @@ -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)); } @@ -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)); } @@ -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 {