Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: no-attributes-during-construction #61

Merged
merged 8 commits into from
Mar 12, 2021
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ To choose from three configuration settings, install the [`eslint-config-lwc`](h
| [lwc/no-api-reassignments](./docs/rules/no-api-reassignments.md) | prevent public property reassignments | |
| [lwc/no-deprecated](./docs/rules/no-deprecated.md) | disallow usage of deprecated LWC APIs | |
| [lwc/no-document-query](./docs/rules/no-document-query.md) | disallow DOM query at the document level | |
| [lwc/no-attributes-during-construction](./docs/rules/no-attributes-during-construction.md) | disallow attributes during construction | |
ekashida marked this conversation as resolved.
Show resolved Hide resolved
| [lwc/no-leading-uppercase-api-name](./docs/rules/no-leading-uppercase-api-name.md) | ensure public property doesn't start with an upper-case character | |
| [lwc/no-unexpected-wire-adapter-usages](./docs/rules/no-unexpected-wire-adapter-usages.md) | enforce wire adapters to be used with `wire` decorator | |
| [lwc/no-unknown-wire-adapters](./docs/rules/no-unknown-wire-adapters.md) | disallow usage of unknown wire adapters | |
Expand Down
51 changes: 51 additions & 0 deletions docs/rules/no-attributes-during-construction.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
# Disallow attributes during construction (no-attributes-during-construction)
ekashida marked this conversation as resolved.
Show resolved Hide resolved

The `LightningElement` base class extended by LWC component classes defines several properties that, when set,
causes its custom element to gain attributes. By
ekashida marked this conversation as resolved.
Show resolved Hide resolved
[specification](https://html.spec.whatwg.org/multipage/custom-elements.html#custom-element-conformance), custom element
constructors must not cause the custom element to gain attributes.

This rule prevents set operations in the constructor method that violate this restriction. It only knows about
`LightningElement` properties that implement this behavior. It will not detect custom properties that set attributes
during construction time.

A few examples of such properties are `hidden`, `id`, `role`, `tabIndex`, and `title`.

## Rule details

Example of **incorrect** code:

```js
import { LightningElement } from 'lwc';

export default class Test extends LightningElement {
constructor() {
this.title = 'this causes the element to gain the title attribute during construction';
}
}
```

Examples of **correct** code:

```js
import { LightningElement } from 'lwc';

export default class Test extends LightningElement {
connectedCallback() {
this.title = 'this causes the element to gain the title attribute upon connection';
}
}
```

```js
import { LightningElement } from 'lwc';

export default class Test extends LightningElement {
title = 'this custom property overrides the one in LightningElement';

constructor() {
this.title =
'this does not cause the element to gain the title attribute during construction';
}
}
```
1 change: 1 addition & 0 deletions lib/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ const rules = {
'no-api-reassignments': require('./rules/no-api-reassignments'),
'no-async-await': require('./rules/no-async-await'),
'no-async-operation': require('./rules/no-async-operation'),
'no-attributes-during-construction': require('./rules/no-attributes-during-construction'),
'no-deprecated': require('./rules/no-deprecated'),
'no-document-query': require('./rules/no-document-query'),
'no-dupe-class-members': require('./rules/no-dupe-class-members'),
Expand Down
132 changes: 132 additions & 0 deletions lib/rules/no-attributes-during-construction.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
/*
* Copyright (c) 2018, salesforce.com, inc.
* All rights reserved.
* SPDX-License-Identifier: MIT
* For full license text, see the LICENSE file in the repo root or https://opensource.org/licenses/MIT
*/
'use strict';

const { isComponent } = require('../util/component');
const { docUrl } = require('../util/doc-url');

// https://github.com/salesforce/lwc/blob/6e6ad1011887d842e3f03546c6a81e9cee057611/packages/%40lwc/shared/src/aria.ts#L10-L67
const ARIA_PROPERTY_NAMES = [
'ariaActiveDescendant',
'ariaAtomic',
'ariaAutoComplete',
'ariaBusy',
'ariaChecked',
'ariaColCount',
'ariaColIndex',
'ariaColSpan',
'ariaControls',
'ariaCurrent',
'ariaDescribedBy',
'ariaDetails',
'ariaDisabled',
'ariaErrorMessage',
'ariaExpanded',
'ariaFlowTo',
'ariaHasPopup',
'ariaHidden',
'ariaInvalid',
'ariaKeyShortcuts',
'ariaLabel',
'ariaLabelledBy',
'ariaLevel',
'ariaLive',
'ariaModal',
'ariaMultiLine',
'ariaMultiSelectable',
'ariaOrientation',
'ariaOwns',
'ariaPlaceholder',
'ariaPosInSet',
'ariaPressed',
'ariaReadOnly',
'ariaRelevant',
'ariaRequired',
'ariaRoleDescription',
'ariaRowCount',
'ariaRowIndex',
'ariaRowSpan',
'ariaSelected',
'ariaSetSize',
'ariaSort',
'ariaValueMax',
'ariaValueMin',
'ariaValueNow',
'ariaValueText',
'role',
];

// https://github.com/salesforce/lwc/blob/6e6ad1011887d842e3f03546c6a81e9cee057611/packages/%40lwc/engine-core/src/framework/attributes.ts#L9-L20
const LWC_DEFAULT_PROPERTY_NAMES = [
'accessKey',
'dir',
'draggable',
'hidden',
'id',
'lang',
'spellcheck',
'tabIndex',
'title',
];

const PROPERTIES_THAT_REFLECT = new Set([...ARIA_PROPERTY_NAMES, ...LWC_DEFAULT_PROPERTY_NAMES]);

module.exports = {
meta: {
type: 'problem',
docs: {
description: 'no attributes during construction',
category: 'LWC',
recommended: true,
url: docUrl('no-attributes-during-construction'),
},
schema: [],
},

create(context) {
return {
'MethodDefinition[kind=constructor]': function (ctorMethod) {
const classBody = ctorMethod.parent;

// classBody.parent is ClassDeclaration or ClassExpression
if (!isComponent(classBody.parent, context)) {
return;
}

const blockStatement = ctorMethod.value.body;
const assignmentExpressions = blockStatement.body
.filter((node) => node.type === 'ExpressionStatement')
.filter((node) => node.expression.type === 'AssignmentExpression')
ekashida marked this conversation as resolved.
Show resolved Hide resolved
.map((node) => node.expression);

if (assignmentExpressions.length !== 0) {
const classProperties = classBody.body
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I never realized before this that if a class field is defined with the same name than a global HTML property, the property doesn't reflect.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, the descriptors we copy from the HTMLElement prototype are added to the base lightning element so they can be overridden by the component class.

ekashida marked this conversation as resolved.
Show resolved Hide resolved
.filter(
({ computed, type }) => type === 'ClassProperty' && computed === false,
)
.map((node) => node.key.name);

for (const expression of assignmentExpressions) {
const { left } = expression;
if (!left.computed) {
const name = left.property.name;
ekashida marked this conversation as resolved.
Show resolved Hide resolved
if (
ekashida marked this conversation as resolved.
Show resolved Hide resolved
PROPERTIES_THAT_REFLECT.has(name) &&
!classProperties.includes(name)
) {
context.report({
message: `Invariant violation: Setting "${name}" in the constructor results in a rendered attribute during construction. Change the name of this property, move this assignment to another lifecycle method, or override the default behavior by defining "${name}" as a property on the class.`,
node: expression,
});
}
}
}
}
},
};
},
};
22 changes: 17 additions & 5 deletions lib/util/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,26 @@
* @param {Context} context The Eslint context.
*/
function isComponent(node, context) {
if (!node.superClass) {
const { superClass } = node;

if (!superClass) {
return false;
}

// Checking if the class extends `Element` covers 99% of the LWC cases. We can make this safer
// in the future by ensuring that the `superClass` identifier is imported from engine.
const sourceCode = context.getSourceCode();
return sourceCode.getText(node.superClass) === 'LightningElement';
const program = context.getAncestors(node).find(({ type }) => type === 'Program');

const importDeclaration = program.body
.filter(({ type }) => type === 'ImportDeclaration')
.find(({ source }) => source.value === 'lwc');
ekashida marked this conversation as resolved.
Show resolved Hide resolved

if (importDeclaration) {
const importSpecifier = importDeclaration.specifiers.find(
({ imported }) => imported.name === 'LightningElement',
);
return superClass.name === importSpecifier.local.name;
}

return false;
}

module.exports = {
Expand Down
53 changes: 40 additions & 13 deletions test/lib/rules/consistent-component-name.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,10 @@ const ruleTester = new RuleTester(ESLINT_TEST_CONFIG);
ruleTester.run('consistent-class-name', rule, {
valid: [
{
code: `export class Foo extends LightningElement {}`,
code: `
import { LightningElement } from 'lwc';
export class Foo extends LightningElement {}
`,
filename: 'foo.js',
},
{
Expand All @@ -26,52 +29,76 @@ ruleTester.run('consistent-class-name', rule, {
],
invalid: [
{
code: `export default class extends LightningElement {}`,
code: `
import { LightningElement } from 'lwc';
export default class extends LightningElement {}
`,
filename: 'foo.js',
errors: [
{
type: 'ClassDeclaration',
column: 16,
column: 32,
message: 'Lightning component class should be named "Foo".',
},
],
output: 'export default class Foo extends LightningElement {}',
output: `
import { LightningElement } from 'lwc';
export default class Foo extends LightningElement {}
`,
},
{
code: `export default class Foo extends LightningElement {}`,
code: `
import { LightningElement } from 'lwc';
export default class Foo extends LightningElement {}
`,
filename: 'bar.js',
errors: [
{
column: 16,
column: 32,
type: 'ClassDeclaration',
message: 'Lightning component class should be named "Bar".',
},
],
output: 'export default class Bar extends LightningElement {}',
output: `
import { LightningElement } from 'lwc';
export default class Bar extends LightningElement {}
`,
},
{
code: `export default class ComplexName extends LightningElement {}`,
code: `
import { LightningElement } from 'lwc';
export default class ComplexName extends LightningElement {}
`,
filename: 'superComplexName.js',
errors: [
{
column: 16,
column: 32,
type: 'ClassDeclaration',
message: 'Lightning component class should be named "SuperComplexName".',
},
],
output: 'export default class SuperComplexName extends LightningElement {}',
output: `
import { LightningElement } from 'lwc';
export default class SuperComplexName extends LightningElement {}
`,
},
{
code: `export default class complexName extends LightningElement {}`,
code: `
import { LightningElement } from 'lwc';
export default class complexName extends LightningElement {}
`,
filename: 'complexName.js',
errors: [
{
column: 16,
column: 32,
type: 'ClassDeclaration',
message: 'Lightning component class should be named "ComplexName".',
},
],
output: 'export default class ComplexName extends LightningElement {}',
output: `
import { LightningElement } from 'lwc';
export default class ComplexName extends LightningElement {}
`,
},
],
});
Loading