opendev/gerrit
Code Issues Proposed changes

Cleanup eslint rules and fix some eslint warnings

Browse Source 
Change-Id: If4aeaa1bba276ed9c3bfea417af47110f51591f9
This commit is contained in:
Dmitrii Filippov
2020-10-21 17:03:52 +02:00
committed by Luca Milanesio
parent 68aac7fb2e
commit f5d728ede5
9 changed files with 57 additions and 422 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
polygerrit-ui/app/.eslintrc.js
View File

@@ -300,8 +300,6 @@ module.exports = {
{
"files": [
"*.html",
"common-test-setup.js",
"common-test-setup-karma.js",
"*_test.js",
"a11y-test-utils.js",
],

4
polygerrit-ui/app/services/gr-reporting/gr-reporting_impl.ts
View File

@@ -127,8 +127,8 @@ export function initErrorReporter(appContext: AppContext) {
oldOnError(msg, url, line, column, error);
}
if (error) {
line = line || (error as any).lineNumber;
column = column || (error as any).columnNumber;
line = line || error.lineNumber;
column = column || error.columnNumber;
let shortenedErrorStack = msg;
if (error.stack) {
const errorStackLines = error.stack.split('\n');

3
polygerrit-ui/app/test/common-test-setup-karma.ts
View File

@@ -86,6 +86,9 @@ function flushImpl(callback?: () => void): Promise<void> | void {
// Ideally, this function would be a call to Polymer.dom.flush, but that
// doesn't support a callback yet
// (https://github.com/Polymer/polymer-dev/issues/851)
// The type is used only in one place, disable eslint warning instead of
// creating an interface
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(window as any).Polymer.dom.flush();
if (callback) {
nativeSetTimeout(callback, 0);

4
polygerrit-ui/app/test/common-test-setup.ts
View File

@@ -182,9 +182,7 @@ function checkChildAllowed(element: Element) {
`The following node remains in document after the test:
${element.tagName}
Outer HTML:
${element.outerHTML},
Stack trace:
${(element as any).stackTrace}`
${element.outerHTML}`
);
}
function checkGlobalSpace() {

93
polygerrit-ui/app/types/globals.ts
View File

@@ -75,50 +75,50 @@ declare global {
// TODO(TS): should clean up those and removing them may break certain plugin behaviors
// TODO(TS): as @brohlfs suggested, to avoid importing anything from elements/ to types/
// use any for them for now
GrDisplayNameUtils: any;
GrAnnotation: any;
GrAttributeHelper: any;
GrDiffLine: any;
GrDiffLineType: any;
GrDiffGroup: any;
GrDiffGroupType: any;
GrDiffBuilder: any;
GrDiffBuilderSideBySide: any;
GrDiffBuilderImage: any;
GrDiffBuilderUnified: any;
GrDiffBuilderBinary: any;
GrChangeActionsInterface: any;
GrChangeReplyInterface: any;
GrEditConstants: any;
GrDomHooksManager: any;
GrDomHook: any;
GrEtagDecorator: any;
GrThemeApi: any;
SiteBasedCache: any;
FetchPromisesCache: any;
GrRestApiHelper: any;
GrLinkTextParser: any;
GrPluginEndpoints: any;
GrReviewerUpdatesParser: any;
GrPopupInterface: any;
GrCountStringFormatter: any;
GrReviewerSuggestionsProvider: any;
util: any;
Auth: any;
EventEmitter: any;
GrAdminApi: any;
GrAnnotationActionsContext: any;
GrAnnotationActionsInterface: any;
GrChangeMetadataApi: any;
GrEmailSuggestionsProvider: any;
GrGroupSuggestionsProvider: any;
GrEventHelper: any;
GrPluginRestApi: any;
GrRepoApi: any;
GrSettingsApi: any;
GrStylesApi: any;
PluginLoader: any;
GrPluginActionContext: any;
GrDisplayNameUtils: unknown;
GrAnnotation: unknown;
GrAttributeHelper: unknown;
GrDiffLine: unknown;
GrDiffLineType: unknown;
GrDiffGroup: unknown;
GrDiffGroupType: unknown;
GrDiffBuilder: unknown;
GrDiffBuilderSideBySide: unknown;
GrDiffBuilderImage: unknown;
GrDiffBuilderUnified: unknown;
GrDiffBuilderBinary: unknown;
GrChangeActionsInterface: unknown;
GrChangeReplyInterface: unknown;
GrEditConstants: unknown;
GrDomHooksManager: unknown;
GrDomHook: unknown;
GrEtagDecorator: unknown;
GrThemeApi: unknown;
SiteBasedCache: unknown;
FetchPromisesCache: unknown;
GrRestApiHelper: unknown;
GrLinkTextParser: unknown;
GrPluginEndpoints: unknown;
GrReviewerUpdatesParser: unknown;
GrPopupInterface: unknown;
GrCountStringFormatter: unknown;
GrReviewerSuggestionsProvider: unknown;
util: unknown;
Auth: unknown;
EventEmitter: unknown;
GrAdminApi: unknown;
GrAnnotationActionsContext: unknown;
GrAnnotationActionsInterface: unknown;
GrChangeMetadataApi: unknown;
GrEmailSuggestionsProvider: unknown;
GrGroupSuggestionsProvider: unknown;
GrEventHelper: unknown;
GrPluginRestApi: unknown;
GrRepoApi: unknown;
GrSettingsApi: unknown;
GrStylesApi: unknown;
PluginLoader: unknown;
GrPluginActionContext: unknown;
_apiUtils: {};
}
@@ -138,4 +138,9 @@ declare global {
// TODO(TS): replace with composedPath if possible
readonly path: EventTarget[];
}
interface Error {
lineNumber?: number; // non-standard property
columnNumber?: number; // non-standard property
}
}

3
polygerrit-ui/app/utils/common-util.ts
View File

@@ -33,7 +33,8 @@ export function hasOwnProperty(obj: any, prop: PropertyKey) {
}
// TODO(TS): move to common types once we have type utils
// tslint:disable-next-line:no-any Required for constructor signature.
// Required for constructor signature.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
export type Constructor<T> = new (...args: any[]) => T;
/**

160
tools/js/eslint-rules/goog-module-id.js
View File

@@ -1,160 +0,0 @@
/**
* @license
* Copyright (C) 2020 The Android Open Source Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
const fs = require('fs');
const path = require('path');
const jsExt = '.js';
class NonJsValidator {
onProgramEnd(context, node) {
}
onGoogDeclareModuleId(context, node) {
context.report({
message: 'goog.declareModuleId is allowed only in .js files',
node: node,
});
}
}
class JsOnlyValidator {
onProgramEnd(context, node) {
}
onGoogDeclareModuleId(context, node) {
context.report({
message: 'goog.declareModuleId present, but .d.ts file doesn\'t exist. '
+ 'Either remove goog.declareModuleId or add the .d.ts file.',
node: node,
});
}
}
class JsWithDtsValidator {
constructor() {
this._googDeclareModuleIdExists = false;
}
onProgramEnd(context, node) {
if(!this._googDeclareModuleIdExists) {
context.report({
message: 'goog.declareModuleId(...) is missed. ' +
'Either add it or remove the associated .d.ts file.',
node: node,
})
}
}
onGoogDeclareModuleId(context, node) {
if(this._googDeclareModuleIdExists) {
context.report({
message: 'Duplicated goog.declareModuleId.',
node: node,
});
return;
}
const filename = context.getFilename();
this._googDeclareModuleIdExists = true;
const scope = context.getScope();
if(scope.type !== 'global' && scope.type !== 'module') {
context.report({
message: 'goog.declareModuleId is allowed only at the root level.',
node: node,
});
// no return - other problems are possible
}
if(node.arguments.length !== 1) {
context.report({
message: 'goog.declareModuleId must have exactly one parameter.',
node: node,
});
if(node.arguments.length === 0) {
return;
}
}
const argument = node.arguments[0];
if(argument.type !== 'Literal') {
context.report({
message: 'The argument for the declareModuleId method '
+ 'must be a string literal.',
node: argument,
});
return;
}
const pathStart = '/polygerrit-ui/app/';
const index = filename.lastIndexOf(pathStart);
if(index < 0) {
context.report({
message: 'The file located outside of polygerrit-ui/app directory. ' +
'Please check eslint config.',
node: argument,
});
return;
}
const expectedName = 'polygerrit.' +
filename.slice(index + pathStart.length, -jsExt.length)
.replace(/\//g, '.') // Replace all occurrences of '/' with '.'
.replace(/-/g, '$2d'); // Replace all occurrences of '-' with '$2d'
if(argument.value !== expectedName) {
context.report({
message: `Invalid module id. It must be '${expectedName}'.`,
node: argument,
fix: function(fixer) {
return fixer.replaceText(argument, `'${expectedName}'`);
},
});
}
}
}
module.exports = {
meta: {
type: 'problem',
docs: {
description: 'Check that goog.declareModuleId is valid',
category: 'TS imports JS errors',
recommended: false,
},
fixable: "code",
schema: [],
},
create: function (context) {
let fileValidator;
return {
Program: function(node) {
const filename = context.getFilename();
if(filename.endsWith(jsExt)) {
const dtsFilename = filename.slice(0, -jsExt.length) + ".d.ts";
if(fs.existsSync(dtsFilename)) {
fileValidator = new JsWithDtsValidator();
} else {
fileValidator = new JsOnlyValidator();
}
}
else {
fileValidator = new NonJsValidator();
}
},
"Program:exit": function(node) {
fileValidator.onProgramEnd(context, node);
fileValidator = null;
},
'ExpressionStatement > CallExpression[callee.property.name="declareModuleId"][callee.object.name="goog"]': function(node) {
fileValidator.onGoogDeclareModuleId(context, node);
}
};
},
};

101
tools/js/eslint-rules/report-ts-error.js
View File

@@ -1,101 +0,0 @@
/**
* @license
* Copyright (C) 2020 The Android Open Source Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
// While we are migrating to typescript, gerrit can have .d.ts files.
// The option "skipLibCheck" is set to true In the tsconfig.json.
// This is required, because we want to skip type checking in node_modules
// directory - some .d.ts files in 3rd-party modules are incorrect.
// Unfortunately, this options also excludes our own .d.ts files from type
// checking. This rule reports all .ts errors in a file as tslint errors.
function getMassageTextFromChain(chainNode, prefix) {
let nestedMessages = prefix + chainNode.messageText;
if (chainNode.next && chainNode.next.length > 0) {
nestedMessages += "\n";
for (const node of chainNode.next) {
nestedMessages +=
getMassageTextFromChain(node, prefix + " ");
if(!nestedMessages.endsWith('\n')) {
nestedMessages += "\n";
}
}
}
return nestedMessages;
}
function getMessageText(diagnostic) {
if (typeof diagnostic.messageText === 'string') {
return diagnostic.messageText;
}
return getMassageTextFromChain(diagnostic.messageText, "");
}
function getDiagnosticStartAndEnd(diagnostic) {
if(diagnostic.start) {
const file = diagnostic.file;
const start = file.getLineAndCharacterOfPosition(diagnostic.start);
const length = diagnostic.length ? diagnostic.length : 0;
return {
start,
end: file.getLineAndCharacterOfPosition(diagnostic.start + length),
};
}
return {
start: {line:0, character: 0},
end: {line:0, character: 0},
}
}
module.exports = {
meta: {
type: "problem",
docs: {
description: "Reports all typescript problems as linter problems",
category: ".d.ts",
recommended: false
},
schema: [],
},
create: function (context) {
const program = context.parserServices.program;
return {
Program: function(node) {
const sourceFile =
context.parserServices.esTreeNodeToTSNodeMap.get(node);
const allDiagnostics = [
...program.getDeclarationDiagnostics(sourceFile),
...program.getSemanticDiagnostics(sourceFile)];
for(const diagnostic of allDiagnostics) {
const {start, end } = getDiagnosticStartAndEnd(diagnostic);
context.report({
message: getMessageText(diagnostic),
loc: {
start: {
line: start.line + 1,
column: start.character,
},
end: {
line: end.line + 1,
column: end.character,
}
}
});
}
},
};
}
};

109
tools/js/eslint-rules/ts-imports-js.js
View File

@@ -1,109 +0,0 @@
/**
* @license
* Copyright (C) 2020 The Android Open Source Project
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
const path = require('path');
const fs = require('fs');
function checkImportValid(context, node) {
const file = context.getFilename();
const importSource = node.source.value;
if(importSource.startsWith('/')) {
return {
message: 'Do not use absolute path for import.',
};
}
const targetFile = path.resolve(path.dirname(file), importSource);
const extName = path.extname(targetFile);
// There is a polymer.dom.js file, so .dom is not an extension
if(extName !== '' && !targetFile.endsWith('polymer.dom')) {
return {
message: 'Do not specify extensions for import path.',
fix: function(fixer) {
return fixer.replaceText(node.source, `'${importSource.slice(0, -extName.length)}'`);
},
};
}
if(!importSource.startsWith('./') && !importSource.startsWith('../')) {
// Import from node_modules - nothing else to check
return null;
}
if(fs.existsSync(targetFile + ".ts")) {
// .ts file exists - nothing to check
return null;
}
const jsFileExists = fs.existsSync(targetFile + '.js');
const dtsFileExists = fs.existsSync(targetFile + '.d.ts');
if(jsFileExists && !dtsFileExists) {
return {
message: `The '${importSource}.d.ts' file doesn't exist.`
};
}
if(!jsFileExists && dtsFileExists) {
return {
message: `The '${importSource}.js' file doesn't exist.`
};
}
// If both files (.js and .d.ts) don't exist, the error is reported by
// the typescript compiler. Do not report anything from the rule.
return null;
}
module.exports = {
meta: {
type: "problem",
docs: {
description: "Check that TS file can import specific JS file",
category: "TS imports JS errors",
recommended: false
},
schema: [],
fixable: "code",
},
create: function (context) {
return {
Program: function(node) {
const filename = context.getFilename();
if(filename.endsWith('.ts') && !filename.endsWith('.d.ts')) {
return;
}
context.report({
message: 'The rule must be used only with .ts files. ' +
'Check eslint settings.',
node: node,
});
},
ImportDeclaration: function (node) {
const importProblem = checkImportValid(context, node);
if(importProblem) {
context.report({
message: importProblem.message,
node: node.source,
fix: importProblem.fix,
});
}
}
};
}
};