From 3431206c0e47010dffc046036d976d623cb82648 Mon Sep 17 00:00:00 2001 From: Pascal Hartig Date: Thu, 22 Apr 2021 05:20:56 -0700 Subject: [PATCH] Add linter for naked `console.error`s Summary: `console.error(err)` are hard to identify in the codebase especially as we often don't have reliable stack trace information. I've already cleaned up a bunch of them manually by going after the most high-firing ones; this should make it easier to identify the remaining ones. Reviewed By: jknoxville Differential Revision: D27913964 fbshipit-source-id: 0ff6624a0c083829846550b40954945d655b7cf6 --- desktop/.eslintrc.js | 1 + desktop/eslint-plugin-flipper/src/index.ts | 4 ++ .../noConsoleErrorWithoutContext.node.ts | 52 +++++++++++++++ .../src/rules/noConsoleErrorWithoutContext.ts | 64 +++++++++++++++++++ 4 files changed, 121 insertions(+) create mode 100644 desktop/eslint-plugin-flipper/src/rules/__tests__/noConsoleErrorWithoutContext.node.ts create mode 100644 desktop/eslint-plugin-flipper/src/rules/noConsoleErrorWithoutContext.ts diff --git a/desktop/.eslintrc.js b/desktop/.eslintrc.js index 2a4717341..147a95180 100644 --- a/desktop/.eslintrc.js +++ b/desktop/.eslintrc.js @@ -85,6 +85,7 @@ module.exports = { 'node/no-extraneous-require': [2, {allowModules: builtInModules}], 'flipper/no-relative-imports-across-packages': [2], 'flipper/no-electron-remote-imports': [1], + 'flipper/no-console-error-without-context': [1], }, settings: { 'import/resolver': { diff --git a/desktop/eslint-plugin-flipper/src/index.ts b/desktop/eslint-plugin-flipper/src/index.ts index 5b3bda50b..ec4faa3ee 100644 --- a/desktop/eslint-plugin-flipper/src/index.ts +++ b/desktop/eslint-plugin-flipper/src/index.ts @@ -13,10 +13,14 @@ import noRelativeImportsAcrossPackages, { import noElectronRemoteImports, { RULE_NAME as noElectronRemoteImportsRuleName, } from './rules/noElectronRemoteImports'; +import noConsoleErrorWithoutContext, { + RULE_NAME as noConsoleErrorWithoutContextRuleName, +} from './rules/noConsoleErrorWithoutContext'; module.exports = { rules: { [noRelativeImportsAcrossPackagesRuleName]: noRelativeImportsAcrossPackages, [noElectronRemoteImportsRuleName]: noElectronRemoteImports, + [noConsoleErrorWithoutContextRuleName]: noConsoleErrorWithoutContext, }, }; diff --git a/desktop/eslint-plugin-flipper/src/rules/__tests__/noConsoleErrorWithoutContext.node.ts b/desktop/eslint-plugin-flipper/src/rules/__tests__/noConsoleErrorWithoutContext.node.ts new file mode 100644 index 000000000..09c903510 --- /dev/null +++ b/desktop/eslint-plugin-flipper/src/rules/__tests__/noConsoleErrorWithoutContext.node.ts @@ -0,0 +1,52 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @format + */ + +import {TSESLint} from '@typescript-eslint/experimental-utils'; +import rule, {RULE_NAME} from '../noConsoleErrorWithoutContext'; + +const tester = new TSESLint.RuleTester({ + parser: require.resolve('@typescript-eslint/parser'), + parserOptions: { + sourceType: 'module', + ecmaVersion: 2020, + }, +}); + +tester.run(RULE_NAME, rule, { + valid: [ + { + code: `console.error("I've made a big mistake:", err);`, + filename: __filename, + }, + { + code: `console.error("This should never happen.");`, + filename: __filename, + }, + { + code: `console.error("Failed to open user settings: " + err);`, + filename: __filename, + }, + { + code: `console.warn(e);`, + filename: __filename, + }, + ], + invalid: [ + { + code: `console.error(err);`, + filename: __filename, + errors: [{messageId: 'noConsoleErrorWithoutContext'}], + }, + { + code: `console.error(err, "Too late for context.");`, + filename: __filename, + errors: [{messageId: 'noConsoleErrorWithoutContext'}], + }, + ], +}); diff --git a/desktop/eslint-plugin-flipper/src/rules/noConsoleErrorWithoutContext.ts b/desktop/eslint-plugin-flipper/src/rules/noConsoleErrorWithoutContext.ts new file mode 100644 index 000000000..bc2bf364f --- /dev/null +++ b/desktop/eslint-plugin-flipper/src/rules/noConsoleErrorWithoutContext.ts @@ -0,0 +1,64 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @format + */ + +import {TSESTree} from '@typescript-eslint/experimental-utils'; +import {createESLintRule} from '../utils/createEslintRule'; + +type Options = []; + +export type MessageIds = 'noConsoleErrorWithoutContext'; +export const RULE_NAME = 'no-console-error-without-context'; + +export default createESLintRule({ + name: RULE_NAME, + meta: { + type: 'problem', + docs: { + description: 'TBD', + category: 'Possible Errors', + recommended: 'warn', + }, + schema: [], + messages: { + noConsoleErrorWithoutContext: + '"Naked" console.error calls are hard to identify without providing a context. Please add a static hint, e.g. `console.error("Reading user config failed", err);`', + }, + }, + defaultOptions: [], + create(context) { + return { + ExpressionStatement(node: TSESTree.ExpressionStatement) { + if (node.expression.type === 'CallExpression') { + const callee = node.expression.callee; + const isConsoleError = + callee.type === 'MemberExpression' && + callee.object.type === 'Identifier' && + callee.object.name === 'console' && + callee.property.type === 'Identifier' && + callee.property.name === 'error'; + + if ( + isConsoleError && + // If this is an Identifier it means the first argument is an object, e.g. + // console.error(err); + // Whereas a literal implies there's a static context given, e.g. + // console.error("My identifiable error: ", err); + // Expressions like concatenations are also fine here. + node.expression.arguments[0]?.type === 'Identifier' + ) { + context.report({ + node, + messageId: 'noConsoleErrorWithoutContext', + }); + } + } + }, + }; + }, +});