Fix ESLint component naming check with multiple components
This CL updates the check_component_naming rule to make it deal with a
file that has multiple components defined; something that would
previously cause the rule to fail and hence it was disabled in some
files.
Fixed: 1226741
Change-Id: I3eb282182c10e3faf120d6bcd2d0846667524861
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/3141471
Commit-Queue: Jack Franklin <jacktfranklin@chromium.org>
Auto-Submit: Jack Franklin <jacktfranklin@chromium.org>
Reviewed-by: Andres Olivares <andoli@chromium.org>
diff --git a/front_end/panels/application/components/OriginTrialTreeView.ts b/front_end/panels/application/components/OriginTrialTreeView.ts
index de27222..78c9f9b 100644
--- a/front_end/panels/application/components/OriginTrialTreeView.ts
+++ b/front_end/panels/application/components/OriginTrialTreeView.ts
@@ -2,7 +2,6 @@
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.
-// eslint-disable-next-line rulesdir/check_component_naming
import * as i18n from '../../../core/i18n/i18n.js';
import * as Protocol from '../../../generated/protocol.js';
import * as Adorners from '../../../ui/components/adorners/adorners.js';
@@ -204,7 +203,7 @@
}
export class OriginTrialTokenRows extends HTMLElement {
- static litTagName = LitHtml.literal`devtools-resources-origin-trial-token-rows`;
+ static readonly litTagName = LitHtml.literal`devtools-resources-origin-trial-token-rows`;
private readonly shadow = this.attachShadow({mode: 'open'});
private tokenWithStatus: Protocol.Page.OriginTrialTokenWithStatus|null = null;
private parsedTokenDetails: TokenField[] = [];
@@ -301,7 +300,7 @@
}
export class OriginTrialTreeView extends HTMLElement {
- static litTagName = LitHtml.literal`devtools-resources-origin-trial-tree-view`;
+ static readonly litTagName = LitHtml.literal`devtools-resources-origin-trial-tree-view`;
private readonly shadow = this.attachShadow({mode: 'open'});
set data(data: OriginTrialTreeViewData) {
@@ -330,3 +329,11 @@
}
ComponentHelpers.CustomElements.defineComponent('devtools-resources-origin-trial-tree-view', OriginTrialTreeView);
+
+declare global {
+ interface HTMLElementTagNameMap {
+ 'devtools-resources-origin-trial-tree-view': OriginTrialTreeView;
+ 'devtools-resources-origin-trial-token-rows': OriginTrialTokenRows;
+ 'devtools-resources-origin-trial-tree-view-badge': Badge;
+ }
+}
diff --git a/front_end/panels/application/components/StackTrace.ts b/front_end/panels/application/components/StackTrace.ts
index bc1e2da..209ec8d 100644
--- a/front_end/panels/application/components/StackTrace.ts
+++ b/front_end/panels/application/components/StackTrace.ts
@@ -207,8 +207,6 @@
}
}
-// TODO(jacktfranklin): re-enable once https://crbug.com/1226741 is resolved.
-// eslint-disable-next-line rulesdir/check_component_naming
ComponentHelpers.CustomElements.defineComponent('devtools-stack-trace-row', StackTraceRow);
ComponentHelpers.CustomElements.defineComponent('devtools-stack-trace-link-button', StackTraceLinkButton);
ComponentHelpers.CustomElements.defineComponent('devtools-resources-stack-trace', StackTrace);
diff --git a/front_end/panels/application/components/TrustTokensView.ts b/front_end/panels/application/components/TrustTokensView.ts
index b6c4a6b..9082eb6 100644
--- a/front_end/panels/application/components/TrustTokensView.ts
+++ b/front_end/panels/application/components/TrustTokensView.ts
@@ -196,8 +196,6 @@
return s.replace(/\/$/, '');
}
-// TODO(jacktfranklin): re-enable once https://crbug.com/1226741 is resolved.
-// eslint-disable-next-line rulesdir/check_component_naming
ComponentHelpers.CustomElements.defineComponent('devtools-trust-tokens-delete-button', TrustTokensDeleteButton);
ComponentHelpers.CustomElements.defineComponent('devtools-trust-tokens-storage-view', TrustTokensView);
diff --git a/scripts/eslint_rules/lib/check_component_naming.js b/scripts/eslint_rules/lib/check_component_naming.js
index cf31ebf..86f57a6 100644
--- a/scripts/eslint_rules/lib/check_component_naming.js
+++ b/scripts/eslint_rules/lib/check_component_naming.js
@@ -14,11 +14,9 @@
fixable: 'code',
schema: [],
messages: {
- nonMatchingTagName:
- 'Found inconsistent tag names in the component definition. Make sure the static litTagName, the tag in the defineComponent() call and the TS interface key are all identical.',
noStaticTagName: 'Found a component class that does not define a static litTagName property.',
- noTSInterface: 'Could not find the TS interface declaration for the component.',
- noDefineCall: 'Could not find a defineComponent() call for the component.',
+ noTSInterface: 'Could not find the TS interface declaration for the component {{ tagName }}.',
+ noDefineCall: 'Could not find a defineComponent() call for the component {{ tagName }}.',
defineCallNonLiteral: 'defineComponent() first argument must be a string literal.',
staticLiteralInvalid: 'static readonly litTagName must use a literal string, with no interpolation.',
litTagNameNotLiteral:
@@ -31,20 +29,17 @@
return node.type === 'ClassDeclaration' && node.superClass && node.superClass.name === 'HTMLElement';
}
- function findComponentClassDefinition(programNode) {
- const nodeWithClassDeclaration = programNode.body.find(node => {
+ function findAllComponentClassDefinitions(programNode) {
+ const nodesWithClassDeclaration = programNode.body.filter(node => {
if (node.type === 'ExportNamedDeclaration' && node.declaration) {
return nodeIsHTMLElementClassDeclaration(node.declaration);
}
return nodeIsHTMLElementClassDeclaration(node);
});
- if (!nodeWithClassDeclaration) {
- return null;
- }
-
- return nodeWithClassDeclaration.type === 'ExportNamedDeclaration' ? nodeWithClassDeclaration.declaration :
- nodeWithClassDeclaration;
+ return nodesWithClassDeclaration.map(node => {
+ return node.type === 'ExportNamedDeclaration' ? node.declaration : node;
+ });
}
function findComponentTagNameFromStaticProperty(classBodyNode) {
@@ -53,8 +48,8 @@
});
}
- function findCustomElementsDefineComponentCall(programNode) {
- return programNode.body.find(node => {
+ function findCustomElementsDefineComponentCalls(programNode) {
+ return programNode.body.filter(node => {
if (node.type !== 'ExpressionStatement') {
return false;
}
@@ -69,7 +64,7 @@
});
}
- function findTypeScriptDeclareGlobalHTMLInterfaceCall(programNode) {
+ function findTypeScriptDeclareGlobalHTMLInterfaceCalls(programNode) {
const matchingNode = programNode.body.find(node => {
// matches `declare global {}`
const isGlobalCall = node.type === 'TSModuleDeclaration' && node.id.name === 'global';
@@ -83,89 +78,110 @@
});
if (!matchingNode) {
- return undefined;
+ return [];
}
- return matchingNode.body.body.find(node => {
+ return matchingNode.body.body.filter(node => {
return node.type === 'TSInterfaceDeclaration' && node.id.name === 'HTMLElementTagNameMap';
});
}
+ /** @type {Set<{classNode: any, tagName: string}>} */
+ const componentClassDefinitionLitTagNamesFound = new Set();
+
+ /** @type {Set<string>} */
+ const defineComponentCallsFound = new Set();
+
+ /** @type {Set<string>} */
+ const tsInterfaceExtendedEntriesFound = new Set();
+
return {
Program(node) {
- const componentClassDefinition = findComponentClassDefinition(node);
- if (!componentClassDefinition) {
+ /* We allow there to be multiple components defined in one file.
+ * Therefore, this rule gathers all nodes relating to the component
+ * definition and then checks that for each class found that extends
+ * HTMLElement, we have:
+ * 1) the class with a static readonly litTagName
+ * 2) The call to defineComponent
+ * 3) The global interface extension
+ * And that for each of those, the component name string (e.g. 'devtools-foo') is the same.
+ */
+
+ const componentClassDefinitions = findAllComponentClassDefinitions(node);
+ if (componentClassDefinitions.length === 0) {
+ // No components found, our work is done!
return;
}
- const componentTagNameNode = findComponentTagNameFromStaticProperty(componentClassDefinition.body);
- if (!componentTagNameNode) {
- context.report({node: componentClassDefinition, messageId: 'noStaticTagName'});
- return;
- }
- if (componentTagNameNode.value.type !== 'TaggedTemplateExpression') {
- // This means that the value is not LitHtml.literal``. Most likely
- // the user forgot to add LitHtml.literal and has defined the value
- // as a regular string.
- context.report({node: componentTagNameNode, messageId: 'litTagNameNotLiteral'});
- return;
- }
- if (componentTagNameNode.value.quasi.quasis.length > 1) {
- // This means that there's >1 template parts, which means they are
- // being split by an interpolated value, which is not allowed.
- context.report({node: componentTagNameNode, messageId: 'staticLiteralInvalid'});
- return;
+ // Do some checks on the component classes and store the component names that we've found.
+ for (const componentClassDefinition of componentClassDefinitions) {
+ const componentTagNameNode = findComponentTagNameFromStaticProperty(componentClassDefinition.body);
+ if (!componentTagNameNode) {
+ context.report({node: componentClassDefinition, messageId: 'noStaticTagName'});
+ return;
+ }
+ if (componentTagNameNode.value.type !== 'TaggedTemplateExpression') {
+ // This means that the value is not LitHtml.literal``. Most likely
+ // the user forgot to add LitHtml.literal and has defined the value
+ // as a regular string.
+ context.report({node: componentTagNameNode, messageId: 'litTagNameNotLiteral'});
+ return;
+ }
+ if (componentTagNameNode.value.quasi.quasis.length > 1) {
+ // This means that there's >1 template parts, which means they are
+ // being split by an interpolated value, which is not allowed.
+ context.report({node: componentTagNameNode, messageId: 'staticLiteralInvalid'});
+ return;
+ }
+
+ // Enforce that the property is declared as readonly (static readonly litTagName = ...)
+ if (!componentTagNameNode.readonly) {
+ context.report({node: componentTagNameNode, messageId: 'staticLiteralNotReadonly'});
+ return;
+ }
+
+ // Grab the name of the component, e.g:
+ // LitHtml.literal`devtools-foo` will pull "devtools-foo" out.
+ const componentTagName = componentTagNameNode.value.quasi.quasis[0].value.cooked;
+ componentClassDefinitionLitTagNamesFound.add(
+ {tagName: componentTagName, classNode: componentClassDefinition});
}
- // Enforce that the property is declared as readonly (static readonly litTagName = ...)
- if (!componentTagNameNode.readonly) {
- context.report({node: componentTagNameNode, messageId: 'staticLiteralNotReadonly'});
- return;
- }
-
- // Grab the name of the component, e.g:
- // LitHtml.literal`devtools-foo` will pull "devtools-foo" out.
- const componentTagName = componentTagNameNode.value.quasi.quasis[0].value.cooked;
+ // Find all defineComponent() calls and store the arguments to them.
// Now find the CustomElements.defineComponent() line
- const customElementsDefineCall = findCustomElementsDefineComponentCall(node);
- if (!customElementsDefineCall) {
- context.report({node, messageId: 'noDefineCall'});
- return;
+ const customElementsDefineCalls = findCustomElementsDefineComponentCalls(node);
+ for (const customElementsDefineCall of customElementsDefineCalls) {
+ const firstArgumentToDefineCall = customElementsDefineCall.expression.arguments[0];
+ if (!firstArgumentToDefineCall || firstArgumentToDefineCall.type !== 'Literal') {
+ context.report({
+ node: customElementsDefineCall,
+ messageId: 'defineCallNonLiteral',
+ });
+ return;
+ }
+ const tagNamePassedToDefineCall = firstArgumentToDefineCall.value;
+ defineComponentCallsFound.add(tagNamePassedToDefineCall);
+ }
+ const allTSDeclareGlobalInterfaceCalls = findTypeScriptDeclareGlobalHTMLInterfaceCalls(node);
+
+ for (const interfaceDeclaration of allTSDeclareGlobalInterfaceCalls) {
+ for (const node of interfaceDeclaration.body.body) {
+ if (node.type === 'TSPropertySignature') {
+ tsInterfaceExtendedEntriesFound.add(node.key.value);
+ }
+ }
}
- const firstArgumentToDefineCall = customElementsDefineCall.expression.arguments[0];
- if (!firstArgumentToDefineCall || firstArgumentToDefineCall.type !== 'Literal') {
- context.report({
- node: customElementsDefineCall,
- messageId: 'defineCallNonLiteral',
- });
- return;
- }
- const tagNamePassedToDefineCall = firstArgumentToDefineCall.value;
-
- if (tagNamePassedToDefineCall !== componentTagName) {
- context.report({node: customElementsDefineCall, messageId: 'nonMatchingTagName'});
- return;
- }
-
- const typeScriptInterfaceDeclare = findTypeScriptDeclareGlobalHTMLInterfaceCall(node);
- if (!typeScriptInterfaceDeclare) {
- context.report({
- node,
- messageId: 'noTSInterface',
- });
- return;
- }
- const keyForComponent = typeScriptInterfaceDeclare.body.body.find(node => {
- // Find the declaration line for the component tag name.
- return node.type === 'TSPropertySignature' && node.key.value === componentTagName;
- });
- // If we didn't find it, that means it's either not there or it's there
- // but doesn't have the right name, so error.
- if (!keyForComponent) {
- context.report({node: typeScriptInterfaceDeclare, messageId: 'nonMatchingTagName'});
- return;
+ for (const foundComponentClass of componentClassDefinitionLitTagNamesFound) {
+ const {tagName, classNode} = foundComponentClass;
+ // Check that each tagName has a matching entry in both other places we expect it.
+ if (!defineComponentCallsFound.has(tagName)) {
+ context.report({node: classNode, messageId: 'noDefineCall', data: {tagName}});
+ }
+ if (!tsInterfaceExtendedEntriesFound.has(tagName)) {
+ context.report({node: classNode, messageId: 'noTSInterface', data: {tagName}});
+ }
}
// if we got here, everything is valid and the name is correct in all three locations
diff --git a/scripts/eslint_rules/tests/check_component_naming_test.js b/scripts/eslint_rules/tests/check_component_naming_test.js
index 8b6ae18..cc86007 100644
--- a/scripts/eslint_rules/tests/check_component_naming_test.js
+++ b/scripts/eslint_rules/tests/check_component_naming_test.js
@@ -27,6 +27,27 @@
filename: 'front_end/ui/components/Foo.ts'
},
{
+ // Multiple components in one file is valid.
+ code: `export class Foo extends HTMLElement {
+ static readonly litTagName = LitHtml.literal\`devtools-foo\`
+ }
+
+ export class Bar extends HTMLElement {
+ static readonly litTagName = LitHtml.literal\`devtools-bar\`
+ }
+
+ ComponentHelpers.CustomElements.defineComponent('devtools-bar', Bar);
+ ComponentHelpers.CustomElements.defineComponent('devtools-foo', Foo);
+
+ declare global {
+ interface HTMLElementTagNameMap {
+ 'devtools-foo': Foo
+ 'devtools-bar': Bar
+ }
+ }`,
+ filename: 'front_end/ui/components/Foo.ts'
+ },
+ {
code: `export class Foo extends HTMLElement {
static readonly litTagName = LitHtml.literal\`devtools-foo\`
}
@@ -109,10 +130,7 @@
}
}`,
filename: 'front_end/ui/components/Foo.ts',
- errors: [{
- messageId: 'nonMatchingTagName'
-
- }]
+ errors: [{messageId: 'noDefineCall'}]
},
// defineComponent call uses non literal
{
@@ -165,7 +183,7 @@
}
}`,
filename: 'front_end/ui/components/Foo.ts',
- errors: [{messageId: 'nonMatchingTagName'}]
+ errors: [{messageId: 'noTSInterface'}]
},
// TS interface is missing
{
@@ -175,7 +193,7 @@
ComponentHelpers.CustomElements.defineComponent('devtools-foo', Foo);`,
filename: 'front_end/ui/components/Foo.ts',
- errors: [{messageId: 'noTSInterface', data: {componentName: 'devtools-foo'}}]
+ errors: [{messageId: 'noTSInterface', data: {tagName: 'devtools-foo'}}]
},
{
// Not using LitHtml.literal
@@ -193,5 +211,27 @@
filename: 'front_end/ui/components/Foo.ts',
errors: [{messageId: 'litTagNameNotLiteral'}]
},
+ {
+ // Multiple components in one file is valid.
+ // But here devtools-foo is fine, but devtools-bar is missing a define call
+ code: `export class Foo extends HTMLElement {
+ static readonly litTagName = LitHtml.literal\`devtools-foo\`
+ }
+
+ export class Bar extends HTMLElement {
+ static readonly litTagName = LitHtml.literal\`devtools-bar\`
+ }
+
+ ComponentHelpers.CustomElements.defineComponent('devtools-foo', Foo);
+
+ declare global {
+ interface HTMLElementTagNameMap {
+ 'devtools-foo': Foo
+ 'devtools-bar': Bar
+ }
+ }`,
+ filename: 'front_end/ui/components/Foo.ts',
+ errors: [{messageId: 'noDefineCall', data: {tagName: 'devtools-bar'}}]
+ },
]
});