Skip to content

Commit

Permalink
fix: allow decorated properties with class fields (#201)
Browse files Browse the repository at this point in the history
Allows properties decorated with lit property decorators to have class
fields alongside.

This loosens the strictness of the rule under the assumption you have
set `useDefineForClassFields: false` in typescript.

If you use `declare` or `accessor`, those fields will already be ignored
by this rule.

Examples:

```ts
// Error
class X extends LitElement {
  fieldA;
  static properties = {fieldA: {type: String}};
}

// Works now, errored before
class X extends LitElement {
  @Property()
  fieldA;
}

// Worked before, works now
class X extends LitElement {
  @Property()
  declare fieldA;
}

// Worked before, works now
class X extends LitElement {
  declare fieldA;
  static properties = {fieldA: {type: String}};
}

// Worked before, works now
class X extends LitElement {
  @Property()
  accessor fieldA;
}
```

Fixes #193.
  • Loading branch information
43081j committed May 14, 2024
1 parent ab2fb54 commit 9fb7b66
Show file tree
Hide file tree
Showing 8 changed files with 608 additions and 411 deletions.
6 changes: 3 additions & 3 deletions docs/rules/no-classfield-shadowing.md
@@ -1,11 +1,11 @@
# Disallows class fields with same name as static properties
# Disallows class fields with same name as reactive properties

Class fields set with same names as static properties will not trigger updates as expected. They will overwrite
Class fields set with same names as reactive properties will not trigger updates as expected. They will overwrite
accessors used for detecting changes. See https://lit.dev/msg/class-field-shadowing for more information.

## Rule Details

This rule disallows class fields with same name as static properties.
This rule disallows class fields with same name as reactive properties.

The following patterns are considered warnings:

Expand Down
911 changes: 517 additions & 394 deletions package-lock.json

Large diffs are not rendered by default.

4 changes: 2 additions & 2 deletions package.json
Expand Up @@ -49,8 +49,8 @@
"eslint": ">= 5"
},
"devDependencies": {
"@babel/eslint-parser": "^7.18.9",
"@babel/plugin-proposal-decorators": "^7.18.10",
"@babel/eslint-parser": "^7.24.5",
"@babel/plugin-proposal-decorators": "^7.24.1",
"@types/chai": "^4.2.16",
"@types/eslint": "^8.4.6",
"@types/estree": "^1.0.0",
Expand Down
12 changes: 9 additions & 3 deletions src/rules/no-classfield-shadowing.ts
Expand Up @@ -5,7 +5,12 @@

import {Rule} from 'eslint';
import * as ESTree from 'estree';
import {getClassFields, getPropertyMap, isLitClass} from '../util';
import {
getClassFields,
getPropertyMap,
isLitClass,
hasLitPropertyDecorator
} from '../util';

//------------------------------------------------------------------------------
// Rule Definition
Expand All @@ -22,7 +27,7 @@ const rule: Rule.RuleModule = {
messages: {
noClassfieldShadowing:
'The {{ prop }} property is a class field which has the same name as ' +
'static property which could have unintended side-effects.'
'a reactive property which could have unintended side-effects.'
}
},

Expand All @@ -34,7 +39,8 @@ const rule: Rule.RuleModule = {
const classMembers = getClassFields(node);

for (const [prop, {key}] of propertyMap.entries()) {
if (classMembers.has(prop)) {
const member = classMembers.get(prop);
if (member && !hasLitPropertyDecorator(member)) {
context.report({
node: key,
messageId: 'noClassfieldShadowing',
Expand Down
4 changes: 1 addition & 3 deletions src/test/rules/attribute-names_test.ts
Expand Up @@ -17,9 +17,7 @@ const parser = require.resolve('@babel/eslint-parser');
const parserOptions = {
requireConfigFile: false,
babelOptions: {
plugins: [
['@babel/plugin-proposal-decorators', {decoratorsBeforeExport: true}]
]
plugins: [['@babel/plugin-proposal-decorators', {version: '2023-11'}]]
}
};

Expand Down
44 changes: 43 additions & 1 deletion src/test/rules/no-classfield-shadowing_test.ts
Expand Up @@ -21,6 +21,23 @@ const ruleTester = new RuleTester({
}
});

const parser = require.resolve('@babel/eslint-parser');
const parserOptions = {
requireConfigFile: false,
babelOptions: {
plugins: [
[
'@babel/plugin-proposal-decorators',
{
version: '2023-11'
}
]
]
}
};

const tsParser = require.resolve('@typescript-eslint/parser');

ruleTester.run('no-classfield-shadowing', rule, {
valid: [
`class MyElement extends LitElement {
Expand All @@ -39,7 +56,32 @@ ruleTester.run('no-classfield-shadowing', rule, {
properties = {
foo: { type: String }
}
}`
}`,
{
code: `class MyElement extends LitElement {
@property()
foo;
}`,
parser,
parserOptions
},
{
code: `class MyElement extends LitElement {
@property()
accessor foo;
}`,
parser,
parserOptions
},
{
code: `class MyElement extends LitElement {
declare foo;
static properties = {
foo: { type: String }
};
}`,
parser: tsParser
}
],

invalid: [
Expand Down
4 changes: 1 addition & 3 deletions src/test/rules/no-property-change-update_test.ts
Expand Up @@ -25,9 +25,7 @@ const parser = require.resolve('@babel/eslint-parser');
const parserOptions = {
requireConfigFile: false,
babelOptions: {
plugins: [
['@babel/plugin-proposal-decorators', {decoratorsBeforeExport: true}]
]
plugins: [['@babel/plugin-proposal-decorators', {version: '2023-11'}]]
}
};

Expand Down
34 changes: 32 additions & 2 deletions src/util.ts
Expand Up @@ -9,6 +9,10 @@ export interface BabelProperty extends ESTree.MethodDefinition {
decorators?: BabelDecorator[];
}

export type DecoratedNode = ESTree.Node & {
decorators?: BabelDecorator[];
};

/**
* Returns if given node has a lit identifier
* @param {ESTree.Node} node
Expand Down Expand Up @@ -139,6 +143,9 @@ export function getClassFields(
return result;
}

const propertyDecorators = ['state', 'property', 'internalProperty'];
const internalDecorators = ['state', 'internalProperty'];

/**
* Get the properties object of an element class
*
Expand All @@ -149,8 +156,6 @@ export function getPropertyMap(
node: ESTree.Class
): ReadonlyMap<string, PropertyMapEntry> {
const result = new Map<string, PropertyMapEntry>();
const propertyDecorators = ['state', 'property', 'internalProperty'];
const internalDecorators = ['state', 'internalProperty'];

for (const member of node.body.body) {
if (
Expand Down Expand Up @@ -243,6 +248,31 @@ export function getPropertyMap(
return result;
}

/**
* Determines if a node has a lit property decorator
* @param {ESTree.Node} node Node to test
* @return {boolean}
*/
export function hasLitPropertyDecorator(node: ESTree.Node): boolean {
const decoratedNode = node as DecoratedNode;

if (!decoratedNode.decorators || !Array.isArray(decoratedNode.decorators)) {
return false;
}

for (const decorator of decoratedNode.decorators) {
if (
decorator.expression.type === 'CallExpression' &&
decorator.expression.callee.type === 'Identifier' &&
propertyDecorators.includes(decorator.expression.callee.name)
) {
return true;
}
}

return false;
}

/**
* Generates a placeholder string for a given quasi
*
Expand Down

0 comments on commit 9fb7b66

Please sign in to comment.