From 4b37d7fa2118f70cd83f64671e6b0a0785bb65c8 Mon Sep 17 00:00:00 2001 From: Allan Oricil <55927613+AllanOricil@users.noreply.github.com> Date: Fri, 22 Sep 2023 20:53:16 -0300 Subject: [PATCH] feat: add dev-only warning to props that are not decorated with @api (#3713) Fixes #3641 Co-authored-by: Nolan Lawson --- .../src/framework/base-bridge-element.ts | 68 +++++++++++++++-- .../@lwc/engine-core/src/framework/def.ts | 8 +- .../index.spec.js | 4 +- .../test/api/getComponentDef/index.spec.js | 75 ++++++++++++++++++- .../x/htmlElementProps/htmlElementProps.js | 11 +++ .../x/privateAccessors/privateAccessors.js | 27 +++++++ .../component/decorators/api/index.spec.js | 8 +- .../GlobalHTML.properties.spec.js | 12 +-- packages/@lwc/shared/src/language.ts | 2 + 9 files changed, 197 insertions(+), 18 deletions(-) create mode 100644 packages/@lwc/integration-karma/test/api/getComponentDef/x/htmlElementProps/htmlElementProps.js create mode 100644 packages/@lwc/integration-karma/test/api/getComponentDef/x/privateAccessors/privateAccessors.js diff --git a/packages/@lwc/engine-core/src/framework/base-bridge-element.ts b/packages/@lwc/engine-core/src/framework/base-bridge-element.ts index c08333c16c..4ceea7300f 100644 --- a/packages/@lwc/engine-core/src/framework/base-bridge-element.ts +++ b/packages/@lwc/engine-core/src/framework/base-bridge-element.ts @@ -10,22 +10,26 @@ */ import { ArraySlice, + ArrayIndexOf, create, defineProperties, defineProperty, freeze, getOwnPropertyNames, + getOwnPropertyDescriptors, isUndefined, seal, keys, htmlPropertyToAttribute, + isNull, } from '@lwc/shared'; import { applyAriaReflection } from '@lwc/aria-reflection'; -import { logError } from '../shared/logger'; +import { logError, logWarn } from '../shared/logger'; import { getAssociatedVM } from './vm'; import { getReadOnlyProxy } from './membrane'; -import { HTMLElementConstructor } from './html-element'; +import { HTMLElementConstructor, HTMLElementPrototype } from './html-element'; import { HTMLElementOriginalDescriptors } from './html-properties'; +import { LightningElement } from './base-lightning-element'; // A bridge descriptor is a descriptor whose job is just to get the component instance // from the element instance, and get the value or set a new value on the component. @@ -104,6 +108,26 @@ function createAttributeChangedCallback( }; } +function createAccessorThatWarns(propName: string) { + let prop: any; + return { + get() { + logWarn( + `The property "${propName}" is not publicly accessible. Add the @api annotation to the property declaration or getter/setter in the component to make it accessible.` + ); + return prop; + }, + set(value: any) { + logWarn( + `The property "${propName}" is not publicly accessible. Add the @api annotation to the property declaration or getter/setter in the component to make it accessible.` + ); + prop = value; + }, + enumerable: true, + configurable: true, + }; +} + export interface HTMLElementConstructor { prototype: HTMLElement; new (): HTMLElement; @@ -111,8 +135,10 @@ export interface HTMLElementConstructor { export function HTMLBridgeElementFactory( SuperClass: HTMLElementConstructor, - props: string[], - methods: string[] + publicProperties: string[], + methods: string[], + observedFields: string[], + proto: LightningElement | null ): HTMLElementConstructor { const HTMLBridgeElement = class extends SuperClass {}; // generating the hash table for attributes to avoid duplicate fields and facilitate validation @@ -121,9 +147,35 @@ export function HTMLBridgeElementFactory( const { attributeChangedCallback: superAttributeChangedCallback } = SuperClass.prototype as any; const { observedAttributes: superObservedAttributes = [] } = SuperClass as any; const descriptors: PropertyDescriptorMap = create(null); + + // present a hint message so that developers are aware that they have not decorated property with @api + if (process.env.NODE_ENV !== 'production') { + if (!isUndefined(proto) && !isNull(proto)) { + const nonPublicPropertiesToWarnOn = new Set( + [ + // getters, setters, and methods + ...keys(getOwnPropertyDescriptors(proto)), + // class properties + ...observedFields, + ] + // we don't want to override HTMLElement props because these are meaningful in other ways, + // and can break tooling that expects it to be iterable or defined, e.g. Jest: + // https://github.com/jestjs/jest/blob/b4c9587/packages/pretty-format/src/plugins/DOMElement.ts#L95 + // It also doesn't make sense to override e.g. "constructor". + .filter((propName) => !(propName in HTMLElementPrototype)) + ); + + for (const propName of nonPublicPropertiesToWarnOn) { + if (ArrayIndexOf.call(publicProperties, propName) === -1) { + descriptors[propName] = createAccessorThatWarns(propName); + } + } + } + } + // expose getters and setters for each public props on the new Element Bridge - for (let i = 0, len = props.length; i < len; i += 1) { - const propName = props[i]; + for (let i = 0, len = publicProperties.length; i < len; i += 1) { + const propName = publicProperties[i]; attributeToPropMap[htmlPropertyToAttribute(propName)] = propName; descriptors[propName] = { get: createGetter(propName), @@ -175,7 +227,9 @@ export function HTMLBridgeElementFactory( export const BaseBridgeElement = HTMLBridgeElementFactory( HTMLElementConstructor, getOwnPropertyNames(HTMLElementOriginalDescriptors), - [] + [], + [], + null ); if (process.env.IS_BROWSER) { diff --git a/packages/@lwc/engine-core/src/framework/def.ts b/packages/@lwc/engine-core/src/framework/def.ts index dd35023ee7..9f19dc72e2 100644 --- a/packages/@lwc/engine-core/src/framework/def.ts +++ b/packages/@lwc/engine-core/src/framework/def.ts @@ -142,7 +142,13 @@ function createComponentDef(Ctor: LightningElementConstructor): ComponentDef { const superProto = getCtorProto(Ctor); const superDef = superProto !== LightningElement ? getComponentInternalDef(superProto) : lightingElementDef; - const bridge = HTMLBridgeElementFactory(superDef.bridge, keys(apiFields), keys(apiMethods)); + const bridge = HTMLBridgeElementFactory( + superDef.bridge, + keys(apiFields), + keys(apiMethods), + keys(observedFields), + proto + ); const props: PropertyDescriptorMap = assign(create(null), superDef.props, apiFields); const propsConfig = assign(create(null), superDef.propsConfig, apiFieldsConfig); const methods: PropertyDescriptorMap = assign(create(null), superDef.methods, apiMethods); diff --git a/packages/@lwc/integration-karma/test/api/CustomElementConstructor-getter/index.spec.js b/packages/@lwc/integration-karma/test/api/CustomElementConstructor-getter/index.spec.js index b011731ea7..d2f17cb51f 100644 --- a/packages/@lwc/integration-karma/test/api/CustomElementConstructor-getter/index.spec.js +++ b/packages/@lwc/integration-karma/test/api/CustomElementConstructor-getter/index.spec.js @@ -318,7 +318,9 @@ describe('attribute reflection', () => { // for props, only @api props are set expect(elm.observed).toBeUndefined(); expect(elm.api).toBe('foo'); - expect(elm.track).toBeUndefined(); + expect(() => { + expect(elm.track).toBeUndefined(); + }).toLogWarningDev(/Add the @api annotation to the property declaration/); }); it('does not call setter more than once if unchanged', () => { diff --git a/packages/@lwc/integration-karma/test/api/getComponentDef/index.spec.js b/packages/@lwc/integration-karma/test/api/getComponentDef/index.spec.js index 122aa817dc..71a793f551 100644 --- a/packages/@lwc/integration-karma/test/api/getComponentDef/index.spec.js +++ b/packages/@lwc/integration-karma/test/api/getComponentDef/index.spec.js @@ -1,5 +1,5 @@ /* eslint-disable @typescript-eslint/no-unused-vars */ -import { LightningElement, api, getComponentDef } from 'lwc'; +import { LightningElement, api, getComponentDef, createElement } from 'lwc'; import { ariaProperties } from 'test-utils'; import PublicProperties from 'x/publicProperties'; @@ -7,6 +7,8 @@ import PublicAccessors from 'x/publicAccessors'; import PublicMethods from 'x/publicMethods'; import PublicPropertiesInheritance from 'x/publicPropertiesInheritance'; import PublicMethodsInheritance from 'x/publicMethodsInheritance'; +import PrivateAccessors from 'x/privateAccessors'; +import HtmlElementProps from 'x/htmlElementProps'; function testInvalidComponentConstructor(name, ctor) { it(`should throw for ${name}`, () => { @@ -68,6 +70,9 @@ const GLOBAL_HTML_ATTRIBUTES = [ ...Object.keys(Element.prototype).filter((prop) => ariaProperties.includes(prop)), ].sort(); +const message = (propName) => + `Error: [LWC warn]: The property "${propName}" is not publicly accessible. Add the @api annotation to the property declaration or getter/setter in the component to make it accessible.`; + it('it should return the global HTML attributes in props', () => { class Component extends LightningElement {} const def = getComponentDef(Component); @@ -152,6 +157,74 @@ describe('@api', () => { childMethod: PublicMethodsInheritance.prototype.childMethod, }); }); + + it('should log warning when accessing a private prop', () => { + const elm = createElement('x-private-accessor', { is: PrivateAccessors }); + document.body.appendChild(elm); + + expect(() => { + elm['privateProp']; + }).toLogWarningDev(message('privateProp')); + }); + + it('should log warning when setting a private prop', () => { + const elm = createElement('x-private-accessor', { is: PrivateAccessors }); + document.body.appendChild(elm); + + expect(() => { + elm['privateProp'] = 'foo'; + }).toLogWarningDev(message('privateProp')); + }); + + it('should not log warning when accessing a public prop', () => { + const elm = createElement('x-private-accessor', { is: PrivateAccessors }); + document.body.appendChild(elm); + + expect(() => { + elm['publicProb']; + }).not.toLogWarningDev(); + }); + + it('should not log warning when setting a public prop', () => { + const elm = createElement('x-private-accessor', { is: PrivateAccessors }); + document.body.appendChild(elm); + + expect(() => { + elm['publicProb'] = 'foo'; + }).not.toLogWarningDev(); + }); + + it('should log warning when accessing a private prop without a getter', () => { + const elm = createElement('x-private-accessor', { is: PrivateAccessors }); + document.body.appendChild(elm); + + expect(() => { + elm['nonDecoratedPrivateProp']; + }).toLogWarningDev(message('nonDecoratedPrivateProp')); + }); + + it('should log warning when accessing a tracked private prop', () => { + const elm = createElement('x-private-accessor', { is: PrivateAccessors }); + document.body.appendChild(elm); + + expect(() => { + elm['trackedProp']; + }).toLogWarningDev(message('trackedProp')); + }); + + it('should not log a warning on HTMLElement props', () => { + const elm = createElement('x-html-element-props', { is: HtmlElementProps }); + document.body.appendChild(elm); + + expect(() => { + elm.constructor; + elm.tabIndex; + elm.title; + elm.attributes; + }).not.toLogWarningDev(); + + expect(elm.attributes.length).toBe(0); + }); }); describe('circular dependencies', () => { diff --git a/packages/@lwc/integration-karma/test/api/getComponentDef/x/htmlElementProps/htmlElementProps.js b/packages/@lwc/integration-karma/test/api/getComponentDef/x/htmlElementProps/htmlElementProps.js new file mode 100644 index 0000000000..e88ada6158 --- /dev/null +++ b/packages/@lwc/integration-karma/test/api/getComponentDef/x/htmlElementProps/htmlElementProps.js @@ -0,0 +1,11 @@ +import { LightningElement } from 'lwc'; + +export default class HtmlElementProps extends LightningElement { + constructor() { + super(); + } + + attributes = 'baz'; + tabIndex = 'bar'; + title = 'foo'; +} diff --git a/packages/@lwc/integration-karma/test/api/getComponentDef/x/privateAccessors/privateAccessors.js b/packages/@lwc/integration-karma/test/api/getComponentDef/x/privateAccessors/privateAccessors.js new file mode 100644 index 0000000000..0e8f55fe61 --- /dev/null +++ b/packages/@lwc/integration-karma/test/api/getComponentDef/x/privateAccessors/privateAccessors.js @@ -0,0 +1,27 @@ +import { LightningElement, api, track } from 'lwc'; + +export default class PrivateAccessors extends LightningElement { + _privateProp; + get privateProp() { + return this._privateProp; + } + + set privateProp(newValue) { + this._privateProp = newValue; + } + + _publicProp; + @api + get publicProp() { + return this._publicProp; + } + + set publicProp(newValue) { + this._publicProp = newValue; + } + + nonDecoratedPrivateProp = 'foo'; + + @track + trackedProp = 'bar'; +} diff --git a/packages/@lwc/integration-karma/test/component/decorators/api/index.spec.js b/packages/@lwc/integration-karma/test/component/decorators/api/index.spec.js index 3692b8ecdc..213b331445 100644 --- a/packages/@lwc/integration-karma/test/component/decorators/api/index.spec.js +++ b/packages/@lwc/integration-karma/test/component/decorators/api/index.spec.js @@ -20,7 +20,9 @@ describe('properties', () => { document.body.appendChild(elm); expect(elm.publicProp).toBeDefined(); - expect(elm.privateProp).toBeUndefined(); + expect(() => { + expect(elm.privateProp).toBeUndefined(); + }).toLogWarningDev(/Add the @api annotation to the property declaration/); }); it('should make the public property reactive if used in the template', () => { @@ -80,7 +82,9 @@ describe('methods', () => { const elm = createElement('x-methods', { is: Methods }); expect(elm.publicMethod).toBeDefined(); - expect(elm.privateMethod).toBeUndefined(); + expect(() => { + expect(elm.privateMethod).toBeUndefined(); + }).toLogWarningDev(/Add the @api annotation to the property declaration/); }); it('should invoke the method with the right this value and arguments', () => { diff --git a/packages/@lwc/integration-karma/test/shadow-dom/HTMLElement-properties/GlobalHTML.properties.spec.js b/packages/@lwc/integration-karma/test/shadow-dom/HTMLElement-properties/GlobalHTML.properties.spec.js index 4bdee1b515..15ffb8c870 100644 --- a/packages/@lwc/integration-karma/test/shadow-dom/HTMLElement-properties/GlobalHTML.properties.spec.js +++ b/packages/@lwc/integration-karma/test/shadow-dom/HTMLElement-properties/GlobalHTML.properties.spec.js @@ -39,12 +39,12 @@ describe('global HTML Properties', () => { document.body.appendChild(elm); const cmp = elm.componentInstance; expect(cmp.getAttribute('tabindex')).toBe('0'); - }), - it('should not throw when accessing attribute in root elements', () => { - const elm = createElement('x-foo', { is: Test }); - document.body.appendChild(elm); - elm.setAttribute('tabindex', 1); - }); + }); + it('should not throw when accessing attribute in root elements', () => { + const elm = createElement('x-foo', { is: Test }); + document.body.appendChild(elm); + elm.setAttribute('tabindex', 1); + }); it('should delete existing attribute prior rendering', () => { const elm = createElement('x-foo', { is: Test }); diff --git a/packages/@lwc/shared/src/language.ts b/packages/@lwc/shared/src/language.ts index 55088322fe..7a95ca7e33 100644 --- a/packages/@lwc/shared/src/language.ts +++ b/packages/@lwc/shared/src/language.ts @@ -11,6 +11,7 @@ const { defineProperty, freeze, getOwnPropertyDescriptor, + getOwnPropertyDescriptors, getOwnPropertyNames, getPrototypeOf, hasOwnProperty, @@ -99,6 +100,7 @@ export { forEach, freeze, getOwnPropertyDescriptor, + getOwnPropertyDescriptors, getOwnPropertyNames, getPrototypeOf, hasOwnProperty,