From 4eac8f82318f81fdae1360ceaa5241d27d984fe3 Mon Sep 17 00:00:00 2001 From: Nikolay Vitkov Date: Fri, 28 Mar 2025 22:02:08 +0100 Subject: [PATCH] [eslint] Run type checking on custom rules Currently behind a flag, as there are a lot of error. This CL fixes some of them. Bug: 407085691 Change-Id: I736472ff5a9d8c46e45a1ce89a42199774d9075f Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/6410471 Auto-Submit: Nikolay Vitkov Reviewed-by: Danil Somsikov Commit-Queue: Nikolay Vitkov --- .gitignore | 1 + scripts/eslint_rules/lib/check-css-import.js | 2 +- .../eslint_rules/lib/check-license-header.js | 2 +- .../lib/check-test-definitions.js | 8 ++- .../lib/check-was-shown-methods.js | 37 ++++++++-- .../lib/enforce-default-import-name.js | 7 +- .../lib/enforce-ui-strings-as-const.js | 11 ++- scripts/eslint_rules/lib/es-modules-import.js | 24 ++++--- .../lib/inject-checkbox-styles.js | 2 +- .../eslint_rules/lib/l10n-filename-matches.js | 4 +- scripts/eslint_rules/lib/no-assert-equal.js | 69 +++++++++++-------- .../eslint_rules/lib/no-imperative-dom-api.js | 22 +++--- .../dom-api-devtools-extensions.js | 7 +- .../lib/no-imperative-dom-api/dom-api.js | 7 +- .../lib/no-imperative-dom-api/dom-fragment.js | 33 +++++++-- .../lib/no-imports-in-directory.js | 4 +- .../lib/no-new-lit-element-components.js | 2 +- .../lib/set-data-type-reference.js | 9 +-- .../lib/static-custom-event-names.js | 7 +- .../lib/trace-engine-test-timeouts.js | 59 +++++++++------- .../tests/check-test-definitions.test.js | 20 ++---- .../tests/jslog-context-list.test.js | 3 +- .../tests/no-imports-in-directory.test.js | 1 - scripts/eslint_rules/tests/utils.test.js | 41 ++++++----- scripts/eslint_rules/tests/utils/utils.js | 2 +- scripts/eslint_rules/tsconfig.json | 12 ++++ scripts/test/run_lint_check.mjs | 68 ++++++++++++++++++ 27 files changed, 319 insertions(+), 145 deletions(-) create mode 100644 scripts/eslint_rules/tsconfig.json diff --git a/.gitignore b/.gitignore index e0e06040b5..841d4565c9 100644 --- a/.gitignore +++ b/.gitignore @@ -51,3 +51,4 @@ test/perf/.generated # Linters caches .eslintcache .stylelintcache +**/tsconfig.tsbuildinfo diff --git a/scripts/eslint_rules/lib/check-css-import.js b/scripts/eslint_rules/lib/check-css-import.js index 9fae332cd8..93796cee50 100644 --- a/scripts/eslint_rules/lib/check-css-import.js +++ b/scripts/eslint_rules/lib/check-css-import.js @@ -33,7 +33,7 @@ module.exports = { const filename = context.filename ?? context.getFilename(); return { ImportDeclaration(node) { - const importPath = path.normalize(node.source.value); + const importPath = path.normalize(`${node.source.value}`); if (importPath.endsWith('.css.js')) { const importingFileName = path.resolve(filename); diff --git a/scripts/eslint_rules/lib/check-license-header.js b/scripts/eslint_rules/lib/check-license-header.js index ff728f2e29..75b4ff162b 100644 --- a/scripts/eslint_rules/lib/check-license-header.js +++ b/scripts/eslint_rules/lib/check-license-header.js @@ -19,7 +19,7 @@ const FRONT_END_FOLDER = path.join( 'front_end', ); -const CURRENT_YEAR = new Date().getFullYear(); +const CURRENT_YEAR = `${new Date().getFullYear()}`; const LINE_LICENSE_HEADER = [ `Copyright ${CURRENT_YEAR} The Chromium Authors. All rights reserved.`, 'Use of this source code is governed by a BSD-style license that can be', diff --git a/scripts/eslint_rules/lib/check-test-definitions.js b/scripts/eslint_rules/lib/check-test-definitions.js index e6b64add92..8a3801ccba 100644 --- a/scripts/eslint_rules/lib/check-test-definitions.js +++ b/scripts/eslint_rules/lib/check-test-definitions.js @@ -51,12 +51,16 @@ module.exports = { const sourceCode = context.sourceCode ?? context.getSourceCode(); return { MemberExpression(node) { + if (node.object.type !== 'Identifier' || node.property.type !== 'Identifier') { + return; + } + if ((node.object.name === 'it' || node.object.name === 'describe' || node.object.name === 'itScreenshot') && (node.property.name === 'skip' || node.property.name === 'skipOnPlatforms') && node.parent.type === 'CallExpression') { const testNameNode = node.property.name === 'skip' ? node.parent.arguments[0] : node.parent.arguments[1]; - if(!testNameNode) { + if (!testNameNode) { return; } @@ -78,7 +82,7 @@ module.exports = { }, CallExpression(node) { - if (node.callee.name === 'it' && node.arguments[0]) { + if (node.callee.type === 'Identifier' && node.callee.name === 'it' && node.arguments[0]) { const textValue = getTextValue(node.arguments[0]); if (textValue && TEST_NAME_REGEX.test(textValue)) { diff --git a/scripts/eslint_rules/lib/check-was-shown-methods.js b/scripts/eslint_rules/lib/check-was-shown-methods.js index 239c825584..9d0b8d9870 100644 --- a/scripts/eslint_rules/lib/check-was-shown-methods.js +++ b/scripts/eslint_rules/lib/check-was-shown-methods.js @@ -21,13 +21,40 @@ module.exports = { create: function(context) { return { MethodDefinition(node) { + if (node.key.type !== 'Identifier') { + return; + } + const nodeName = node.key.name; - if (node.parent.parent.superClass?.property?.name === 'Widget' && nodeName === 'wasShown') { + if (nodeName !== 'wasShown') { + return; + } + + const ancestorClass = node.parent.parent; + if (ancestorClass.type !== 'ClassDeclaration') { + return; + } + if ( + ancestorClass.superClass?.type === 'MemberExpression' && + ancestorClass.superClass.property.type === 'Identifier' && + ancestorClass.superClass.property.name === 'Widget' + ) { const topBodyNode = node.value.body.body[0]; - if (!(topBodyNode.type === 'ExpressionStatement' && topBodyNode.expression.type === 'CallExpression' && - topBodyNode.expression.callee.object.type === 'Super' && - topBodyNode.expression.callee.property.name === 'wasShown')) { - context.report({node, message: 'Please make sure the first call in wasShown is to super.wasShown().'}); + if ( + !( + topBodyNode.type === 'ExpressionStatement' && + topBodyNode.expression.type === 'CallExpression' && + topBodyNode.expression.callee.type === 'MemberExpression' && + topBodyNode.expression.callee.object.type === 'Super' && + topBodyNode.expression.callee.property.type === 'Identifier' && + topBodyNode.expression.callee.property.name === 'wasShown' + ) + ) { + context.report({ + node, + message: + 'Please make sure the first call in wasShown is to super.wasShown().', + }); } } } diff --git a/scripts/eslint_rules/lib/enforce-default-import-name.js b/scripts/eslint_rules/lib/enforce-default-import-name.js index 0fd2143b88..6b74104b8c 100644 --- a/scripts/eslint_rules/lib/enforce-default-import-name.js +++ b/scripts/eslint_rules/lib/enforce-default-import-name.js @@ -7,7 +7,7 @@ const path = require('path'); function isStarAsImportSpecifier(specifiers) { - return specifiers.length === 1 && specifiers[0].type === 'ImportNamespaceSpecifier'; + return (specifiers.length === 1 && specifiers[0].type === 'ImportNamespaceSpecifier'); } /** @@ -49,8 +49,9 @@ module.exports = { // conventions for module imports. return; } - const importPath = path.normalize(node.source.value); - const importPathForErrorMessage = node.source.value.replace(/\\/g, '/'); + const value = `${node.source.value}`; + const importPath = path.normalize(value); + const importPathForErrorMessage = value.replace(/\\/g, '/'); const absoluteImportPath = path.resolve(importingDir, importPath); const importNameInCode = node.specifiers[0].local.name; diff --git a/scripts/eslint_rules/lib/enforce-ui-strings-as-const.js b/scripts/eslint_rules/lib/enforce-ui-strings-as-const.js index a5770b5767..5fd676faaa 100644 --- a/scripts/eslint_rules/lib/enforce-ui-strings-as-const.js +++ b/scripts/eslint_rules/lib/enforce-ui-strings-as-const.js @@ -58,8 +58,15 @@ module.exports = { node: declaration, messageId: 'invalidUIStringsObject', fix: fixer => { - const objectEnd = declaration.init.range[1]; - return fixer.insertTextAfterRange([objectEnd - 1, objectEnd], ' as const'); + const objectEnd = declaration.init?.range?.[1]; + if (!objectEnd) { + return null; + } + + return fixer.insertTextAfterRange( + [objectEnd - 1, objectEnd], + ' as const', + ); }, }); }, diff --git a/scripts/eslint_rules/lib/es-modules-import.js b/scripts/eslint_rules/lib/es-modules-import.js index 8ea1901b9b..65d5011c48 100644 --- a/scripts/eslint_rules/lib/es-modules-import.js +++ b/scripts/eslint_rules/lib/es-modules-import.js @@ -175,9 +175,9 @@ module.exports = { if (!node.source) { return; } - const importPath = path.normalize(node.source.value); - - const importPathForErrorMessage = node.source.value.replace(/\\/g, '/'); + const value = `${node.source.value}`; + const importPath = path.normalize(value); + const importPathForErrorMessage = value.replace(/\\/g, '/'); checkImportExtension( importPath, importPathForErrorMessage, @@ -186,12 +186,13 @@ module.exports = { ); }, ImportDeclaration(node) { - if (node.source.value.includes('//')) { + const value = `${node.source.value}`; + if (value.includes('//')) { context.report({ node, messageId: 'doubleSlashInImportPath', fix(fixer) { - const fixedValue = node.source.value.replaceAll('//', '/'); + const fixedValue = value.replaceAll('//', '/'); // Replace the original import string with the fixed one. We need // the extra quotes around the value to ensure we produce valid // JS - else it would end up as `import X from ../some/path.js` @@ -199,8 +200,9 @@ module.exports = { }, }); } - const importPath = path.normalize(node.source.value); - const importPathForErrorMessage = node.source.value.replace(/\\/g, '/'); + + const importPath = path.normalize(value); + const importPathForErrorMessage = value.replace(/\\/g, '/'); checkImportExtension( node.source.value, @@ -210,12 +212,15 @@ module.exports = { ); // Type imports are unrestricted + // @ts-expect-error needs typescript if (node.importKind === 'type') { // `import type ... from ...` syntax return; } + // @ts-expect-error needs typescript if (node.importKind === 'value') { // `import {type ...} from ...` syntax + // @ts-expect-error needs typescript if (node.specifiers.every(spec => spec.importKind === 'type')) { return; } @@ -228,10 +233,7 @@ module.exports = { // // Don't use `importPath` here, as `path.normalize` removes // the `./` from same-folder import paths. - if ( - !node.source.value.startsWith('.') && - !/^[\w\-_]+$/.test(node.source.value) - ) { + if (!value.startsWith('.') && !/^[\w\-_]+$/.test(value)) { context.report({ node, message: diff --git a/scripts/eslint_rules/lib/inject-checkbox-styles.js b/scripts/eslint_rules/lib/inject-checkbox-styles.js index 6cb5cbcf6e..77d015b369 100644 --- a/scripts/eslint_rules/lib/inject-checkbox-styles.js +++ b/scripts/eslint_rules/lib/inject-checkbox-styles.js @@ -62,7 +62,7 @@ module.exports = { // Get the absolute path of the current file's directory, so we can // compare it to COMMON_INPUT_STYLES and see if the file does import the common styles. const absoluteDirectory = path.dirname(path.resolve(filename)); - const fullImportPath = path.resolve(absoluteDirectory, node.source.value); + const fullImportPath = path.resolve(absoluteDirectory, `${node.source.value}`); foundInputStylesImport = fullImportPath === COMMON_INPUT_STYLES; if (foundInputStylesImport) { inputStylesImportedName = node.specifiers[0].local.name; diff --git a/scripts/eslint_rules/lib/l10n-filename-matches.js b/scripts/eslint_rules/lib/l10n-filename-matches.js index d63d0fbceb..65f6e58224 100644 --- a/scripts/eslint_rules/lib/l10n-filename-matches.js +++ b/scripts/eslint_rules/lib/l10n-filename-matches.js @@ -90,8 +90,8 @@ module.exports = { const previousFileLocationArgument = callExpression.arguments[0]; const actualPath = path.join( - frontEndDirectory, - previousFileLocationArgument.value, + frontEndDirectory, + `${previousFileLocationArgument.value}`, ); if (!allowedPathArguments.includes(actualPath)) { const newFileName = currentFileRelativeToFrontEnd.replace(/\\/g, '/'); diff --git a/scripts/eslint_rules/lib/no-assert-equal.js b/scripts/eslint_rules/lib/no-assert-equal.js index c0512c3cc8..9873df3e00 100644 --- a/scripts/eslint_rules/lib/no-assert-equal.js +++ b/scripts/eslint_rules/lib/no-assert-equal.js @@ -30,38 +30,47 @@ module.exports = { return { CallExpression(node) { if ( - node.callee.type === 'MemberExpression' && - node.callee.object.name === 'assert' && - node.callee.property.name === 'equal' + node.callee.type !== 'MemberExpression' || + node.callee.object.type !== 'Identifier' || + node.callee.object.name !== 'assert' || + node.callee.property.type !== 'Identifier' || + node.callee.property.name !== 'equal' ) { - context.report({ - node, - message: - 'assert.equal is non-strict. Use assert.strictEqual or assert.deepEqual to compare objects', - fix(fixer) { - /** - * Get the type of the second argument and try to match it to a assert type - */ - const compareToType = node.arguments.at(1)?.type; - if ( - // Match number or string - compareToType === 'Literal' || - // Match `` string - compareToType === 'TemplateElement' - ) { - return fixer.replaceText(node.callee.property, 'strictEqual'); - } - if ( - // Match any object `{...}` - compareToType === 'ObjectExpression' || - // Match any array `[...]` - compareToType === 'ArrayExpression' - ) { - return fixer.replaceText(node.callee.property, 'deepEqual'); - } - }, - }); + return; } + + const calleeProperty = node.callee.property; + + context.report({ + node, + message: + 'assert.equal is non-strict. Use assert.strictEqual or assert.deepEqual to compare objects', + fix(fixer) { + /** + * Get the type of the second argument and try to match it to a assert type + */ + const compareToType = node.arguments.at(1)?.type; + if ( + // Match number or string + compareToType === 'Literal' || + // Match `` string + // @ts-expect-error + compareToType === 'TemplateElement' + ) { + return fixer.replaceText(calleeProperty, 'strictEqual'); + } + if ( + // Match any object `{...}` + compareToType === 'ObjectExpression' || + // Match any array `[...]` + compareToType === 'ArrayExpression' + ) { + return fixer.replaceText(calleeProperty, 'deepEqual'); + } + + return null; + }, + }); }, }; }, diff --git a/scripts/eslint_rules/lib/no-imperative-dom-api.js b/scripts/eslint_rules/lib/no-imperative-dom-api.js index a62e260158..04a4cdd6bd 100644 --- a/scripts/eslint_rules/lib/no-imperative-dom-api.js +++ b/scripts/eslint_rules/lib/no-imperative-dom-api.js @@ -24,18 +24,22 @@ const widget = require('./no-imperative-dom-api/widget.js'); /** @typedef {import('eslint').Scope.Variable} Variable */ /** @typedef {import('eslint').Scope.Reference} Reference*/ +/** + * @type {import('eslint').Rule.RuleModule} + */ module.exports = { - meta : { - type : 'problem', - docs : { - description : 'Prefer template literals over imperative DOM API calls', - category : 'Possible Errors', + meta: { + type: 'problem', + docs: { + description: 'Prefer template literals over imperative DOM API calls', + category: 'Possible Errors', }, messages: { - preferTemplateLiterals: 'Prefer template literals over imperative DOM API calls', + preferTemplateLiterals: + 'Prefer template literals over imperative DOM API calls', }, - fixable : 'code', - schema : [] // no options + fixable: 'code', + schema: [], // no options }, create : function(context) { const sourceCode = context.getSourceCode(); @@ -60,7 +64,7 @@ module.exports = { } } if (event.type === 'Literal') { - return event.value.toString(); + return event.value?.toString() ?? null; } return null; } diff --git a/scripts/eslint_rules/lib/no-imperative-dom-api/dom-api-devtools-extensions.js b/scripts/eslint_rules/lib/no-imperative-dom-api/dom-api-devtools-extensions.js index 4fdb8e10b7..19a679cb79 100644 --- a/scripts/eslint_rules/lib/no-imperative-dom-api/dom-api-devtools-extensions.js +++ b/scripts/eslint_rules/lib/no-imperative-dom-api/dom-api-devtools-extensions.js @@ -12,7 +12,7 @@ const {isIdentifier} = require('./ast.js'); /** @typedef {import('./dom-fragment.js').DomFragment} DomFragment */ module.exports = { - create : function(context) { + create: function (context) { const sourceCode = context.getSourceCode(); return { @@ -34,7 +34,8 @@ module.exports = { return true; } } - } + return false; + }, }; - } + }, }; diff --git a/scripts/eslint_rules/lib/no-imperative-dom-api/dom-api.js b/scripts/eslint_rules/lib/no-imperative-dom-api/dom-api.js index 1f9e3f5680..4af46203a5 100644 --- a/scripts/eslint_rules/lib/no-imperative-dom-api/dom-api.js +++ b/scripts/eslint_rules/lib/no-imperative-dom-api/dom-api.js @@ -74,8 +74,11 @@ module.exports = { if (isIdentifier(property, 'setAttribute')) { const attribute = firstArg; const value = secondArg; - if (attribute.type === 'Literal' && value.type !== 'SpreadElement') { - domFragment.attributes.push({key: attribute.value.toString(), value}); + if (attribute.type === 'Literal' && attribute.value && value.type !== 'SpreadElement') { + domFragment.attributes.push({ + key: attribute.value.toString(), + value, + }); return true; } } diff --git a/scripts/eslint_rules/lib/no-imperative-dom-api/dom-fragment.js b/scripts/eslint_rules/lib/no-imperative-dom-api/dom-fragment.js index adba79c376..3fe48fca82 100644 --- a/scripts/eslint_rules/lib/no-imperative-dom-api/dom-fragment.js +++ b/scripts/eslint_rules/lib/no-imperative-dom-api/dom-fragment.js @@ -60,7 +60,9 @@ class DomFragment { } } if (key instanceof ClassMember) { - result.references = [...key.references].map(r => ({node: /** @type {EsLintNode} */ (r)})); + result.references = [...key.references].map(r => ({ + node: /** @type {EsLintNode} */ (r), + })); result.initializer = /** @type {EsLintNode} */ (key.initializer); } return result; @@ -90,7 +92,7 @@ class DomFragment { return node; } if (node.type === 'Literal' && !quoteLiterals) { - return node.value.toString(); + return node.value?.toString() ?? ''; } const text = sourceCode.getText(node); if (node.type === 'TemplateLiteral') { @@ -120,16 +122,31 @@ class DomFragment { lineLength += this.tagName.length + 1; } if (this.classList.length) { - appendExpression(`class="${this.classList.map(c => toOutputString(c)).join(' ')}"`); + appendExpression( + `class="${this.classList.map(c => toOutputString(c)).join(' ')}"`, + ); } for (const attribute of this.attributes || []) { - appendExpression(`${attribute.key}=${attributeValue(toOutputString(attribute.value))}`); + appendExpression( + `${attribute.key}=${attributeValue(toOutputString(attribute.value))}`, + ); } for (const eventListener of this.eventListeners || []) { - appendExpression(`@${eventListener.key}=${attributeValue(toOutputString(eventListener.value))}`); + appendExpression( + `@${eventListener.key}=${ + attributeValue( + toOutputString(eventListener.value), + )}`, + ); } for (const binding of this.bindings || []) { - appendExpression(`.${binding.key}=${toOutputString(binding.value, /* quoteLiterals=*/ true)}`); + appendExpression( + `.${binding.key}=${ + toOutputString( + binding.value, + /* quoteLiterals=*/ true, + )}`, + ); } if (this.style.length) { const style = this.style.map(s => `${s.key}:${toOutputString(s.value)}`).join('; '); @@ -147,7 +164,9 @@ class DomFragment { } components.push(`\n${' '.repeat(indent)}`); } - components.push(''); + if (this.tagName) { + components.push(''); + } return components; } diff --git a/scripts/eslint_rules/lib/no-imports-in-directory.js b/scripts/eslint_rules/lib/no-imports-in-directory.js index 9fa574597e..95a7d98ebe 100644 --- a/scripts/eslint_rules/lib/no-imports-in-directory.js +++ b/scripts/eslint_rules/lib/no-imports-in-directory.js @@ -35,8 +35,8 @@ module.exports = { const fileNameOfFileBeingChecked = path.resolve(filename); return { - 'ImportDeclaration'(node) { - const importPath = path.resolve(path.dirname(fileNameOfFileBeingChecked), node.source.value); + ImportDeclaration(node) { + const importPath = path.resolve(path.dirname(fileNameOfFileBeingChecked), `${node.source.value}`); for (const banned of bannedPaths) { if (importPath.includes(banned)) { context.report({ diff --git a/scripts/eslint_rules/lib/no-new-lit-element-components.js b/scripts/eslint_rules/lib/no-new-lit-element-components.js index 8d7d73fa7a..c125f77e19 100644 --- a/scripts/eslint_rules/lib/no-new-lit-element-components.js +++ b/scripts/eslint_rules/lib/no-new-lit-element-components.js @@ -24,7 +24,7 @@ module.exports = { return { ClassDeclaration(node) { // Use `extends LitElement` as a signal. - if (node.superClass?.name !== 'LitElement') { + if (node.superClass?.type !== 'Identifier' || node.superClass?.name !== 'LitElement') { return; } // Existing components are still allowed. diff --git a/scripts/eslint_rules/lib/set-data-type-reference.js b/scripts/eslint_rules/lib/set-data-type-reference.js index 7a54597491..5e8ec36a37 100644 --- a/scripts/eslint_rules/lib/set-data-type-reference.js +++ b/scripts/eslint_rules/lib/set-data-type-reference.js @@ -22,19 +22,20 @@ module.exports = { return { ClassDeclaration(node) { // Only enforce this rule for custom elements - if (!node.superClass || node.superClass.name !== 'HTMLElement') { + if (!node.superClass || node.superClass.type !== 'Identifier' || node.superClass.name !== 'HTMLElement') { return; } const dataSetterDefinition = node.body.body.find(methodDefinition => { - return methodDefinition.kind === 'set' && methodDefinition.key.name === 'data'; + return ( + 'kind' in methodDefinition && methodDefinition.kind === 'set' && methodDefinition.key.name === 'data'); }); - if (!dataSetterDefinition) { + if (!dataSetterDefinition || dataSetterDefinition.type === 'StaticBlock') { return; } - const dataSetterParam = dataSetterDefinition.value.params[0]; + const dataSetterParam = dataSetterDefinition.value?.params?.[0]; if (!dataSetterParam) { context.report( {node: dataSetterDefinition, message: 'A data setter must take a parameter that is explicitly typed.'}); diff --git a/scripts/eslint_rules/lib/static-custom-event-names.js b/scripts/eslint_rules/lib/static-custom-event-names.js index ae3d5021ed..1716c8e44a 100644 --- a/scripts/eslint_rules/lib/static-custom-event-names.js +++ b/scripts/eslint_rules/lib/static-custom-event-names.js @@ -28,13 +28,16 @@ module.exports = { }, create: function(context) { function findConstructorAndSuperCallAndFirstArgumentToSuper(node) { + /** + * @type {{constructor: any, superExpression: any, firstArgumentToSuper: any}} + */ const foundNodes = { constructor: undefined, superExpression: undefined, firstArgumentToSuper: undefined, }; const constructor = node.body.body.find(bodyNode => { - return bodyNode.type === 'MethodDefinition' && bodyNode.key?.name === 'constructor'; + return (bodyNode.type === 'MethodDefinition' && bodyNode.key?.name === 'constructor'); }); if (!constructor) { return foundNodes; @@ -165,7 +168,7 @@ module.exports = { return; } - if (!node.superClass) { + if (!node.superClass || node.superClass.type !== 'Identifier') { return; } if (node.superClass.name !== 'Event') { diff --git a/scripts/eslint_rules/lib/trace-engine-test-timeouts.js b/scripts/eslint_rules/lib/trace-engine-test-timeouts.js index 34ef551f72..a181c493de 100644 --- a/scripts/eslint_rules/lib/trace-engine-test-timeouts.js +++ b/scripts/eslint_rules/lib/trace-engine-test-timeouts.js @@ -23,19 +23,23 @@ module.exports = { }, schema: [] // no options }, - create: function(context) { - const MOCHA_CALLS_TO_CHECK = new Set([ - 'it', - 'before', - 'beforeEach', - 'after', - 'afterEach', - ]); - function walkUpTreeToFindMochaFunctionCall(node) { - if(node.type === 'CallExpression' && node.callee.type === 'Identifier' && MOCHA_CALLS_TO_CHECK.has(node.callee.name)) { + create: function (context) { + const MOCHA_CALLS_TO_CHECK = new Set([ + 'it', + 'before', + 'beforeEach', + 'after', + 'afterEach', + ]); + function walkUpTreeToFindMochaFunctionCall(node) { + if ( + node.type === 'CallExpression' && + node.callee.type === 'Identifier' && + MOCHA_CALLS_TO_CHECK.has(node.callee.name) + ) { return node; } - if(!node || !node.parent) { + if (!node || !node.parent) { return null; } return walkUpTreeToFindMochaFunctionCall(node.parent); @@ -43,14 +47,18 @@ module.exports = { return { MemberExpression(node) { - const objectIsTraceLoader = node.object.type === 'Identifier' && node.object.name === 'TraceLoader'; - if(!objectIsTraceLoader) { + const objectIsTraceLoader = + node.object.type === 'Identifier' && + node.object.name === 'TraceLoader'; + if (!objectIsTraceLoader) { return; } // Find out if this is an await call (which needs the additional test timeout). const callExpression = node.parent; - const isAwait = callExpression.parent && callExpression.parent.type === 'AwaitExpression'; - if(!isAwait) { + const isAwait = + callExpression.parent && + callExpression.parent.type === 'AwaitExpression'; + if (!isAwait) { return; } // We now know that we have await TraceLoader.[something](); @@ -58,24 +66,25 @@ module.exports = { // we can then check that its function is defined via a function // and not as an arrow function. const mochaFunctionCall = walkUpTreeToFindMochaFunctionCall(node); - if(!mochaFunctionCall) { + if (!mochaFunctionCall) { return; } // This code is within a mocha call. If the call is an `it`, we need // the second argument, otherwise we use the first argument (Mocha // functions like `beforeEach` take only a function as the argument.) - const functionArg = mochaFunctionCall.callee.name === 'it' ? - mochaFunctionCall.arguments[1] : - mochaFunctionCall.arguments[0]; + const functionArg = + mochaFunctionCall.callee.name === 'it' + ? mochaFunctionCall.arguments[1] + : mochaFunctionCall.arguments[0]; - if(!functionArg) { + if (!functionArg) { // The node unexpectedly does not have a function passed. The // developer is probably in the middle of writing it, so we should // just stop and leave them to it. return; } - if(functionArg.type === 'ArrowFunctionExpression') { + if (functionArg.type === 'ArrowFunctionExpression') { context.report({ node: functionArg, messageId: 'needsFunction', @@ -87,11 +96,13 @@ module.exports = { functionArg.range[0], functionArg.range[0] + 11, ]; + + // @ts-expect-error the wrapper function is not typed return fixer.replaceTextRange(rangeToReplace, 'async function()'); - } + }, }); } - } + }, }; - } + }, }; diff --git a/scripts/eslint_rules/tests/check-test-definitions.test.js b/scripts/eslint_rules/tests/check-test-definitions.test.js index b9f1c615ed..fa4548da9c 100644 --- a/scripts/eslint_rules/tests/check-test-definitions.test.js +++ b/scripts/eslint_rules/tests/check-test-definitions.test.js @@ -98,7 +98,7 @@ new RuleTester().run('check-test-definitions', rule, { }); `, filename: 'test/e2e/folder/file.ts', - errors: [{message: rule.meta.messages.missingBugId}], + errors: [{messageId: 'missingBugId'}], }, { code: `import {describe, it} from '../../shared/mocha-extensions.js'; @@ -109,7 +109,7 @@ new RuleTester().run('check-test-definitions', rule, { }); `, filename: 'test/e2e/folder/file.ts', - errors: [{message: rule.meta.messages.comment}], + errors: [{messageId: 'comment'}], }, { code: `import {describe, it} from '../../shared/mocha-extensions.js'; @@ -121,7 +121,7 @@ new RuleTester().run('check-test-definitions', rule, { }); `, filename: 'test/e2e/folder/file.ts', - errors: [{message: rule.meta.messages.missingBugId}], + errors: [{messageId: 'missingBugId'}], }, { code: `describe('e2e-test', async () => { @@ -131,7 +131,7 @@ new RuleTester().run('check-test-definitions', rule, { }); `, filename: 'test/e2e/folder/file.ts', - errors: [{message: rule.meta.messages.missingBugId}], + errors: [{messageId: 'missingBugId'}], }, { code: `import {describe, it} from '../../shared/mocha-extensions.js'; @@ -142,7 +142,7 @@ new RuleTester().run('check-test-definitions', rule, { }); `, filename: 'test/e2e/folder/file.ts', - errors: [{message: rule.meta.messages.extraBugId}], + errors: [{messageId: 'extraBugId'}], }, { code: `import {describe, it} from '../../shared/mocha-extensions.js'; @@ -150,10 +150,7 @@ new RuleTester().run('check-test-definitions', rule, { }); `, filename: 'test/e2e/folder/file.ts', - errors: [ - {message: rule.meta.messages.missingBugId}, - {message: rule.meta.messages.comment}, - ], + errors: [{messageId: 'missingBugId'}, {messageId: 'comment'}], }, { code: `import {describe, it} from '../../shared/mocha-extensions.js'; @@ -163,10 +160,7 @@ new RuleTester().run('check-test-definitions', rule, { }); `, filename: 'test/e2e/folder/file.ts', - errors: [ - {message: rule.meta.messages.missingBugId}, - {message: rule.meta.messages.comment}, - ], + errors: [{messageId: 'missingBugId'}, {messageId: 'comment'}], }, ], }); diff --git a/scripts/eslint_rules/tests/jslog-context-list.test.js b/scripts/eslint_rules/tests/jslog-context-list.test.js index 5af54a7c95..a6954b1ba7 100644 --- a/scripts/eslint_rules/tests/jslog-context-list.test.js +++ b/scripts/eslint_rules/tests/jslog-context-list.test.js @@ -2,11 +2,10 @@ // Use of this source code is governed by a BSD-style license that can be // found in the LICENSE file. 'use strict'; -process.env.ESLINT_FAIL_ON_UNKNOWN_JSLOG_CONTEXT_VALUE = 1; +process.env.ESLINT_FAIL_ON_UNKNOWN_JSLOG_CONTEXT_VALUE = 'true'; const rule = require('../lib/jslog-context-list.js'); const {RuleTester} = require('./utils/utils.js'); - new RuleTester().run('jslog-context-list', rule, { invalid: [ { diff --git a/scripts/eslint_rules/tests/no-imports-in-directory.test.js b/scripts/eslint_rules/tests/no-imports-in-directory.test.js index 737f11f970..994db26ffb 100644 --- a/scripts/eslint_rules/tests/no-imports-in-directory.test.js +++ b/scripts/eslint_rules/tests/no-imports-in-directory.test.js @@ -29,7 +29,6 @@ new RuleTester().run('no-imports-in-directory', rule, { ], }, ], - errors: [{messageId: 'invalidImport'}], }, ], invalid: [ diff --git a/scripts/eslint_rules/tests/utils.test.js b/scripts/eslint_rules/tests/utils.test.js index 0ab2c84ed4..29f0ab4ad8 100644 --- a/scripts/eslint_rules/tests/utils.test.js +++ b/scripts/eslint_rules/tests/utils.test.js @@ -7,33 +7,42 @@ const {assert} = require('chai'); const utils = require('../lib/utils.js'); +function getParsedExpression(code) { + const parsed = parser.parse(code).body[0]; + + if (parsed.type !== 'ExpressionStatement') { + throw new Error('Not an expression'); + } + return parsed.expression; +} + describe('eslint utils', () => { describe('isLitHtmlTemplateCall', () => { it('returns true if the code is Lit.html``', () => { const code = 'Lit.html`foo`'; - const parsed = parser.parse(code); - const result = utils.isLitHtmlTemplateCall(parsed.body[0].expression); + const expression = getParsedExpression(code); + const result = utils.isLitHtmlTemplateCall(expression); assert.strictEqual(result, true); }); it('returns true if the code is html``', () => { const code = 'html`foo`'; - const parsed = parser.parse(code); - const result = utils.isLitHtmlTemplateCall(parsed.body[0].expression); + const expression = getParsedExpression(code); + const result = utils.isLitHtmlTemplateCall(expression); assert.strictEqual(result, true); }); it('returns false if the code is Lit.somethingElse``', () => { const code = 'Lit.somethingElse`foo`'; - const parsed = parser.parse(code); - const result = utils.isLitHtmlTemplateCall(parsed.body[0].expression); + const expression = getParsedExpression(code); + const result = utils.isLitHtmlTemplateCall(expression); assert.strictEqual(result, false); }); it('returns false if the code is another tagged template function``', () => { const code = 'notLitHtml`foo`'; - const parsed = parser.parse(code); - const result = utils.isLitHtmlTemplateCall(parsed.body[0].expression); + const expression = getParsedExpression(code); + const result = utils.isLitHtmlTemplateCall(expression); assert.strictEqual(result, false); }); }); @@ -41,29 +50,29 @@ describe('eslint utils', () => { describe('isLitHtmlRenderCall', () => { it('returns true if the code is Lit.render()', () => { const code = 'Lit.render(Lit.html``, this.#shadow)'; - const parsed = parser.parse(code); - const result = utils.isLitHtmlRenderCall(parsed.body[0].expression); + const expression = getParsedExpression(code); + const result = utils.isLitHtmlRenderCall(expression); assert.strictEqual(result, true); }); it('returns true if the code is render()', () => { const code = 'render(html``, this.#shadow)'; - const parsed = parser.parse(code); - const result = utils.isLitHtmlRenderCall(parsed.body[0].expression); + const expression = getParsedExpression(code); + const result = utils.isLitHtmlRenderCall(expression); assert.strictEqual(result, true); }); it('returns false if the code is not render()', () => { const code = 'notRender(html``, this.#shadow)'; - const parsed = parser.parse(code); - const result = utils.isLitHtmlRenderCall(parsed.body[0].expression); + const expression = getParsedExpression(code); + const result = utils.isLitHtmlRenderCall(expression); assert.strictEqual(result, false); }); it('returns false if the code is Lit.notRender()', () => { const code = 'Lit.notRender(html``, this.#shadow)'; - const parsed = parser.parse(code); - const result = utils.isLitHtmlRenderCall(parsed.body[0].expression); + const expression = getParsedExpression(code); + const result = utils.isLitHtmlRenderCall(expression); assert.strictEqual(result, false); }); }); diff --git a/scripts/eslint_rules/tests/utils/utils.js b/scripts/eslint_rules/tests/utils/utils.js index 6d666afbb7..fa9add98a3 100644 --- a/scripts/eslint_rules/tests/utils/utils.js +++ b/scripts/eslint_rules/tests/utils/utils.js @@ -12,7 +12,7 @@ const eslint = require('eslint'); */ class RuleTester extends eslint.RuleTester { /** - * @param {import(eslint).Linter.Config} config + * @param {import('eslint').Linter.Config} config */ constructor(config = {}) { super({ diff --git a/scripts/eslint_rules/tsconfig.json b/scripts/eslint_rules/tsconfig.json new file mode 100644 index 0000000000..3c3bab2899 --- /dev/null +++ b/scripts/eslint_rules/tsconfig.json @@ -0,0 +1,12 @@ +{ + "extends": "../../config/typescript/tsconfig.base.json", + "compilerOptions": { + "module": "NodeNext", + "moduleResolution": "nodenext", + "lib": ["esnext", "dom"], + "outDir": "ignored", + "checkJs": true, + "noEmit": true, + "noImplicitAny": false + } +} diff --git a/scripts/test/run_lint_check.mjs b/scripts/test/run_lint_check.mjs index 9172c84b61..57e56c5016 100644 --- a/scripts/test/run_lint_check.mjs +++ b/scripts/test/run_lint_check.mjs @@ -15,6 +15,7 @@ import { devtoolsRootPath, litAnalyzerExecutablePath, nodePath, + nodeModulesPath, tsconfigJsonPath, } from '../devtools_paths.js'; @@ -30,6 +31,11 @@ const flags = yargs(hideBin(process.argv)) describe: 'Disable cache validations during debugging, useful for custom rule creation/debugging.', }) + .option('tsc', { + type: 'boolean', + default: false, + describe: 'Temperary here while fixing EsLint rule', + }) .usage('$0 []', 'Run the linter on the provided files', yargs => { yargs.positional('files', { describe: 'File(s), glob(s), or directories', @@ -268,6 +274,61 @@ function shouldIgnoreFile(path) { return false; } +async function runEslintRulesTypeCheck(_files) { + const tscPath = join(nodeModulesPath(), '.bin', 'tsc'); + const tsConfigEslintRules = join( + devtoolsRootPath(), + 'scripts', + 'eslint_rules', + 'tsconfig.json', + ); + const args = [tscPath, '-b', tsConfigEslintRules]; + /** + * @returns {Promise<{output: string, error: string, status:boolean}>} + */ + async function runTypeCheck() { + const result = { + output: '', + error: '', + status: false, + }; + + return await new Promise(resolve => { + const litAnalyzerProcess = spawn(nodePath(), args, { + encoding: 'utf-8', + cwd: devtoolsRootPath(), + }); + + litAnalyzerProcess.stdout.on('data', data => { + result.output += `\n${data.toString()}`; + }); + litAnalyzerProcess.stderr.on('data', data => { + result.error += `\n${data.toString()}`; + }); + + litAnalyzerProcess.on('error', message => { + result.error += `\n${message}`; + resolve(result); + }); + + litAnalyzerProcess.on('exit', code => { + result.status = code === 0; + resolve(result); + }); + }); + } + + const result = await runTypeCheck(); + + if (result.output && !result.output.includes('Found 0 problems')) { + console.log(result.output); + } + if (result.error) { + console.log(result.error); + } + return result.status; +} + async function run() { const scripts = []; const styles = []; @@ -287,6 +348,9 @@ async function run() { } const frontEndFiles = scripts.filter(script => script.includes('front_end')); + const esLintRules = scripts.filter(script => + script.includes('scripts/eslint_rules'), + ); let succeed = true; if (scripts.length !== 0) { @@ -298,6 +362,10 @@ async function run() { if (styles.length !== 0) { succeed &&= await runStylelint(styles); } + if (esLintRules.length !== 0 && flags.tsc) { + succeed &&= await runEslintRulesTypeCheck(); + } + return succeed; }