[eslint] prevent using constructor property params in initializers (e…

…lastic#119130)
kibanamachine · Nov 19, 2021 · 584b7a1 · 584b7a1
1 parent f8a036c
commit 584b7a1
Show file tree

Hide file tree

Showing 31 changed files with 430 additions and 139 deletions.
diff --git a/packages/elastic-eslint-config-kibana/.eslintrc.js b/packages/elastic-eslint-config-kibana/.eslintrc.js
@@ -104,5 +104,6 @@ module.exports = {
     '@kbn/eslint/no_async_promise_body': 'error',
     '@kbn/eslint/no_async_foreach': 'error',
     '@kbn/eslint/no_trailing_import_slash': 'error',
+    '@kbn/eslint/no_constructor_args_in_property_initializers': 'error',
   },
 };
diff --git a/packages/kbn-cli-dev-mode/src/log.ts b/packages/kbn-cli-dev-mode/src/log.ts
@@ -20,16 +20,18 @@ export interface Log {
 }
 
 export class CliLog implements Log {
-  public toolingLog = new ToolingLog({
-    level: this.silent ? 'silent' : 'info',
-    writeTo: {
-      write: (msg) => {
-        this.write(msg);
+  public toolingLog: ToolingLog;
+
+  constructor(private readonly silent: boolean) {
+    this.toolingLog = new ToolingLog({
+      level: this.silent ? 'silent' : 'info',
+      writeTo: {
+        write: (msg) => {
+          this.write(msg);
+        },
       },
-    },
-  });
-
-  constructor(private readonly silent: boolean) {}
+    });
+  }
 
   good(label: string, ...args: any[]) {
     if (this.silent) {

diff --git a/packages/kbn-eslint-plugin-eslint/index.js b/packages/kbn-eslint-plugin-eslint/index.js
@@ -16,5 +16,6 @@ module.exports = {
     no_async_promise_body: require('./rules/no_async_promise_body'),
     no_async_foreach: require('./rules/no_async_foreach'),
     no_trailing_import_slash: require('./rules/no_trailing_import_slash'),
+    no_constructor_args_in_property_initializers: require('./rules/no_constructor_args_in_property_initializers'),
   },
 };
diff --git a/packages/kbn-eslint-plugin-eslint/rules/no_async_promise_body.js b/packages/kbn-eslint-plugin-eslint/rules/no_async_promise_body.js
@@ -14,14 +14,10 @@ const esTypes = tsEstree.AST_NODE_TYPES;
 const babelTypes = require('@babel/types');
 
 /** @typedef {import("eslint").Rule.RuleModule} Rule */
-/** @typedef {import("@typescript-eslint/parser").ParserServices} ParserServices */
 /** @typedef {import("@typescript-eslint/typescript-estree").TSESTree.Expression} Expression */
 /** @typedef {import("@typescript-eslint/typescript-estree").TSESTree.ArrowFunctionExpression} ArrowFunctionExpression */
 /** @typedef {import("@typescript-eslint/typescript-estree").TSESTree.FunctionExpression} FunctionExpression */
-/** @typedef {import("@typescript-eslint/typescript-estree").TSESTree.TryStatement} TryStatement */
 /** @typedef {import("@typescript-eslint/typescript-estree").TSESTree.NewExpression} NewExpression */
-/** @typedef {import("typescript").ExportDeclaration} ExportDeclaration */
-/** @typedef {import("eslint").Rule.RuleFixer} Fixer */
 
 const ERROR_MSG =
   'Passing an async function to the Promise constructor leads to a hidden promise being created and prevents handling rejections';

diff --git a/packages/kbn-eslint-plugin-eslint/rules/no_constructor_args_in_property_initializers.js b/packages/kbn-eslint-plugin-eslint/rules/no_constructor_args_in_property_initializers.js
@@ -0,0 +1,103 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+const tsEstree = require('@typescript-eslint/typescript-estree');
+const traverse = require('eslint-traverse');
+const esTypes = tsEstree.AST_NODE_TYPES;
+
+/** @typedef {import("eslint").Rule.RuleModule} Rule */
+/** @typedef {import("@typescript-eslint/typescript-estree").TSESTree.Node} Node */
+/** @typedef {import("@typescript-eslint/typescript-estree").TSESTree.ClassBody} ClassBody */
+/** @typedef {import("@typescript-eslint/typescript-estree").TSESTree.Parameter} Parameter */
+/** @typedef {import("@typescript-eslint/typescript-estree").TSESTree.TSParameterProperty} TSParameterProperty */
+
+/**
+ * @param {Parameter} param
+ * @returns {param is TSParameterProperty}
+ */
+function isTsParameterProperty(param) {
+  return param.type === esTypes.TSParameterProperty;
+}
+
+/**
+ * @param {string} arg
+ */
+const errorMsg = (arg) =>
+  `The constructor argument "${arg}" can't be used in a class property intializer, define the property in the constructor instead`;
+
+/** @type {Rule} */
+module.exports = {
+  meta: {
+    schema: [],
+  },
+  create: (context) => ({
+    ClassBody(_) {
+      const node = /** @type {ClassBody} */ (_);
+
+      const constructor = node.body.find(
+        (n) => n.type === esTypes.MethodDefinition && n.kind === 'constructor'
+      );
+
+      if (!constructor || constructor.type !== esTypes.MethodDefinition) {
+        return;
+      }
+
+      const constructorArgProps = constructor.value.params
+        .filter(isTsParameterProperty)
+        .map((p) => {
+          if (p.parameter.type === esTypes.Identifier) {
+            return p.parameter.name;
+          }
+
+          if (
+            p.parameter.type === esTypes.AssignmentPattern &&
+            p.parameter.left.type === esTypes.Identifier
+          ) {
+            return p.parameter.left.name;
+          }
+        });
+
+      if (!constructorArgProps.length) {
+        return;
+      }
+
+      for (const prop of node.body) {
+        if (prop.type !== esTypes.PropertyDefinition) {
+          continue;
+        }
+
+        const visitor = (path) => {
+          /** @type {Node} node */
+          const node = path.node;
+
+          if (
+            node.type === esTypes.FunctionExpression ||
+            node.type === esTypes.ArrowFunctionExpression
+          ) {
+            return traverse.STOP;
+          }
+
+          if (
+            node.type === esTypes.MemberExpression &&
+            node.object.type === esTypes.ThisExpression &&
+            node.property.type === esTypes.Identifier &&
+            node.property.name &&
+            constructorArgProps.includes(node.property.name)
+          ) {
+            context.report({
+              message: errorMsg(node.property.name),
+              loc: node.property.loc,
+            });
+          }
+        };
+
+        traverse(context, prop, visitor);
+      }
+    },
+  }),
+};
diff --git a/packages/kbn-eslint-plugin-eslint/rules/no_constructor_args_in_property_initializers.test.js b/packages/kbn-eslint-plugin-eslint/rules/no_constructor_args_in_property_initializers.test.js
@@ -0,0 +1,100 @@
+/*
+ * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
+ * or more contributor license agreements. Licensed under the Elastic License
+ * 2.0 and the Server Side Public License, v 1; you may not use this file except
+ * in compliance with, at your election, the Elastic License 2.0 or the Server
+ * Side Public License, v 1.
+ */
+
+const { RuleTester } = require('eslint');
+const rule = require('./no_constructor_args_in_property_initializers');
+const dedent = require('dedent');
+
+const ruleTester = new RuleTester({
+  parser: require.resolve('@typescript-eslint/parser'),
+  parserOptions: {
+    sourceType: 'module',
+    ecmaVersion: 2018,
+    ecmaFeatures: {
+      jsx: true,
+    },
+  },
+});
+
+ruleTester.run('@kbn/eslint/no_constructor_args_in_property_initializers', rule, {
+  valid: [
+    {
+      code: dedent`
+        class Foo {
+          bar = 'baz'
+        }
+      `,
+    },
+    {
+      code: dedent`
+        class Foo {
+          bar = 'baz'
+          constructor(private readonly foo: Box) {}
+        }
+      `,
+    },
+    {
+      code: dedent`
+        class Foo {
+          bar = 'baz'
+          constructor(private readonly foo: () => void) {}
+
+          get = () => {
+            return this.foo()
+          }
+        }
+      `,
+    },
+  ],
+
+  invalid: [
+    // no catch
+    {
+      code: dedent`
+        class Foo {
+          bar = this.foo.split().reverse()
+          constructor(private readonly foo: string) {}
+        }
+      `,
+      errors: [
+        {
+          line: 2,
+          message: `The constructor argument "foo" can't be used in a class property intializer, define the property in the constructor instead`,
+        },
+      ],
+    },
+    {
+      code: dedent`
+        class Foo {
+          bar = this.foo()
+          constructor(private readonly foo: () => void) {}
+        }
+      `,
+      errors: [
+        {
+          line: 2,
+          message: `The constructor argument "foo" can't be used in a class property intializer, define the property in the constructor instead`,
+        },
+      ],
+    },
+    {
+      code: dedent`
+        class Foo {
+          bar = this.foo()
+          constructor(private readonly foo: (() => void) = defaultValue) {}
+        }
+      `,
+      errors: [
+        {
+          line: 2,
+          message: `The constructor argument "foo" can't be used in a class property intializer, define the property in the constructor instead`,
+        },
+      ],
+    },
+  ],
+});
diff --git a/packages/kbn-optimizer/src/worker/bundle_refs_plugin.ts b/packages/kbn-optimizer/src/worker/bundle_refs_plugin.ts
@@ -43,10 +43,12 @@ type ModuleFactory = (data: RequestData, callback: Callback<BundleRefModule>) =>
 export class BundleRefsPlugin {
   private readonly resolvedRefEntryCache = new Map<BundleRef, Promise<string>>();
   private readonly resolvedRequestCache = new Map<string, Promise<string | undefined>>();
-  private readonly ignorePrefix = Path.resolve(this.bundle.contextDir) + Path.sep;
+  private readonly ignorePrefix: string;
   private allowedBundleIds = new Set<string>();
 
-  constructor(private readonly bundle: Bundle, private readonly bundleRefs: BundleRefs) {}
+  constructor(private readonly bundle: Bundle, private readonly bundleRefs: BundleRefs) {
+    this.ignorePrefix = Path.resolve(this.bundle.contextDir) + Path.sep;
+  }
 
   /**
    * Called by webpack when the plugin is passed in the webpack config

diff --git a/packages/kbn-test/src/functional_test_runner/lib/lifecycle_event.ts b/packages/kbn-test/src/functional_test_runner/lib/lifecycle_event.ts
@@ -15,21 +15,27 @@ export type GetArgsType<T extends LifecycleEvent<any>> = T extends LifecycleEven
 export class LifecycleEvent<Args extends readonly any[]> {
   private readonly handlers: Array<(...args: Args) => Promise<void> | void> = [];
 
-  private readonly beforeSubj = this.options.singular
-    ? new Rx.BehaviorSubject(undefined)
-    : new Rx.Subject<void>();
-  public readonly before$ = this.beforeSubj.asObservable();
+  private readonly beforeSubj: Rx.Subject<void>;
+  public readonly before$: Rx.Observable<void>;
 
-  private readonly afterSubj = this.options.singular
-    ? new Rx.BehaviorSubject(undefined)
-    : new Rx.Subject<void>();
-  public readonly after$ = this.afterSubj.asObservable();
+  private readonly afterSubj: Rx.Subject<void>;
+  public readonly after$: Rx.Observable<void>;
 
   constructor(
     private readonly options: {
       singular?: boolean;
     } = {}
-  ) {}
+  ) {
+    this.beforeSubj = this.options.singular
+      ? new Rx.BehaviorSubject<void>(undefined)
+      : new Rx.Subject<void>();
+    this.before$ = this.beforeSubj.asObservable();
+
+    this.afterSubj = this.options.singular
+      ? new Rx.BehaviorSubject<void>(undefined)
+      : new Rx.Subject<void>();
+    this.after$ = this.afterSubj.asObservable();
+  }
 
   public add(fn: (...args: Args) => Promise<void> | void) {
     this.handlers.push(fn);

diff --git a/packages/kbn-test/src/functional_test_runner/lib/lifecycle_phase.ts b/packages/kbn-test/src/functional_test_runner/lib/lifecycle_phase.ts
@@ -19,19 +19,23 @@ export class LifecyclePhase<Args extends readonly any[]> {
 
   public triggered = false;
 
-  private readonly beforeSubj = new Rx.Subject<void>();
-  public readonly before$ = this.beforeSubj.asObservable();
+  private readonly beforeSubj: Rx.Subject<void>;
+  public readonly before$: Rx.Observable<void>;
 
-  private readonly afterSubj = this.options.singular
-    ? new Rx.ReplaySubject<void>(1)
-    : new Rx.Subject<void>();
-  public readonly after$ = this.afterSubj.asObservable();
+  private readonly afterSubj: Rx.Subject<void>;
+  public readonly after$: Rx.Observable<void>;
 
   constructor(
     private readonly options: {
       singular?: boolean;
     } = {}
-  ) {}
+  ) {
+    this.beforeSubj = new Rx.Subject<void>();
+    this.before$ = this.beforeSubj.asObservable();
+
+    this.afterSubj = this.options.singular ? new Rx.ReplaySubject<void>(1) : new Rx.Subject<void>();
+    this.after$ = this.afterSubj.asObservable();
+  }
 
   public add(fn: (...args: Args) => Promise<void> | void) {
     this.handlers.push(fn);

diff --git a/src/core/public/chrome/chrome_service.test.ts b/src/core/public/chrome/chrome_service.test.ts
@@ -21,10 +21,12 @@ import { ChromeService } from './chrome_service';
 import { getAppInfo } from '../application/utils';
 
 class FakeApp implements App {
-  public title = `${this.id} App`;
+  public title: string;
   public mount = () => () => {};
 
-  constructor(public id: string, public chromeless?: boolean) {}
+  constructor(public id: string, public chromeless?: boolean) {
+    this.title = `${this.id} App`;
+  }
 }
 
 const store = new Map();

diff --git a/src/core/public/injected_metadata/injected_metadata_service.ts b/src/core/public/injected_metadata/injected_metadata_service.ts
@@ -69,11 +69,13 @@ export interface InjectedMetadataParams {
  * @internal
  */
 export class InjectedMetadataService {
-  private state = deepFreeze(
-    this.params.injectedMetadata
-  ) as InjectedMetadataParams['injectedMetadata'];
+  private state: InjectedMetadataParams['injectedMetadata'];
 
-  constructor(private readonly params: InjectedMetadataParams) {}
+  constructor(private readonly params: InjectedMetadataParams) {
+    this.state = deepFreeze(
+      this.params.injectedMetadata
+    ) as InjectedMetadataParams['injectedMetadata'];
+  }
 
   public start(): InjectedMetadataStart {
     return this.setup();

diff --git a/src/core/server/plugins/plugins_service.ts b/src/core/server/plugins/plugins_service.ts
@@ -89,10 +89,10 @@ export interface PluginsServiceDiscoverDeps {
 /** @internal */
 export class PluginsService implements CoreService<PluginsServiceSetup, PluginsServiceStart> {
   private readonly log: Logger;
-  private readonly prebootPluginsSystem = new PluginsSystem(this.coreContext, PluginType.preboot);
+  private readonly prebootPluginsSystem: PluginsSystem<PluginType.preboot>;
   private arePrebootPluginsStopped = false;
   private readonly prebootUiPluginInternalInfo = new Map<PluginName, InternalPluginInfo>();
-  private readonly standardPluginsSystem = new PluginsSystem(this.coreContext, PluginType.standard);
+  private readonly standardPluginsSystem: PluginsSystem<PluginType.standard>;
   private readonly standardUiPluginInternalInfo = new Map<PluginName, InternalPluginInfo>();
   private readonly configService: IConfigService;
   private readonly config$: Observable<PluginsConfig>;
@@ -105,6 +105,8 @@ export class PluginsService implements CoreService<PluginsServiceSetup, PluginsS
     this.config$ = coreContext.configService
       .atPath<PluginsConfigType>('plugins')
       .pipe(map((rawConfig) => new PluginsConfig(rawConfig, coreContext.env)));
+    this.prebootPluginsSystem = new PluginsSystem(this.coreContext, PluginType.preboot);
+    this.standardPluginsSystem = new PluginsSystem(this.coreContext, PluginType.standard);
   }
 
   public async discover({ environment }: PluginsServiceDiscoverDeps): Promise<DiscoveredPlugins> {