feat: add dev-only warning to props that are not decorated with @api (#…

…3713)

Fixes #3641

Co-authored-by: Nolan Lawson <nlawson@salesforce.com>

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,15 +108,37 @@ 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;
 }
 
 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) {

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

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', () => {

packages/@lwc/integration-karma/test/api/getComponentDef/index.spec.js

@@ -1,12 +1,14 @@
 /* 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';
 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', () => {

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

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

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', () => {

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

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,