Repository: artusjs/core (ts)

Issue:
[RFC] 重新明确 Plugin 及 Framework 配置项的合并逻辑
问题描述：

- 目前的 `Plugin` / `Framework` 配置项中，`path` / `package` 不应当同时存在（优先级 / fallback 令人头大且意义不大）
- 现行实现中，除 `Framework` 自身单独设计递归结构时，可以确保单个配置项不合并，其他 Plugin 会在 `ConfigurationHandler` 中按照统一逻辑处理深合并，由此可能造成单个插件合并结果中 `path` / `package` 同时存在
  - 深合并有两种场景会被触发：
    - 跨环境配置，如同时存在 `plugin.dev.ts` 和 `plugin.default.ts`
    - 用户项目和依赖的 `Framework` 或 `Framework` 依赖链各项间，同时存在 `plugin.${env}.ts`
- 深合并逻辑不应被直接替换为覆盖逻辑，存在下级同时配置了 `enable` 和 `package`，上级需要单独修改 `enable`，package 配置项需被继承

待选解决方案：

1. 撤销 `package` 字段，统一采用 `path`，要求用户使用 `path: getPath('packageName')` 指定插件
2. 利用 `deepmerge` 中的 `custommerge` 方法，对含有 `enable` / `path` / `package` 的配置项单独处理 cc @JerrysShan （`ConfigurationHandler` 需限定这一行为在 `plugin` 作用域）
3. 在合并链中增加隐式规定：
  - 如果已有配置不包括 `path` 或 `package`，则允许待合并项合入
  - 如果已有配置包括 `path` 或 `package`，待合并项中不包括这两字段，则允许待合并项合入
  - 如果已有配置包括 `path` 或 `package`，待合并项中包括这两字段，则不允许待合并项合入，显式抛错
  - 合并链目前不完全是线性的，存在内/外层和环境两集，需保证优先级为 外层指定环境 > 外层default > 内层指定环境 > 内层 default
 
Co-Author-by: @wengeezhang 


Pull request:
refactor: 支持 Plugin.package 字段在扫描阶段的预处理
- Plugin 抽象 getPath 能力、增加 path/package 互斥判断
- Config 相关 Loader 重构，支持对 PluginConfig 进行预处理
- Scanner 局部逻辑重构
   - 重构 getAllConfig 兼容上述插件处理逻辑
   - 修正多环境条件下的配置数据隔离 BUG
   - 从 Manifest 中移除 PluginConfig，防止运行时对其的重新合并诱发两阶段配置状态不一致
 - 增加相关单元测试用例

resolve #109 

Patch:
diff --git a/src/loader/impl/config.ts b/src/loader/impl/config.ts
index f1209c3..9532d61 100644
--- a/src/loader/impl/config.ts
+++ b/src/loader/impl/config.ts
@@ -6,6 +6,12 @@ import { DefineLoader } from '../decorator';
 import { ManifestItem, Loader, LoaderFindOptions } from '../types';
 import compatibleRequire from '../../utils/compatible_require';
 import { isMatch } from '../../utils';
+import { Application } from '../../types';
+
+export interface ConfigFileMeta {
+  env: string;
+  namespace?: string;
+}
 
 @DefineLoader('config')
 class ConfigLoader implements Loader {
@@ -15,6 +21,14 @@ class ConfigLoader implements Loader {
     this.container = container;
   }
 
+  protected get app(): Application {
+    return this.container.get(ArtusInjectEnum.Application);
+  }
+
+  protected get configurationHandler(): ConfigurationHandler {
+    return this.container.get(ConfigurationHandler);
+  }
+
   static async is(opts: LoaderFindOptions): Promise<boolean> {
     if (this.isConfigDir(opts)) {
       return isMatch(opts.filename, CONFIG_PATTERN);
@@ -28,24 +42,39 @@ class ConfigLoader implements Loader {
   }
 
   async load(item: ManifestItem) {
-    const originConfigObj = await compatibleRequire(item.path);
+    const { namespace, env } = await this.getConfigFileMeta(item);
+    let configObj = await this.loadConfigFile(item);
+    if (namespace) {
+      configObj = {
+        [namespace]: configObj
+      };
+    }
+    this.configurationHandler.setConfig(env, configObj);
+  }
+
+  protected async getConfigFileMeta(item: ManifestItem): Promise<ConfigFileMeta> {
     let [namespace, env, extname] = item.filename.split('.');
     if (!extname) {
       // No env flag, set to Default
       env = ARTUS_DEFAULT_CONFIG_ENV.DEFAULT;
     }
+    const meta: ConfigFileMeta = {
+      env
+    };
+    if (namespace !== 'config') {
+      meta.namespace = namespace;
+    }
+    return meta
+  }
+
+  protected async loadConfigFile(item: ManifestItem): Promise<Record<string, any>> {
+    const originConfigObj = await compatibleRequire(item.path);
     let configObj = originConfigObj;
     if(typeof originConfigObj === 'function') {
       const app = this.container.get(ArtusInjectEnum.Application);
       configObj = originConfigObj(app);
     }
-    if (namespace !== 'config') {
-      configObj = {
-        [namespace]: configObj
-      };
-    }
-    const configHandler = this.container.get(ConfigurationHandler);
-    configHandler.setConfig(env, configObj);
+    return configObj;
   }
 }
 
diff --git a/src/loader/impl/framework_config.ts b/src/loader/impl/framework_config.ts
index 01ff60b..9c84706 100644
--- a/src/loader/impl/framework_config.ts
+++ b/src/loader/impl/framework_config.ts
@@ -1,8 +1,7 @@
-import ConfigurationHandler from '../../configuration';
+import { FrameworkObject } from '../../configuration';
 import { DefineLoader } from '../decorator';
 import { ManifestItem, Loader, LoaderFindOptions } from '../types';
-import compatibleRequire from '../../utils/compatible_require';
-import { ArtusInjectEnum, ARTUS_DEFAULT_CONFIG_ENV, FRAMEWORK_PATTERN } from '../../constant';
+import { FRAMEWORK_PATTERN } from '../../constant';
 import ConfigLoader from './config';
 import { isMatch } from '../../utils';
 
@@ -20,20 +19,9 @@ class FrameworkConfigLoader extends ConfigLoader implements Loader {
   }
 
   async load(item: ManifestItem) {
-    const originConfigObj = await compatibleRequire(item.path);
-    let [, env, extname] = item.filename.split('.');
-    if (!extname) {
-      // No env flag, set to Default
-      env = ARTUS_DEFAULT_CONFIG_ENV.DEFAULT;
-    }
-    let configObj = originConfigObj;
-    if (typeof originConfigObj === 'function') {
-      const app = this.container.get(ArtusInjectEnum.Application);
-      configObj = originConfigObj(app);
-    }
-
-    const configHandler = this.container.get(ConfigurationHandler);
-    configHandler.addFramework(item.source || 'app', configObj, {
+    const { env } = await this.getConfigFileMeta(item);
+    const configObj = await this.loadConfigFile(item) as FrameworkObject;
+    this.configurationHandler.addFramework(item.source || 'app', configObj, {
       env,
       unitName: item.unitName || '',
     });
diff --git a/src/loader/impl/plugin_config.ts b/src/loader/impl/plugin_config.ts
index 8a727ea..9afceeb 100644
--- a/src/loader/impl/plugin_config.ts
+++ b/src/loader/impl/plugin_config.ts
@@ -1,4 +1,6 @@
 import { PLUGIN_CONFIG_PATTERN } from '../../constant';
+import { ArtusPlugin } from '../../plugin';
+import { PluginConfigItem } from '../../plugin/types';
 import { isMatch } from '../../utils';
 import { DefineLoader } from '../decorator';
 import { ManifestItem, Loader, LoaderFindOptions } from '../types';
@@ -15,7 +17,25 @@ class PluginConfigLoader extends ConfigLoader implements Loader {
   }
 
   async load(item: ManifestItem) {
-    await super.load(item);
+    const { env } = await this.getConfigFileMeta(item);
+    let configObj = await this.loadConfigFile(item);
+    for (const pluginName of Object.keys(configObj)) {
+      const pluginConfigItem: PluginConfigItem = configObj[pluginName];
+      if (pluginConfigItem.package) {
+        // convert package to path when load plugin config
+        if (pluginConfigItem.path) {
+          throw new Error(`Plugin ${pluginName} config can't have both package and path at ${item.path}`);
+        }
+        if (pluginConfigItem.enable) {
+          pluginConfigItem.path = ArtusPlugin.getPath(pluginConfigItem.package);
+        }
+        delete pluginConfigItem.package;
+        configObj[pluginName] = pluginConfigItem;
+      }
+    }
+    this.configurationHandler.setConfig(env, {
+      plugin: configObj
+    });
   }
 }
 
diff --git a/src/plugin/base.ts b/src/plugin/base.ts
index 84be67b..132df0f 100644
--- a/src/plugin/base.ts
+++ b/src/plugin/base.ts
@@ -3,23 +3,32 @@ import path from 'path';
 type PluginMap = Map<string, BasePlugin>;
 
 export class BasePlugin implements Plugin {
+  static getPath(packageName: string): string {
+    return path.resolve(require.resolve(`${packageName}/package.json`), '..');
+  }
+
   public name: string;
   public enable: boolean;
-  public importPath: string;
+  public importPath: string = '';
   public metadata: Partial<PluginMetadata> = {};
   public metaFilePath: string = '';
 
   constructor(name: string, configItem: PluginConfigItem) {
     this.name = name;
-    let importPath = configItem.path ?? '';
-    if (configItem.package) {
-      importPath = path.resolve(require.resolve(`${configItem.package}/package.json`), '..');
-    }
-    if (!importPath) {
-      throw new Error(`Plugin ${name} need have path or package field`);
-    }
-    this.importPath = importPath;
     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;
+    }
   }
 
   async init() { }
diff --git a/src/plugin/impl.ts b/src/plugin/impl.ts
index 9bc5814..3c92387 100644
--- a/src/plugin/impl.ts
+++ b/src/plugin/impl.ts
@@ -5,6 +5,9 @@ import { exisis } from '../utils/fs';
 
 export class ArtusPlugin extends BasePlugin {
   async init() {
+    if (!this.enable) {
+      return;
+    }
     await this.checkAndLoadMetadata();
     if (!this.metadata) {
       throw new Error(`${this.name} is not have metadata.`);
diff --git a/src/scanner/scan.ts b/src/scanner/scan.ts
index 6f80fbd..21645d8 100644
--- a/src/scanner/scan.ts
+++ b/src/scanner/scan.ts
@@ -10,10 +10,9 @@ import {
   DEFAULT_LOADER_LIST_WITH_ORDER,
   LOADER_NAME_META,
 } from '../constant';
-import { Manifest, ManifestItem } from '../loader';
+import { LoaderFactory, Manifest, ManifestItem } from '../loader';
 import { ScannerOptions, WalkOptions } from './types';
 import ConfigurationHandler, { ConfigObject } from '../configuration';
-import { ConfigLoader } from '../loader/impl';
 import { FrameworkConfig, FrameworkHandler } from '../framework';
 import { BasePlugin, PluginFactory } from '../plugin';
 import { ScanUtils } from './utils';
@@ -21,9 +20,9 @@ import { ScanUtils } from './utils';
 export class Scanner {
   private moduleExtensions = ['.js', '.json', '.node'];
   private options: ScannerOptions;
-  private itemMap: Map<string, ManifestItem[]>;
-  private configList: ConfigObject[];
-  private configHandle: ConfigurationHandler;
+  private itemMap: Map<string, ManifestItem[]> = new Map();
+  private tmpConfigStore: Map<string, ConfigObject[]> = new Map();
+  private configHandle: ConfigurationHandler = new ConfigurationHandler();
 
   constructor(options: Partial<ScannerOptions> = {}) {
     this.options = {
@@ -36,7 +35,9 @@ export class Scanner {
       excluded: DEFAULT_EXCLUDES.concat(options.excluded ?? []),
       extensions: [...new Set(this.moduleExtensions.concat(options.extensions ?? [], ['.yaml']))],
     };
+  }
 
+  private async initItemMap(): Promise<void> {
     this.itemMap = new Map(
       this.options.loaderListGenerator(DEFAULT_LOADER_LIST_WITH_ORDER).map(loaderNameOrClazz => {
         if (typeof loaderNameOrClazz === 'string') {
@@ -50,8 +51,6 @@ export class Scanner {
         return [loaderName, []];
       })
     );
-    this.configList = [];
-    this.configHandle = new ConfigurationHandler();
   }
 
   private async scanEnvList(root: string): Promise<string[]> {
@@ -87,6 +86,9 @@ export class Scanner {
   }
 
   private async scanManifestByEnv(root: string, env: string): Promise<Manifest> {
+    // 0. init clean itemMap
+    await this.initItemMap();
+  
     const config = await this.getAllConfig(root, env);
 
     // 1. scan all file in framework
@@ -95,8 +97,12 @@ export class Scanner {
       await this.walk(frameworkDir, this.formatWalkOptions('framework', frameworkDir));
     }
 
+
     // 2. scan all file in plugin
-    this.configList.forEach(config => this.configHandle.setConfig(env, config));
+    if (this.tmpConfigStore.has(env)) {
+      const configList = this.tmpConfigStore.get(env) ?? [];
+      configList.forEach(config => this.configHandle.setConfig(env, config));
+    }
     const { plugin } = this.configHandle.getMergedConfig(env);
     const pluginSortedList = await PluginFactory.createFromConfig(plugin || {});
     for (const plugin of pluginSortedList.reverse()) {
@@ -134,28 +140,48 @@ export class Scanner {
     });
   }
 
-  private async getAllConfig(root: string, env: string) {
-    const configDir = this.getConfigDir(root, this.options.configDir);
+  private async getAllConfig(baseDir: string, env: string) {
+    const configDir = this.getConfigDir(baseDir, this.options.configDir);
     if (!configDir) {
       return {};
     }
-    const configFileList = await fs.readdir(path.resolve(root, configDir));
+    const root = path.resolve(baseDir, configDir);
+    const configFileList = await fs.readdir(root);
     const container = new Container(ArtusInjectEnum.DefaultContainerName);
     container.set({ type: ConfigurationHandler });
-    const configHandler = new ConfigLoader(container);
-    for (const pluginConfigFile of configFileList) {
-      const extname = path.extname(pluginConfigFile);
-      if (ScanUtils.isExclude(pluginConfigFile, extname, this.options.excluded, this.options.extensions)) {
-        continue;
+    const loaderFactory = LoaderFactory.create(container);
+    const configItemList: (ManifestItem|null)[] = await Promise.all(configFileList.map(async filename => {
+      const extname = path.extname(filename);
+      if (ScanUtils.isExclude(filename, extname, this.options.excluded, this.options.extensions)) {
+        return null;
       }
-      await configHandler.load({
-        path: path.join(root, configDir, pluginConfigFile),
-        extname: extname,
-        filename: pluginConfigFile,
+      let loader = await loaderFactory.findLoaderName({
+        filename,
+        baseDir,
+        root,
+        configDir
       });
+      if (loader === 'framework-config') {
+        // SEEME: framework-config is a special loader, cannot be used when scan, need refactor later
+        loader = 'config';
+      }
+      return {
+        path: path.resolve(root, filename),
+        extname,
+        filename,
+        loader,
+        source: 'config',
+      };
+    }));
+    await loaderFactory.loadItemList(configItemList.filter(v => v) as ManifestItem[]);
+    const configurationHandler = container.get(ConfigurationHandler);
+    const config = configurationHandler.getMergedConfig(env);
+    let configList = [config];
+    if (this.tmpConfigStore.has(env)) {
+      // equal unshift config to configList
+      configList = configList.concat(this.tmpConfigStore.get(env) ?? []);
     }
-    const config = container.get(ConfigurationHandler).getMergedConfig(env);
-    this.configList.unshift(config);
+    this.tmpConfigStore.set(env, configList);
     return config;
   }
 
@@ -213,7 +239,10 @@ export class Scanner {
       items = items.concat(unitItems);
     }
     relative && items.forEach(item => (item.path = path.relative(appRoot, item.path)));
-    return items;
+    return items.filter(item => (
+      // remove PluginConfig to avoid re-merge on application running
+      item.loader !== 'plugin-config'
+    ));
   }
 
   private async writeFile(filename: string = 'manifest.json', data: string) {


Test patch:
diff --git a/test/fixtures/app_koa_with_ts/src/config/plugin.default.ts b/test/fixtures/app_koa_with_ts/src/config/plugin.default.ts
index 9ef66e0..29c6920 100644
--- a/test/fixtures/app_koa_with_ts/src/config/plugin.default.ts
+++ b/test/fixtures/app_koa_with_ts/src/config/plugin.default.ts
@@ -9,4 +9,8 @@ export default {
     enable: false,
     path: path.resolve(__dirname, '../mysql_plugin')
   },
+  testDuplicate: {
+    enable: false,
+    package: 'unimportant-package'
+  },
 };
diff --git a/test/fixtures/app_koa_with_ts/src/config/plugin.dev.ts b/test/fixtures/app_koa_with_ts/src/config/plugin.dev.ts
new file mode 100644
index 0000000..62e5cea
--- /dev/null
+++ b/test/fixtures/app_koa_with_ts/src/config/plugin.dev.ts
@@ -0,0 +1,8 @@
+import path from 'path';
+
+export default {
+  testDuplicate: {
+    enable: true,
+    path: path.resolve(__dirname, '../test_duplicate_plugin')
+  },
+};
diff --git a/test/fixtures/app_koa_with_ts/src/test_duplicate_plugin/meta.yaml b/test/fixtures/app_koa_with_ts/src/test_duplicate_plugin/meta.yaml
new file mode 100644
index 0000000..d039e8d
--- /dev/null
+++ b/test/fixtures/app_koa_with_ts/src/test_duplicate_plugin/meta.yaml
@@ -0,0 +1,1 @@
+name: testDuplicate
diff --git a/test/scanner.test.ts b/test/scanner.test.ts
index 9d18c02..f56a78c 100644
--- a/test/scanner.test.ts
+++ b/test/scanner.test.ts
@@ -8,13 +8,13 @@ describe('test/scanner.test.ts', () => {
         const scanner = new Scanner({ needWriteFile: false, extensions: ['.ts', '.js', '.json'] });
         const scanResults = await scanner.scan(path.resolve(__dirname, './fixtures/app_koa_with_ts'));
         const { default: manifest } = scanResults;
-        expect(Object.entries(scanResults).length).toBe(1);
+        expect(Object.entries(scanResults).length).toBe(2);
         expect(manifest).toBeDefined();
         expect(manifest.items).toBeDefined();
         // console.log('manifest', manifest);
-        expect(manifest.items.length).toBe(12);
+        expect(manifest.items.length).toBe(11);
 
-        expect(manifest.items.filter(item => item.loader === 'plugin-config').length).toBe(1);
+        expect(manifest.items.filter(item => item.loader === 'plugin-config').length).toBe(0);
         expect(manifest.items.filter(item => item.loader === 'plugin-meta').length).toBe(1);
         expect(manifest.items.filter(item => item.loader === 'exception').length).toBe(1);
         expect(manifest.items.filter(item => item.loader === 'lifecycle-hook-unit').length).toBe(2);
@@ -23,7 +23,15 @@ describe('test/scanner.test.ts', () => {
 
         expect(manifest.items.filter(item => item.unitName === 'redis').length).toBe(2);
         expect(manifest.items.filter(item => item.unitName === 'mysql').length).toBe(0);
-        expect(manifest.items.filter(item => item.source === 'app').length).toBe(10);
+        expect(manifest.items.filter(item => item.source === 'app').length).toBe(9);
+
+        const { dev: devManifest } = scanResults;
+        // console.log('devManifest', devManifest);
+        expect(devManifest).toBeDefined();
+        expect(devManifest.items).toBeDefined();
+        expect(devManifest.items.length).toBe(12);
+        expect(devManifest.items.filter(item => item.loader === 'plugin-meta').length).toBe(2);
+        expect(devManifest.items.find(item => item.unitName === 'testDuplicate')).toBeDefined();
     });
 
     it('should scan module with custom loader', async () => {