Skip to content

Commit

Permalink
[eslint-plugin] Add attribute names rule (#4516)
Browse files Browse the repository at this point in the history
  • Loading branch information
justinfagnani committed Feb 1, 2024
1 parent 9c7dba2 commit c51bc18
Show file tree
Hide file tree
Showing 15 changed files with 321 additions and 187 deletions.
5 changes: 5 additions & 0 deletions .changeset/tender-pans-count.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@lit-labs/testing': patch
---

Update @web/test-runner-commands dependency
1 change: 1 addition & 0 deletions packages/labs/analyzer/src/lib/javascript/functions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,7 @@ const getParameter = (
)[0];
const p: Parameter = {
name: param.name.getText(),
node: param,
type: getTypeForNode(param, analyzer),
...(paramTag ? parseJSDocDescription(paramTag, analyzer) : {}),
optional: false,
Expand Down
2 changes: 1 addition & 1 deletion packages/labs/analyzer/src/lib/javascript/modules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -305,7 +305,7 @@ const getJSPathFromSourcePath = (
) => {
sourcePath = analyzer.path.normalize(sourcePath) as AbsolutePath;
// If the source file was already JS, just return that
if (sourcePath.endsWith('js')) {
if (sourcePath.endsWith('.js')) {
return sourcePath;
}
// TODO(kschaaf): If the source file was a declaration file, this means we're
Expand Down
23 changes: 16 additions & 7 deletions packages/labs/analyzer/src/lib/lit-element/properties.ts
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ export const getProperties = (
const options = getPropertyOptions(ts, propertyDecorator);
reactiveProperties.set(name, {
name,
node: prop,
optionsNode: options,
type: getTypeForNode(prop, analyzer),
attribute: getPropertyAttribute(ts, options, name),
typeOption: getPropertyType(ts, options),
Expand Down Expand Up @@ -131,6 +133,8 @@ const addPropertiesFromStaticBlock = (
const nodeForType = undecoratedProperties.get(name);
reactiveProperties.set(name, {
name,
node: prop,
optionsNode: options,
type:
nodeForType !== undefined
? getTypeForNode(nodeForType, analyzer)
Expand Down Expand Up @@ -246,18 +250,23 @@ const addConstructorInitializers = (

/**
* Gets the `attribute` property of a property options object as a string.
*
* The attribute value returned is the value that is used at runtime by
* ReactiveElement, not the raw option. If the attribute property option is
* not given or is `true`, the lower-cased property name is used. If the
* attribute property option is `false`, `undefined` is returned.
*/
export const getPropertyAttribute = (
ts: TypeScript,
obj: ts.ObjectLiteralExpression | undefined,
propName: string
optionsNode: ts.ObjectLiteralExpression | undefined,
propertyName: string
) => {
if (obj === undefined) {
return propName.toLowerCase();
if (optionsNode === undefined) {
return propertyName.toLowerCase();
}
const attributeProperty = getObjectProperty(ts, obj, 'attribute');
const attributeProperty = getObjectProperty(ts, optionsNode, 'attribute');
if (attributeProperty === undefined) {
return propName.toLowerCase();
return propertyName.toLowerCase();
}
const {initializer} = attributeProperty;
if (ts.isStringLiteral(initializer)) {
Expand All @@ -271,7 +280,7 @@ export const getPropertyAttribute = (
(ts.isIdentifier(initializer) && initializer.text === 'undefined') ||
initializer.kind === ts.SyntaxKind.UndefinedKeyword
) {
return propName.toLowerCase();
return propertyName.toLowerCase();
}
return undefined;
};
Expand Down
13 changes: 13 additions & 0 deletions packages/labs/analyzer/src/lib/model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -637,6 +637,7 @@ export interface LitElementExport extends LitElementDeclaration {

export interface PropertyLike extends DeprecatableDescribed {
name: string;
node: ts.Node;
type: Type | undefined;
default?: string | undefined;
}
Expand All @@ -648,11 +649,23 @@ export interface Return {
}

export interface Parameter extends PropertyLike {
node: ts.ParameterDeclaration;
optional?: boolean | undefined;
rest?: boolean | undefined;
}

export interface ReactiveProperty extends PropertyLike {
/**
* The property declaration.
*
* A ts.PropertyDeclaration if the property was declared as a class field,
* or a ts.PropertyAssignment if the property was declared in a static
* properties block.
*/
node: ts.PropertyDeclaration | ts.PropertyAssignment;

optionsNode: ts.ObjectLiteralExpression | undefined;

reflect: boolean;

// TODO(justinfagnani): should we convert into attribute name?
Expand Down
2 changes: 1 addition & 1 deletion packages/labs/eslint-plugin/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
"clean": "if-file-deleted"
},
"test": {
"command": "node --enable-source-maps --test-reporter=dot --test test/**/*_test.js",
"command": "node --enable-source-maps --test-reporter=spec --test test/**/*_test.js",
"dependencies": [
"build"
],
Expand Down
11 changes: 3 additions & 8 deletions packages/labs/eslint-plugin/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,14 @@
* SPDX-License-Identifier: BSD-3-Clause
*/

import type {Rule, ESLint} from 'eslint';
import {noKevin} from './rules/no-kevin.js';
import type {ESLint} from 'eslint';

export const rules = {
'no-kevin': noKevin as unknown as Rule.RuleModule,
};
export const rules = {};

export const configs: ESLint.Plugin['configs'] = {
all: {
plugins: ['@lit-labs/eslint-plugin'],
rules: {
'@lit-labs/no-kevin': 'error',
},
rules: {},
},
recommended: {
plugins: ['@lit-labs/eslint-plugin'],
Expand Down
65 changes: 65 additions & 0 deletions packages/labs/eslint-plugin/src/lib/util.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
/**
* @license
* Copyright 2024 Google LLC
* SPDX-License-Identifier: BSD-3-Clause
*/

import {AbsolutePath, Analyzer} from '@lit-labs/analyzer';
import {
ESLintUtils,
ParserServicesWithTypeInformation,
} from '@typescript-eslint/utils';
// TODO (justinfagnani): get from ESLintUtils if possible?
// See q: https://discord.com/channels/1026804805894672454/1084238921677946992/1199174067459198986
import ts from 'typescript';
import * as path from 'path';

export const createRule = ESLintUtils.RuleCreator(
// TODO (justinfagnani): set up rule doc publishing pipeline for lit.dev
(name: string) => `https://lit.dev/eslint-plugin/${name}`
);

const analyzerByProgram = new WeakMap<ts.Program, Analyzer>();

export const getAnalyzer = (services: ParserServicesWithTypeInformation) => {
const program = services.program;
let analyzer = analyzerByProgram.get(program);
if (analyzer === undefined) {
analyzer = new Analyzer({
typescript: ts,
getProgram: () => program,
fs: ts.sys,
path,
});
}
return analyzer;
};

export const getDeclarationForNode = (
analyzer: Analyzer,
filename: string,
node: ts.Node
) => {
const modulePath = analyzer.fs.useCaseSensitiveFileNames
? filename
: filename.toLocaleLowerCase();
const module = analyzer.getModule(modulePath as AbsolutePath);
for (const declaration of module.declarations) {
if (declaration.node === node) {
return declaration;
}
}
return undefined;
};

export const isTrueLiteral = (e: ts.Expression): e is ts.TrueLiteral =>
e.kind === ts.SyntaxKind.TrueKeyword;

export const isFalseLiteral = (e: ts.Expression): e is ts.FalseLiteral =>
e.kind === ts.SyntaxKind.FalseKeyword;

// TODO (justinfagnani): add a type predicate?
// When is `undefined` ever not an identifier?
export const isUndefinedLiteral = (e: ts.Expression) =>
(ts.isIdentifier(e) && e.text === 'undefined') ||
e.kind === ts.SyntaxKind.UndefinedKeyword;
111 changes: 111 additions & 0 deletions packages/labs/eslint-plugin/src/rules/attribute-names.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
/**
* @license
* Copyright 2024 Google LLC
* SPDX-License-Identifier: BSD-3-Clause
*/

/**
* @fileoverview Enforces attribute naming conventions
*/

import {ESLintUtils} from '@typescript-eslint/utils';
import ts from 'typescript';
import {
createRule,
getAnalyzer,
getDeclarationForNode,
isFalseLiteral,
isTrueLiteral,
isUndefinedLiteral,
} from '../lib/util.js';

export const attributeNames = createRule({
name: 'attribute-names',
meta: {
type: 'problem',
docs: {
description: 'Enforces attribute naming conventions',
recommended: 'recommended',
},
schema: [],
messages: {
casedAttribute:
'Attributes are case-insensitive and therefore should be ' +
'defined in lower case',
casedPropertyWithoutAttribute:
'Property has non-lowercase casing but no attribute. It should ' +
'instead have an explicit `attribute` set to the lower case ' +
'name (usually snake-case)',
},
},

create(context) {
const services = ESLintUtils.getParserServices(context);
const filename = context.filename;
const analyzer = getAnalyzer(services);

return {
ClassDeclaration(node) {
const tsNode = services.esTreeNodeToTSNodeMap.get(node);
const declaration = getDeclarationForNode(analyzer, filename, tsNode);
if (
declaration === undefined ||
!declaration.isLitElementDeclaration()
) {
return;
}
const properties = declaration.reactiveProperties;

for (const [propertyName, property] of properties.entries()) {
const attributeOptionNode = property.optionsNode?.properties.find(
(p): p is ts.PropertyAssignment =>
ts.isPropertyAssignment(p) &&
ts.isIdentifier(p.name) &&
p.name.text === 'attribute'
);

// TODO (justinfagnani): handle other statically known truthy and
// undefined values? This would let attribute names be in const
// variables, etc., but still be lintable.
if (
attributeOptionNode === undefined ||
isTrueLiteral(attributeOptionNode.initializer) ||
isUndefinedLiteral(attributeOptionNode.initializer)
) {
if (propertyName.toLowerCase() !== propertyName) {
// Report on the whole property declaration, since there's no
// attribute option
const estreeNode = services.tsNodeToESTreeNodeMap.get(
property.node
);
context.report({
node: estreeNode,
messageId: 'casedPropertyWithoutAttribute',
});
}
} else if (isFalseLiteral(attributeOptionNode.initializer)) {
continue;
} else if (ts.isStringLiteral(attributeOptionNode.initializer)) {
const attributeName = attributeOptionNode.initializer.text;
if (attributeName.toLowerCase() !== attributeName) {
// Report on just the attribute option
const estreeNode =
services.tsNodeToESTreeNodeMap.get(attributeOptionNode);
context.report({
node: estreeNode,
messageId: 'casedAttribute',
});
}
} else {
// Unsupported attribute option.
// TODO (justinfagnani): Report?
}
}
},
};
},
defaultOptions: [] as ReadonlyArray<unknown>,
});

// TODO (justinfagnani): Is this necessary?
export default attributeNames;
40 changes: 0 additions & 40 deletions packages/labs/eslint-plugin/src/rules/no-kevin.ts

This file was deleted.

0 comments on commit c51bc18

Please sign in to comment.