refactor(engine): clean up error messages (#1193)

* refactor(engine): tweak error messages for attributes.ts

* refactor(engine): tweak error messages for restrictions.ts

* chore: review feedback

* refactor: remove incorrect error

1) This code was dead code because globalHTMLProperty.reflective is either `true` or `undefined`.
2) The error was incorrect. If a property is not reflective, we shouldn't be using getAttribute to read its value.

* refactor: remove error and add reflective to global style attr

* chore: tweaks

* refactor: remove error logs for experimental properties

* chore: remove readonly error in getter and update wording

* chore: check that the global attr matches the attr being set

* chore: review feedback

* chore: review feedback

* chore: redefine getGlobalHTMLPropertiesInfo as an object

* refactor: remove logWarning

* chore: adjustments

* refactor: relax url validation for href attribute

* chore: remove errors

* chore: convert straggling logWarning to logError

* chore: remove logged error from base-bridge-element.ts

Assigning a non-reactive value ${newValue} to member property ${key} of ${vm} is not common because mutations on that value cannot be observed.

* chore: remove logged error from base-bridge-element.ts

If property ${key} decorated with @api in ${vm} is used in the template, the value ${newValue} set manually may be overridden by the template, consider binding the property only in the template.

* chore: remove logged error from template.ts

The template rendered by ${vm} references this.${propName}, which is not declared. Check for a typo in the template.

* chore: remove logged error from restrictions.ts

Invalid attribute access for ${name}. Don't use DOM APIs to mutate elements created by a template. Use the template to update the attribute instead.

* test: update tests to reflect error message changes

* chore: add back validation for unknown props in the template

* fix: ignore default slot content when validating slot names

packages/@lwc/engine/scripts/jest/matchers/log-matchers.js → packages/@lwc/engine/scripts/jest/matchers/log-error.js

@@ -100,5 +100,4 @@ function createMatcher(methodName) {
 
 module.exports = {
     toLogError: createMatcher('logError'),
-    toLogWarning: createMatcher('logWarning'),
 };

packages/@lwc/engine/scripts/jest/setup-test.js

@@ -12,10 +12,10 @@ const chalk = require('chalk');
 require('@lwc/synthetic-shadow');
 
 const { CONSOLE_WHITELIST } = require('./test-whitelist');
-const { toLogError, toLogWarning } = require('./matchers/log-matchers');
+const { toLogError } = require('./matchers/log-error');
 
 // Extract original methods from console
-const { warn: originalWarn, error: originalError } = console;
+const { error: originalError } = console;
 
 let currentSpec;
 jasmine.getEnv().addReporter({
@@ -28,50 +28,44 @@ jasmine.getEnv().addReporter({
 });
 
 beforeEach(() => {
-    console.warn = jest.spyOn(console, 'warn');
     console.error = jest.spyOn(console, 'error');
 });
 
 afterEach(() => {
     const { fullName } = currentSpec;
 
     const isWhitelistedTest = CONSOLE_WHITELIST.includes(fullName);
-    const didTestLogged = [...console.warn.mock.calls, ...console.error.mock.calls].length > 0;
+    const didTestLogged = console.error.mock.calls.length > 0;
 
     try {
         if (isWhitelistedTest) {
             if (!didTestLogged) {
+                const boldedName = chalk.green.bold(fullName);
+                const boldedFile = chalk.green.bold('test-whitelist.js');
                 const message = [
-                    `This test used to used to log a warning or an error, but don't log anymore.`,
-                    `Please remove "${chalk.green.bold(fullName)}" from "${chalk.green.bold(
-                        'test-whitelist.js'
-                    )}"`,
+                    `This test used to log an error but no longer does.`,
+                    `Please remove "${boldedName}" from "${boldedFile}"`,
                 ].join('\n');
 
                 throw new Error(message);
             }
         } else {
             if (didTestLogged) {
                 const message = [
-                    `Expect test not to log an error or a warning.\n`,
-                    `If the message expected, make sure you asserts against those logs in the tests.\n`,
-                    `Use instead: ${chalk.green.bold(
-                        `expect(<function>).toLogError(<message>)`
-                    )} or ${chalk.green.bold(`expect(<function>).toLogWarning(<message>)`)}`,
+                    `Expected the test to not result in an error being logged.`,
+                    `If this is expected, make sure to add an assertion for it:`,
+                    `${chalk.green.bold(`expect(<function>).toLogError(<message>)`)}`,
                 ].join('\n');
 
                 throw new Error(message);
             }
         }
     } finally {
-        // Make sure to reset the original console methods after each tests
-        console.warn = originalWarn;
         console.error = originalError;
     }
 });
 
 // Register custom console matchers in jasmine
 expect.extend({
     toLogError,
-    toLogWarning,
 });

packages/@lwc/engine/scripts/jest/test-whitelist.js

@@ -26,7 +26,6 @@ const CONSOLE_WHITELIST = [
     'html-element global HTML Properties should log console error when user land code changes attribute via querySelector',
     'html-element global HTML Properties should log console error when user land code removes attribute via querySelector',
     'html-element global HTML Properties should log error message when attribute is set via elm.setAttribute if reflective property is defined',
-    'html-element life-cycles should not throw error when accessing a non-observable property from tracked property when not rendering',
 ];
 
 for (let i = 0; i < CONSOLE_WHITELIST.length; i++) {

packages/@lwc/engine/src/framework/__tests__/api.spec.ts

@@ -125,7 +125,7 @@ describe('api', () => {
             expect(vnodes).toEqual(['1a', '2a']);
         });
 
-        it('should log warning when invalid iteration value', () => {
+        it('should log an error when invalid iteration value', () => {
             const html = compileTemplate(`<template></template>`);
             class VmRendering extends LightningElement {
                 render() {
@@ -136,7 +136,7 @@ describe('api', () => {
             const elm = createElement('x-vm-aux', { is: VmRendering });
             expect(() => {
                 document.body.appendChild(elm);
-            }).toLogWarning('it should be an Array or an iterable Object.');
+            }).toLogError('It must be an Array or an iterable Object.');
         });
     });
 
@@ -225,7 +225,7 @@ describe('api', () => {
             const elm = createElement('x-foo', { is: Foo });
             expect(() => {
                 document.body.appendChild(elm);
-            }).toLogWarning('This attribute can only be set to 0 or -1.');
+            }).toLogError('This attribute must be set to 0 or -1.');
             expect(normalized).toBe(0);
         });
 
@@ -275,7 +275,7 @@ describe('api', () => {
             const elm = createElement('x-foo', { is: Foo });
             expect(() => {
                 document.body.appendChild(elm);
-            }).toLogWarning('This attribute can only be set to 0 or -1.');
+            }).toLogError('This attribute must be set to 0 or -1.');
             expect(normalized).toBe(0);
         });
 

packages/@lwc/engine/src/framework/__tests__/html-element.spec.ts

@@ -495,7 +495,7 @@ describe('html-element', () => {
             }).not.toThrow();
         });
 
-        it('should not log a warning when setting tracked value to null', function() {
+        it('should not log an error when setting tracked value to null', function() {
             class MyComponent extends LightningElement {
                 state = {};
                 connectedCallback() {
@@ -506,10 +506,10 @@ describe('html-element', () => {
             MyComponent.track = { state: 1 };
             const elm = createElement('x-foo-tracked-null', { is: MyComponent });
 
-            expect(() => document.body.appendChild(elm)).not.toLogWarning();
+            expect(() => document.body.appendChild(elm)).not.toLogError();
         });
 
-        it('should not log a warning when initializing api value to null', function() {
+        it('should not log an error when initializing api value to null', function() {
             class MyComponent extends LightningElement {
                 foo = null;
             }
@@ -518,7 +518,7 @@ describe('html-element', () => {
             };
             const elm = createElement('x-foo-init-api', { is: MyComponent });
 
-            expect(() => document.body.appendChild(elm)).not.toLogWarning();
+            expect(() => document.body.appendChild(elm)).not.toLogError();
         });
     });
 
@@ -1386,7 +1386,7 @@ describe('html-element', () => {
             expect(() => {
                 createElement('prop-setter-title', { is: MyComponent });
             }).toLogError(
-                `constructor should not read the value of property "title". The owner component has not yet set the value. Instead use the constructor to set default values for properties.`
+                "`PatchedHTMLElement` constructor can't read the value of property `title` because the owner component hasn't set the value yet. Instead, use the `PatchedHTMLElement` constructor to set a default value for the property."
             );
         });
 

packages/@lwc/engine/src/framework/__tests__/scoped-ids.spec.ts

@@ -86,7 +86,9 @@ describe('scoped-ids', () => {
                 const elm = createElement('x-foo', { is: MyComponent });
                 expect(() => {
                     document.body.appendChild(elm);
-                }).toLogError('Invalid id value "undefined". Expected a non-empty string.');
+                }).toLogError(
+                    'Invalid id value "undefined". The id attribute must contain a non-empty string.'
+                );
                 const div = elm.shadowRoot.querySelector('div');
                 expect(div.getAttribute('id')).toEqual(null);
             });
@@ -104,7 +106,9 @@ describe('scoped-ids', () => {
                 const elm = createElement('x-foo', { is: MyComponent });
                 expect(() => {
                     document.body.appendChild(elm);
-                }).toLogError('Invalid id value "". Expected a non-empty string.');
+                }).toLogError(
+                    'Invalid id value "". The id attribute must contain a non-empty string.'
+                );
                 const div = elm.shadowRoot.querySelector('div');
                 expect(div.getAttribute('id')).toEqual('');
             });

packages/@lwc/engine/src/framework/api.ts

@@ -270,8 +270,8 @@ export function h(sel: string, data: ElementCompilerData, children: VNodes): VEl
             `vnode.data.styleMap and vnode.data.style ambiguous declaration.`
         );
         if (data.style && !isString(data.style)) {
-            assert.logWarning(
-                `Invalid 'style' attribute passed to <${sel}> should be a string value, and will be ignored.`,
+            assert.logError(
+                `Invalid 'style' attribute passed to <${sel}> is ignored. This attribute must be a string value.`,
                 vmBeingRendered!.elm
             );
         }
@@ -321,10 +321,10 @@ export function ti(value: any): number {
     const shouldNormalize = value > 0 && !(isTrue(value) || isFalse(value));
     if (process.env.NODE_ENV !== 'production') {
         if (shouldNormalize) {
-            assert.logWarning(
+            assert.logError(
                 `Invalid tabindex value \`${toString(
                     value
-                )}\` in template for ${vmBeingRendered}. This attribute can only be set to 0 or -1.`,
+                )}\` in template for ${vmBeingRendered}. This attribute must be set to 0 or -1.`,
                 vmBeingRendered!.elm
             );
         }
@@ -392,8 +392,8 @@ export function c(
             `vnode.data.styleMap and vnode.data.style ambiguous declaration.`
         );
         if (data.style && !isString(data.style)) {
-            assert.logWarning(
-                `Invalid 'style' attribute passed to <${sel}> should be a string value, and will be ignored.`,
+            assert.logError(
+                `Invalid 'style' attribute passed to <${sel}> is ignored. This attribute must be a string value.`,
                 vmBeingRendered!.elm
             );
         }
@@ -444,10 +444,10 @@ export function i(
     markAsDynamicChildren(list);
     if (isUndefined(iterable) || iterable === null) {
         if (process.env.NODE_ENV !== 'production') {
-            assert.logWarning(
+            assert.logError(
                 `Invalid template iteration for value "${toString(
                     iterable
-                )}" in ${vmBeingRendered}, it should be an Array or an iterable Object.`,
+                )}" in ${vmBeingRendered}. It must be an Array or an iterable Object.`,
                 vmBeingRendered!.elm
             );
         }
@@ -459,7 +459,7 @@ export function i(
             isUndefined(iterable[SymbolIterator]),
             `Invalid template iteration for value \`${toString(
                 iterable
-            )}\` in ${vmBeingRendered}, it requires an array-like object, not \`null\` or \`undefined\`.`
+            )}\` in ${vmBeingRendered}. It must be an array-like object and not \`null\` nor \`undefined\`.`
         );
     }
     const iterator = iterable[SymbolIterator]();
@@ -502,15 +502,15 @@ export function i(
                         if (keyMap[key] === 1 && isUndefined(iterationError)) {
                             iterationError = `Duplicated "key" attribute value for "<${
                                 childVnode.sel
-                            }>" in ${vmBeingRendered} for item number ${j}. Key with value "${
+                            }>" in ${vmBeingRendered} for item number ${j}. A key with value "${
                                 childVnode.key
-                            }" appears more than once in iteration. Key values must be unique numbers or strings.`;
+                            }" appears more than once in the iteration. Key values must be unique numbers or strings.`;
                         }
                         keyMap[key] = 1;
                     } else if (isUndefined(iterationError)) {
                         iterationError = `Invalid "key" attribute value in "<${
                             childVnode.sel
-                        }>" in ${vmBeingRendered} for item number ${j}. Instead set a unique "key" attribute value on all iteration children so internal state can be preserved during rehydration.`;
+                        }>" in ${vmBeingRendered} for item number ${j}. Set a unique "key" value on all iterated child elements.`;
                     }
                 }
             });
@@ -681,7 +681,7 @@ export function gid(id: string | undefined | null): string | null | undefined {
     if (isUndefined(id) || id === '') {
         if (process.env.NODE_ENV !== 'production') {
             assert.logError(
-                `Invalid id value "${id}". Expected a non-empty string.`,
+                `Invalid id value "${id}". The id attribute must contain a non-empty string.`,
                 vmBeingRendered!.elm
             );
         }
@@ -698,10 +698,12 @@ export function gid(id: string | undefined | null): string | null | undefined {
 export function fid(url: string | undefined | null): string | null | undefined {
     if (isUndefined(url) || url === '') {
         if (process.env.NODE_ENV !== 'production') {
-            assert.logError(
-                `Invalid url value "${url}". Expected a non-empty string.`,
-                vmBeingRendered!.elm
-            );
+            if (isUndefined(url)) {
+                assert.logError(
+                    `Undefined url value for "href" or "xlink:href" attribute. Expected a non-empty string.`,
+                    vmBeingRendered!.elm
+                );
+            }
         }
         return url;
     }