refactor(engine): clean up error messages #1193

ekashida · 2019-04-23T06:17:27Z

Does this PR introduce a breaking change?

No

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

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

-    'This property will round the value to an integer, and it is considered an anti-pattern. Instead, you can use `this.getBoundingClientRect()` to obtain `left`, `top`, `right`, `bottom`, `x`, `y`, `width`, and `height` fractional values describing the overall border-box in pixels.';
+function offsetPropertyErrorMessage(name) {
+    return `Using the \`${name}\` property is considered an anti-pattern because it will round the value to an integer. Use the \`getBoundingClientRect\` method instead to obtain fractional values for the size of an element and its position relative to the viewport.`;
+}

 // Global HTML Attributes & Properties
 // https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes


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

@@ -95,9 +96,9 @@ function getNodeRestrictionsDescriptors(
    return {
        appendChild: generateDataDescriptor({
            value(this: Node, aChild: Node) {
-                if (this instanceof Element && options.isPortal !== true) {
+                if (this instanceof Element && !isTrue(options.isPortal)) {


caridy · 2019-04-23T14:08:48Z

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

@@ -106,9 +107,9 @@ function getNodeRestrictionsDescriptors(
        }),
        insertBefore: generateDataDescriptor({
            value(this: Node, newNode: Node, referenceNode: Node) {
-                if (!isDomMutationAllowed && this instanceof Element && options.isPortal !== true) {
+                if (!isDomMutationAllowed && this instanceof Element && !isTrue(options.isPortal)) {


caridy

LGTM, the only small detail is to use isFalse vs isTrue

kevinv11n

@jbeyle should review this PR

jbleyleSF · 2019-04-23T18:54:19Z

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

@@ -78,7 +79,7 @@ export function getGlobalHTMLPropertiesInfo() {
        dataset: {
            readOnly: true,
            error:
-                'Using property "dataset" is an anti-pattern. Components should not rely on dataset to implement its internal logic, nor use that as a communication channel.',
+                'Using the `dataset` property is an anti-pattern because it is not statically analyzable. Components should instead expose its public API using the `@api` decorator.',


Using the dataset property is an anti-pattern because it can't be statically analyzed. To expose the component's public API, use the @api decorator instead.

jbleyleSF · 2019-04-23T18:55:13Z

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

@@ -157,7 +158,8 @@ export function getGlobalHTMLPropertiesInfo() {
        },
        style: {
            attribute: 'style',
-            error: `Using property or attribute "style" is an anti-pattern. Instead use property "classList".`,
+            error:
+                'Using the `style` attribute is an anti-pattern. Use the `classList` API along with classes defined in a CSS file instead.',


Use the classList API and define classes in a CSS file instead.

jbleyleSF · 2019-04-23T18:55:40Z

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

@@ -177,7 +179,7 @@ export function getGlobalHTMLPropertiesInfo() {
        slot: {
            attribute: 'slot',
            experimental: true,
-            error: `Using property or attribute "slot" is an anti-pattern.`,
+            error: 'Using the `slot` attribute is an anti-pattern.',


Do we have an "instead" for this rule?

We don't. This is just something we ask users not to do because it's not really a useful feature and would entail needless complexity. @caridy can keep me honest here.

you're correct

jbleyleSF · 2019-04-23T18:59:58Z

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

                    assert.logError(
-                        `insertBefore is disallowed in Element unless \`lwc:dom="manual"\` directive is used in the template.`,
+                        'The `insertBefore` method is only available on elements using the `lwc:dom="manual"` directive.',


using -> that have

jbleyleSF · 2019-04-23T19:00:07Z

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

                    assert.logError(
-                        `removeChild is disallowed in Element unless \`lwc:dom="manual"\` directive is used in the template.`,
+                        'The `removeChild` method is only available on elements using the `lwc:dom="manual"` directive.',


using -> that have

jbleyleSF · 2019-04-23T19:00:14Z

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

                    assert.logError(
-                        `replaceChild is disallowed in Element unless \`lwc:dom="manual"\` directive is used in the template.`,
+                        'The `replaceChild` method is only available on elements using the `lwc:dom="manual"` directive.',


using -> that have

jbleyleSF · 2019-04-23T19:00:25Z

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

                    assert.logError(
-                        `nodeValue is disallowed in Element unless \`lwc:dom="manual"\` directive is used in the template.`,
+                        'The `nodeValue` property is only available on elements using the `lwc:dom="manual"` directive.',


using -> that have

jbleyleSF · 2019-04-23T19:00:34Z

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

                    assert.logError(
-                        `textContent is disallowed in Element unless \`lwc:dom="manual"\` directive is used in the template.`,
+                        'The `textContent` property is only available on elements using the `lwc:dom="manual"` directive.',


using -> that have

jbleyleSF · 2019-04-23T19:00:44Z

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

                    assert.logError(
-                        `innerHTML is disallowed in Element unless \`lwc:dom="manual"\` directive is used in the template.`,
+                        'The `innerHTML` property is only available on elements using the `lwc:dom="manual"` directive.',


using -> that have

jbleyleSF · 2019-04-23T19:02:29Z

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

        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.`,
+            `Invalid attribute access for \`${name}\`. Elements created via template should not be mutated using DOM APIs. Update the attribute through the template instead.`,


Elements created via template should not be mutated using DOM APIs. -> Don't use DOM APIs to mutate elements created by a template. Use the template to update the attribute instead.

jbleyleSF · 2019-04-23T19:04:07Z

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

@@ -484,7 +485,7 @@ function getComponentRestrictionsDescriptors(cmp: ComponentInterface): PropertyD
                            assert.logError(error, getComponentVM(this).elm);
                        } else if (experimental) {
                            assert.logError(
-                                `Attribute \`${attrName}\` is an experimental attribute that is not standardized or supported by all browsers. Property "${propName}" and attribute "${attrName}" are ignored.`,
+                                `Using the experimental \`${attrName}\` attribute and its corresponding \`${propName}\` property is disallowed as it is not standardized or supported by all browsers.`,


as it is not -> because it is not

apapko · 2019-04-24T16:31:09Z

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

@@ -78,7 +79,7 @@ export function getGlobalHTMLPropertiesInfo() {
        dataset: {
            readOnly: true,
            error:
-                'Using property "dataset" is an anti-pattern. Components should not rely on dataset to implement its internal logic, nor use that as a communication channel.',
+                "Using the `dataset` property is an anti-pattern because it can't be statically analyzed. To expose the component's public API, use the `@api` decorator instead.",


"To expose the component's public api" - i feel like this maybe confusing for the devs trying to use data-*. What are your thoughts on:
"Using the dataset property is an anti-pattern because it can't be statically analyzed. Instead of exposing data-*, use component's public API -@api decorator."

I see what you mean, but I think your suggestion is also a bit ambiguous. How about:

Using the dataset property is an anti-pattern because it can't be statically analyzed. Expose each property individually using the @api decorator instead.

apapko · 2019-04-24T16:34:42Z

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

        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.`,
+            `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.`,


Use the template to update the attribute instead.

Are we asking to make the change in the markup itself? I'm not quite sure what the direction implies

We're telling the author that they should update attribute values through the template instead of doing it programmatically via setAttribute. Does that clear it up? Do you have a better suggestion?

I don't like this error because we do this on purpose in components when we need an escape hatch, the one that comes to mind is dealing with aria attributes that need to cross shadow boundaries. That results in noise for everyone else and they can't act on it

ekashida · 2019-04-30T00:45:41Z

@caridy Removed our experimental property validation because I agree with @pmdartus that we shouldn't be in the business of preventing our users from using these.

ekashida · 2019-04-30T00:54:38Z

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

            reflective: true,
        },
        style: {
            attribute: 'style',
-            error: `Using property or attribute "style" is an anti-pattern. Instead use property "classList".`,


@caridy Removed this since there's no reason to block users from doing setAttribute('style', styleValue). We also log a warning when the user tries to access this.style in the component.

ekashida · 2019-04-30T00:58:16Z

packages/@lwc/engine/src/framework/decorators/register.ts

-            if (
-                globalHTMLProperty &&
-                globalHTMLProperty.attribute &&
-                globalHTMLProperty.reflective === false


Removed this whole block because it was dead code (.reflective was either undefined or true) and this code executes during initialization of public attributes which can be move to the compiler.

ekashida · 2019-04-30T01:00:29Z

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

-                    }
-                    if (readOnly) {
-                        // TODO - need to improve this message
-                        msg.push(`Property is read-only.`);


@caridy Didn't make sense to log this warning in the getter. Moving to the setter.

pmdartus

@ekashida Did you do a pass on all the lightning component warnings to verify if the warnings and errors are still valid?

pmdartus · 2019-04-30T07:29:33Z

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

@@ -78,86 +79,74 @@ export function getGlobalHTMLPropertiesInfo() {
        dataset: {
            readOnly: true,


readOnly and reflective properties are not used anymore. We should simplify the code here by converting the getGlobalHTMLPropertiesInfo method into an object literal:

export const HTMLPropsAttributeMapping: { [prop: string]: { attribute: string, error?: string } } = { /* ... */ };

apapko

Few minor comments/question, but other than that great improvement! Thank you

apapko · 2019-05-23T04:06:57Z

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

@@ -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."


what do you think of 'parent' instead of 'owner'?

On the second note, should we point the user to the lifecycle hook during the invocation of which the owner has already set the values? Such as connected callback?

@apapko I think suggesting a different lifecycle hook might be assuming too much since we don't know what the developer is trying to do. They might have some logic which doesn't care about whether the value is the initialized value or the owner-provided value.

The error message does seem a little weird though since it looks like it'll get logged even for

class Foo extends LightningElement { @api coffee = 'black'; constructor() { super(); this._coffee = this.coffee; } }

@caridy Seems like another candidate for removal?

For posterity, we discussed this and agreed it should be removed since we can't predict the author's intention.

apapko · 2019-05-23T04:14:07Z

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

+    isUndefined,
+    StringReplace,
+    StringToLowerCase,
+} from '../shared/language';


thanks for the cleanup

This is mainly our code formatter in action, although I did take the opportunity to reorder the imports :)

apapko · 2019-05-23T04:17:47Z

packages/@lwc/engine/src/framework/base-bridge-element.ts

@@ -82,7 +82,7 @@ function createSetter(key: string) {
                if (vmBeingUpdated !== vm) {
                    // logic for setting new properties of the element directly from the DOM
                    // is only recommended for root elements created via createElement()
-                    assert.logWarning(
+                    assert.logError(


I agree that this should be an error. i wonder how many cases we have of devs violating this assertion.

apapko · 2019-05-23T04:23:48Z

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

-                );
+                const { error, attribute } = globalHTMLProperties[propName];
+                const msg: string[] = [];
+                msg.push(`Accessing the global HTML property "${propName}" is disabled.`);


id access disabled or disallowed?

I have no opinion on this.

apapko · 2019-05-23T04:37:09Z

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

-            assert.logWarning(
-                `The template rendered by ${vm} references \`this.${propName}\`, which is not declared. This is likely a typo in the template.`,
+            assert.logError(
+                `The template rendered by ${vm} references \`this.${propName}\`, which is not declared. Check for a typo in the template.`,


Does this work today? I reference a non-existing property but don't see any warnings:
https://playground.lwcjs.org/projects/g7KqPbpUA/2/edit

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

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.

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

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.

caridy

looking good! still a bunch of karma tests failing due to the mismatch with the old messages

salesforce-best-lwc-internal · 2019-05-23T23:05:44Z

Benchmark results

Base commit: 47437a1 | Target commit: 248e74a

lwc-engine-benchmark

table-append-1k	metric	base(`47437a1`)	target(`248e74a`)	trend
benchmark-table/append/1k	duration	139.55 (±2.30 ms)	142.85 (±3.70 ms)	+3.3ms (2.4%) 👎

table-clear-1k	metric	base(`47437a1`)	target(`248e74a`)	trend
benchmark-table/clear/1k	duration	10.40 (±1.10 ms)	10.40 (±0.80 ms)	0.0ms (0.0%) 👌

table-create-10k	metric	base(`47437a1`)	target(`248e74a`)	trend
benchmark-table/create/10k	duration	846.45 (±5.00 ms)	853.35 (±6.50 ms)	+6.9ms (0.8%) 👎

table-create-1k	metric	base(`47437a1`)	target(`248e74a`)	trend
benchmark-table/create/1k	duration	107.60 (±2.40 ms)	107.65 (±3.35 ms)	+0.1ms (0.0%) 👌

table-update-10th-1k	metric	base(`47437a1`)	target(`248e74a`)	trend
benchmark-table/update-10th/1k	duration	69.30 (±3.50 ms)	68.60 (±4.00 ms)	-0.7ms (1.0%) 👌

tablecmp-append-1k	metric	base(`47437a1`)	target(`248e74a`)	trend
benchmark-table-component/append/1k	duration	210.95 (±17.95 ms)	220.95 (±14.05 ms)	+10.0ms (4.7%) 👌

tablecmp-clear-1k	metric	base(`47437a1`)	target(`248e74a`)	trend
benchmark-table-component/clear/1k	duration	7.15 (±1.40 ms)	7.10 (±1.30 ms)	-0.1ms (0.7%) 👌

tablecmp-create-10k	metric	base(`47437a1`)	target(`248e74a`)	trend
benchmark-table-component/create/10k	duration	1606.70 (±10.05 ms)	1610.60 (±13.90 ms)	+3.9ms (0.2%) 👌

tablecmp-create-1k	metric	base(`47437a1`)	target(`248e74a`)	trend
benchmark-table-component/create/1k	duration	182.90 (±5.05 ms)	189.90 (±5.60 ms)	+7.0ms (3.8%) 👎

tablecmp-update-10th-1k	metric	base(`47437a1`)	target(`248e74a`)	trend
benchmark-table-component/update-10th/1k	duration	68.25 (±5.25 ms)	67.20 (±4.40 ms)	-1.1ms (1.5%) 👌

wc-append-1k	metric	base(`47437a1`)	target(`248e74a`)	trend
benchmark-table-wc/append/1k	duration	239.05 (±14.35 ms)	240.75 (±12.75 ms)	+1.7ms (0.7%) 👌

wc-clear-1k	metric	base(`47437a1`)	target(`248e74a`)	trend
benchmark-table-wc/clear/1k	duration	12.10 (±1.30 ms)	12.00 (±1.40 ms)	-0.1ms (0.8%) 👌

wc-create-10k	metric	base(`47437a1`)	target(`248e74a`)	trend
benchmark-table-wc/create/10k	duration	1808.00 (±13.55 ms)	1750.00 (±11.60 ms)	-58.0ms (3.2%) 👍

wc-create-1k	metric	base(`47437a1`)	target(`248e74a`)	trend
benchmark-table-wc/create/1k	duration	202.15 (±5.25 ms)	200.20 (±5.65 ms)	-2.0ms (1.0%) 👌

wc-update-10th-1k	metric	base(`47437a1`)	target(`248e74a`)	trend
benchmark-table-wc/update-10th/1k	duration	68.45 (±5.15 ms)	66.55 (±5.45 ms)	-1.9ms (2.8%) 👌

restriction was removed in #1193

* test: remove test for duplicate handlers restriction was removed in #1193 * test: enable test for readonly properties * test: enable test for dispatchEvent * test: remove test for removing non-existent listener on custom element * test: test setting innerHTML, outerHTML on host will throw fixed in #1001 * test: enable tests migrated to karma suite * test: more enabled tests * test: revert the suite back to disabled state * test: account for different error message across browsers * test: error message in ie11 is taken care of * fix: isConnected polyfill in IE11 fixes #987 * test: enable tests for #997 and #990 issue Tests for #997 & #990 * test: enable test for iteration fix #1285 * test: adjust error message fix #1285 * test: enable tests that work in native-shadow * test: disable non-composed event tests because of webkit bug WebKit bug - https://bugs.webkit.org/show_bug.cgi?id=206374 * fix: use regex for error message variants

This comment has been minimized.

Sign in to view

pmdartus reviewed Apr 23, 2019

View reviewed changes

caridy reviewed Apr 23, 2019

View reviewed changes

caridy approved these changes Apr 23, 2019

View reviewed changes

kevinv11n reviewed Apr 23, 2019

View reviewed changes

jbleyleSF reviewed Apr 23, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

apapko reviewed Apr 24, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

ekashida force-pushed the ekashida/error-messages branch from 8ed6687 to 43df9c9 Compare April 28, 2019 00:32

This comment has been minimized.

Sign in to view

ekashida commented Apr 30, 2019

View reviewed changes

pmdartus reviewed Apr 30, 2019

View reviewed changes

pmdartus changed the title ~~refactor(engine): clean up error messages~~ refactor(engine): clean up error message Apr 30, 2019

ekashida and others added 9 commits May 22, 2019 19:59

refactor: remove error logs for experimental properties

e81a86d

chore: remove readonly error in getter and update wording

b4d3411

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

b2146b0

chore: review feedback

c749398

chore: review feedback

97e0da9

chore: redefine getGlobalHTMLPropertiesInfo as an object

a8e0664

refactor: remove logWarning

9f83c19

chore: adjustments

598c7b4

refactor: relax url validation for href attribute

db4983a

ekashida force-pushed the ekashida/error-messages branch from f0484df to db4983a Compare May 23, 2019 03:04

ekashida added 2 commits May 22, 2019 20:11

chore: remove errors

764fd61

chore: convert straggling logWarning to logError

8fdfcc2

This comment has been minimized.

Sign in to view

apapko approved these changes May 23, 2019

View reviewed changes

ekashida added 4 commits May 22, 2019 23:39

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

de16257

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

9cae359

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

042f838

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

e21fd2d

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.

This comment has been minimized.

Sign in to view

caridy approved these changes May 23, 2019

View reviewed changes

test: update tests to reflect error message changes

7ff440a

This comment has been minimized.

Sign in to view

ekashida added 2 commits May 23, 2019 15:24

chore: add back validation for unknown props in the template

5b581fb

fix: ignore default slot content when validating slot names

248e74a

ekashida merged commit 1afe614 into master May 23, 2019

ekashida deleted the ekashida/error-messages branch May 23, 2019 23:30

ravijayaramappa added a commit that referenced this pull request Jan 13, 2020

test: remove test for duplicate handlers

a06b41c

restriction was removed in #1193

ravijayaramappa mentioned this pull request Mar 5, 2020

test: migrate test suites with skipped unit tests to karma #1753

Merged

		@@ -78,86 +79,74 @@ export function getGlobalHTMLPropertiesInfo() {
		dataset: {
		readOnly: true,

refactor(engine): clean up error messages #1193

refactor(engine): clean up error messages #1193

Conversation

ekashida commented Apr 23, 2019

Does this PR introduce a breaking change?

This comment has been minimized.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

caridy left a comment

Choose a reason for hiding this comment

kevinv11n left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

ekashida commented Apr 30, 2019

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmdartus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

apapko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment has been minimized.

caridy left a comment

Choose a reason for hiding this comment

This comment has been minimized.

salesforce-best-lwc-internal bot commented May 23, 2019

Benchmark results

lwc-engine-benchmark