Repository: artusjs/core
Pull request #199 (2022-09-19 08:36:21)

refactor(plugin): simplify impl
此 PR 简化了 Plugin 相关类的实现，对齐了统一的 Interface 命名和使用规则（*Type）

codecov-commenter: # [Codecov](https://codecov.io/gh/artusjs/core/pull/199?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Report
Base: **90.68**% // Head: **91.75**% // Increases project coverage by **`+1.06%`** :tada:
> Coverage data is based on head [(`81aa6c8`)](https://codecov.io/gh/artusjs/core/pull/199?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) compared to base [(`f71fa5a`)](https://codecov.io/gh/artusjs/core/commit/f71fa5aba5b071b7bfddf205e7985074a81771ec?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).
> Patch coverage: 93.18% of modified lines in pull request are covered.

> :exclamation: Current head 81aa6c8 differs from pull request most recent head 10de9d4. Consider uploading reports for the commit 10de9d4 to get more accurate results

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master     #199      +/-   ##
==========================================
+ Coverage   90.68%   91.75%   +1.06%     
==========================================
  Files          48       46       -2     
  Lines        1106     1091      -15     
  Branches      200      198       -2     
==========================================
- Hits         1003     1001       -2     
+ Misses        103       90      -13     
```


| [Impacted Files](https://codecov.io/gh/artusjs/core/pull/199?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | |
|---|---|---|
| [src/plugin/impl.ts](https://codecov.io/gh/artusjs/core/pull/199/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL3BsdWdpbi9pbXBsLnRz) | `84.31% <89.65%> (+5.14%)` | :arrow_up: |
| [src/loader/impl/plugin\_config.ts](https://codecov.io/gh/artusjs/core/pull/199/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL2xvYWRlci9pbXBsL3BsdWdpbl9jb25maWcudHM=) | `96.15% <100.00%> (ø)` | |
| [src/plugin/common.ts](https://codecov.io/gh/artusjs/core/pull/199/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL3BsdWdpbi9jb21tb24udHM=) | `100.00% <100.00%> (ø)` | |
| [src/plugin/factory.ts](https://codecov.io/gh/artusjs/core/pull/199/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL3BsdWdpbi9mYWN0b3J5LnRz) | `90.90% <100.00%> (+25.28%)` | :arrow_up: |
| [src/plugin/index.ts](https://codecov.io/gh/artusjs/core/pull/199/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL3BsdWdpbi9pbmRleC50cw==) | `100.00% <100.00%> (ø)` | |
| [src/scanner/scan.ts](https://codecov.io/gh/artusjs/core/pull/199/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-c3JjL3NjYW5uZXIvc2Nhbi50cw==) | `95.23% <100.00%> (ø)` | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/artusjs/core/pull/199?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None).


Patch:
diff --git a/src/application.ts b/src/application.ts
index 57076fe..31c6bfb 100644
--- a/src/application.ts
+++ b/src/application.ts
@@ -7,7 +7,7 @@ import { LoaderFactory, Manifest } from './loader';
 import { Application, ApplicationInitOptions } from './types';
 import Trigger from './trigger';
 import ConfigurationHandler from './configuration';
-import { Logger } from './logger';
+import { Logger, LoggerType } from './logger';
 
 export class ArtusApplication implements Application {
   public manifest?: Manifest;
@@ -48,6 +48,10 @@ export class ArtusApplication implements Application {
     return this.container.get(ConfigurationHandler);
   }
 
+  get logger(): LoggerType {
+    return this.container.get(Logger);
+  }
+
   loadDefaultClass() {
     // load Artus default clazz
     this.container.set({ id: Container, value: this.container });
diff --git a/src/loader/impl/plugin_config.ts b/src/loader/impl/plugin_config.ts
index aa16eb5..cbed336 100644
--- a/src/loader/impl/plugin_config.ts
+++ b/src/loader/impl/plugin_config.ts
@@ -1,5 +1,5 @@
 import { PLUGIN_CONFIG_PATTERN } from '../../constant';
-import { ArtusPlugin } from '../../plugin';
+import { getPackagePath } from '../../plugin/common';
 import { PluginConfigItem } from '../../plugin/types';
 import { isMatch } from '../../utils';
 import { DefineLoader } from '../decorator';
@@ -29,7 +29,7 @@ class PluginConfigLoader extends ConfigLoader implements Loader {
           );
         }
         const loaderState = item.loaderState as { baseDir: string };
-        pluginConfigItem.path = ArtusPlugin.getPath(pluginConfigItem.package, [loaderState?.baseDir]);
+        pluginConfigItem.path = getPackagePath(pluginConfigItem.package, [loaderState?.baseDir]);
         delete pluginConfigItem.package;
         configObj[pluginName] = pluginConfigItem;
       }
diff --git a/src/plugin/base.ts b/src/plugin/base.ts
deleted file mode 100644
index 0118dda..0000000
--- a/src/plugin/base.ts
+++ /dev/null
@@ -1,57 +0,0 @@
-import { Plugin, PluginConfigItem, PluginMetadata } from "./types";
-import path from 'path';
-type PluginMap = Map<string, BasePlugin>;
-
-export class BasePlugin implements Plugin {
-  static getPath(packageName: string, paths?: string[]): string {
-    const opts = paths ? { paths } : undefined;
-    return path.resolve(require.resolve(packageName, opts), '..');
-  }
-
-  public name: string;
-  public enable: boolean;
-  public importPath = '';
-  public metadata: Partial<PluginMetadata> = {};
-  public metaFilePath = '';
-
-  constructor(name: string, configItem: PluginConfigItem) {
-    this.name = name;
-    this.enable = configItem.enable ?? false;
-    if (this.enable) {
-      let importPath = configItem.path ?? '';
-      if (configItem.package) {
-        if (importPath) {
-          throw new Error(`plugin ${name} config error, package and path can't be set at the same time.`);
-        }
-        importPath = BasePlugin.getPath(configItem.package);
-      }
-      if (!importPath) {
-        throw new Error(`Plugin ${name} need have path or package field`);
-      }
-      this.importPath = importPath;
-    }
-  }
-
-  // eslint-disable-next-line @typescript-eslint/no-empty-function
-  async init() { }
-
-  checkDepExisted(pluginMap: PluginMap) {
-    for (const { name: pluginName, optional } of this.metadata.dependencies ?? []) {
-      const instance = pluginMap.get(pluginName);
-      if (!instance || !instance.enable) {
-        if (optional) {
-          // TODO: use artus logger instead
-          console.warn(`Plugin ${this.name} need have optional dependence: ${pluginName}.`);
-        } else {
-          throw new Error(`Plugin ${this.name} need have dependence: ${pluginName}.`);
-        }
-      }
-    }
-  }
-
-  getDepEdgeList(): [string, string][] {
-    return this.metadata.dependencies
-      ?.filter(({ optional }) => !optional)
-      ?.map(({ name: depPluginName }) => [this.name, depPluginName]) ?? [];
-  }
-}
diff --git a/src/plugin/common.ts b/src/plugin/common.ts
index 7931b1b..e9362e8 100644
--- a/src/plugin/common.ts
+++ b/src/plugin/common.ts
@@ -1,7 +1,8 @@
-import { Plugin } from './types';
+import path from 'path';
+import { PluginType } from './types';
 
 // A utils function that toplogical sort plugins
-export function topologicalSort(pluginInstanceMap: Map<string, Plugin>, pluginDepEdgeList: [string, string][]): string[] {
+export function topologicalSort(pluginInstanceMap: Map<string, PluginType>, pluginDepEdgeList: [string, string][]): string[] {
   const res: string[] = [];
   const indegree: Map<string, number> = new Map();
 
@@ -31,3 +32,9 @@ export function topologicalSort(pluginInstanceMap: Map<string, Plugin>, pluginDe
   }
   return res;
 }
+
+// A util function of get package path for plugin
+export function getPackagePath(packageName: string, paths?: string[]): string {
+  const opts = paths ? { paths } : undefined;
+  return path.resolve(require.resolve(packageName, opts), '..');
+}
diff --git a/src/plugin/factory.ts b/src/plugin/factory.ts
index 7a49c45..e8cdd42 100644
--- a/src/plugin/factory.ts
+++ b/src/plugin/factory.ts
@@ -1,19 +1,18 @@
-import { BasePlugin } from './base';
 import { topologicalSort } from './common';
-import { ArtusPlugin } from './impl';
-import { PluginConfigItem } from './types';
+import { Plugin } from './impl';
+import { PluginConfigItem, PluginCreateOptions, PluginMap, PluginType } from './types';
 
 export class PluginFactory {
-  static async create(name: string, item: PluginConfigItem): Promise<BasePlugin> {
-    const pluginInstance = new ArtusPlugin(name, item);
+  static async create(name: string, item: PluginConfigItem, opts?: PluginCreateOptions): Promise<PluginType> {
+    const pluginInstance = new Plugin(name, item, opts);
     await pluginInstance.init();
     return pluginInstance;
   }
 
-  static async createFromConfig(config: Record<string, PluginConfigItem>): Promise<BasePlugin[]> {
-    const pluginInstanceMap: Map<string, BasePlugin> = new Map();
+  static async createFromConfig(config: Record<string, PluginConfigItem>, opts?: PluginCreateOptions): Promise<PluginType[]> {
+    const pluginInstanceMap: PluginMap = new Map();
     for (const [name, item] of Object.entries(config)) {
-      const pluginInstance = await PluginFactory.create(name, item);
+      const pluginInstance = await PluginFactory.create(name, item, opts);
       if (pluginInstance.enable) {
         pluginInstanceMap.set(name, pluginInstance);
       }
@@ -31,19 +30,4 @@ export class PluginFactory {
     }
     return pluginSortResult.map(name => pluginInstanceMap.get(name)!);
   }
-
-  static filterDuplicatePlugins(plugins: BasePlugin[]): BasePlugin[] {
-    const exists: Map<string, boolean> = new Map();
-    const filtedPlugins: BasePlugin[] = [];
-    for (const plugin of plugins) {
-      const key = plugin.importPath;
-      if (exists.get(key)) {
-        continue;
-      }
-      exists.set(key, true);
-      filtedPlugins.push(plugin);
-    }
-
-    return filtedPlugins;
-  }
 }
diff --git a/src/plugin/impl.ts b/src/plugin/impl.ts
index a6257fe..5226886 100644
--- a/src/plugin/impl.ts
+++ b/src/plugin/impl.ts
@@ -1,10 +1,39 @@
 import path from 'path';
-import { BasePlugin } from './base';
 import { loadMetaFile } from '../utils/load_meta_file';
 import { exisis } from '../utils/fs';
 import { PLUGIN_META_FILENAME } from '../constant';
+import { PluginConfigItem, PluginCreateOptions, PluginMap, PluginMetadata, PluginType } from './types';
+import { getPackagePath } from './common';
+import { LoggerType } from '../logger';
+
+export class Plugin implements PluginType {
+  public name: string;
+  public enable: boolean;
+  public importPath = '';
+  public metadata: Partial<PluginMetadata> = {};
+  public metaFilePath = '';
+
+  private logger?: LoggerType;
+
+  constructor(name: string, configItem: PluginConfigItem, opts?: PluginCreateOptions) {
+    this.name = name;
+    this.enable = configItem.enable ?? false;
+    if (this.enable) {
+      let importPath = configItem.path ?? '';
+      if (configItem.package) {
+        if (importPath) {
+          throw new Error(`plugin ${name} config error, package and path can't be set at the same time.`);
+        }
+        importPath = getPackagePath(configItem.package);
+      }
+      if (!importPath) {
+        throw new Error(`Plugin ${name} need have path or package field`);
+      }
+      this.importPath = importPath;
+    }
+    this.logger = opts?.logger;
+  }
 
-export class ArtusPlugin extends BasePlugin {
   async init() {
     if (!this.enable) {
       return;
@@ -18,6 +47,25 @@ export class ArtusPlugin extends BasePlugin {
     }
   }
 
+  public checkDepExisted(pluginMap: PluginMap) {
+    for (const { name: pluginName, optional } of this.metadata.dependencies ?? []) {
+      const instance = pluginMap.get(pluginName);
+      if (!instance || !instance.enable) {
+        if (optional) {
+          this.logger?.warn(`Plugin ${this.name} need have optional dependence: ${pluginName}.`);
+        } else {
+          throw new Error(`Plugin ${this.name} need have dependence: ${pluginName}.`);
+        }
+      }
+    }
+  }
+
+  public getDepEdgeList(): [string, string][] {
+    return this.metadata.dependencies
+      ?.filter(({ optional }) => !optional)
+      ?.map(({ name: depPluginName }) => [this.name, depPluginName]) ?? [];
+  }
+
   private async checkAndLoadMetadata() {
     // check import path
     if (!await exisis(this.importPath)) {
diff --git a/src/plugin/index.ts b/src/plugin/index.ts
index a2aec1e..90c6a5a 100644
--- a/src/plugin/index.ts
+++ b/src/plugin/index.ts
@@ -1,5 +1,3 @@
-import {} from './types';
-
-export * from './base';
+export * from './types';
 export * from './impl';
 export * from './factory';
diff --git a/src/plugin/types.ts b/src/plugin/types.ts
index dbc782e..e40ea0e 100644
--- a/src/plugin/types.ts
+++ b/src/plugin/types.ts
@@ -1,12 +1,13 @@
-export enum PluginType {
-  simple = 'simple',
-  module = 'module',
+import { LoggerType } from '../logger';
+
+export interface PluginCreateOptions {
+  logger?: LoggerType;
 }
 
 export interface PluginMetadata {
   name: string;
   dependencies?: PluginDependencyItem[];
-  type?: PluginType;
+  type?: 'simple' | 'module' | string;
   configDir?: string
   exclude?: string[];
 }
@@ -22,7 +23,9 @@ export interface PluginConfigItem {
   package?: string;
 }
 
-export interface Plugin {
+export type PluginMap = Map<string, PluginType>;
+
+export interface PluginType {
   name: string;
   enable: boolean;
   importPath: string;
@@ -30,6 +33,6 @@ export interface Plugin {
   metaFilePath: string;
 
   init(): Promise<void>;
-  checkDepExisted(map: Map<string, Plugin>): void;
+  checkDepExisted(map: PluginMap): void;
   getDepEdgeList(): [string, string][];
 }
diff --git a/src/scanner/scan.ts b/src/scanner/scan.ts
index a686414..6e2bd8f 100644
--- a/src/scanner/scan.ts
+++ b/src/scanner/scan.ts
@@ -16,7 +16,7 @@ import { LoaderFactory, Manifest, ManifestItem } from '../loader';
 import { ScannerOptions, WalkOptions } from './types';
 import ConfigurationHandler, { ConfigObject } from '../configuration';
 import { FrameworkConfig, FrameworkHandler } from '../framework';
-import { BasePlugin, PluginFactory } from '../plugin';
+import { PluginType, PluginFactory } from '../plugin';
 import { ScanUtils } from './utils';
 import { PluginConfigItem, PluginMetadata } from '../plugin/types';
 import { getConfigMetaFromFilename } from '../loader/utils/config_file_meta';
@@ -116,7 +116,9 @@ export class Scanner {
     }
     const { plugin } = this.configHandle.getMergedConfig(env);
     const pluginConfig = deepmerge.all([plugin || {}, this.options.plugin || {}]) as Record<string, PluginConfigItem>;
-    const pluginSortedList = await PluginFactory.createFromConfig(pluginConfig);
+    const pluginSortedList = await PluginFactory.createFromConfig(pluginConfig, {
+      logger: this.app.logger,
+    });
     for (const plugin of pluginSortedList) {
       if (!plugin.enable) continue;
       this.setPluginMeta(plugin);
@@ -149,7 +151,7 @@ export class Scanner {
     await ScanUtils.walk(root, options);
   }
 
-  private setPluginMeta(plugin: BasePlugin) {
+  private setPluginMeta(plugin: PluginType) {
     const metaList = this.itemMap.get('plugin-meta') ?? [];
     metaList.push({
       path: plugin.metaFilePath,
diff --git a/src/types.ts b/src/types.ts
index 1d263b2..3d8c580 100644
--- a/src/types.ts
+++ b/src/types.ts
@@ -2,6 +2,7 @@ import { Container } from '@artus/injection';
 import { BaseContext } from '@artus/pipeline';
 import { HookFunction } from './lifecycle';
 import { Manifest } from './loader';
+import { LoggerType } from './logger';
 
 export interface ApplicationLifecycle {
   configWillLoad?: HookFunction;
@@ -23,6 +24,9 @@ export interface Application {
   manifest?: Manifest;
   config?: Record<string, any>;
 
+  // getter
+  logger: LoggerType
+
   load(manifest: Manifest): Promise<this>;
   run(): Promise<void>;
   registerHook(hookName: string, hookFn: HookFunction): void;


Test patch:
diff --git a/test/plugin.test.ts b/test/plugin.test.ts
index 57a0761..cbe9a41 100644
--- a/test/plugin.test.ts
+++ b/test/plugin.test.ts
@@ -1,6 +1,6 @@
 import 'reflect-metadata';
 import path from 'path';
-import { ArtusPlugin, PluginFactory } from '../src';
+import { Logger, Plugin, PluginFactory } from '../src';
 
 const pluginPrefix = 'fixtures/plugins';
 
@@ -45,7 +45,7 @@ describe('test/app.test.ts', () => {
       const pluginList = await PluginFactory.createFromConfig(mockPluginConfig);
       expect(pluginList.length).toEqual(3);
       pluginList.forEach(plugin => {
-        expect(plugin).toBeInstanceOf(ArtusPlugin);
+        expect(plugin).toBeInstanceOf(Plugin);
         expect(plugin.enable).toBeTruthy();
       });
       expect(pluginList.map(plugin => plugin.name)).toStrictEqual(['plugin-c', 'plugin-b', 'plugin-a']);
@@ -193,10 +193,12 @@ describe('test/app.test.ts', () => {
       const originWarn = console.warn;
       const mockWarnFn = jest.fn();
       console.warn = mockWarnFn;
-      const pluginList = await PluginFactory.createFromConfig(mockPluginConfig);
+      const pluginList = await PluginFactory.createFromConfig(mockPluginConfig, {
+        logger: new Logger(),
+      });
       expect(pluginList.length).toEqual(1);
       pluginList.forEach(plugin => {
-        expect(plugin).toBeInstanceOf(ArtusPlugin);
+        expect(plugin).toBeInstanceOf(Plugin);
         expect(plugin.enable).toBeTruthy();
       });
       expect(mockWarnFn).toBeCalledWith(`Plugin plugin-d need have optional dependence: plugin-e.`);